Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Dec 11, 2024

Summary

This PR removes the fallback _ branch at the end of Type::is_subtype_of(): the function now uses an exhaustive match over the first element of the (self, target) pair. This means that we can have much more confidence that we've covered all cases, and it means that it's impossible to forget to update this function if we add more variants to Type in the future (which we will).

The PR reworks our handling of most literal types in the match statement so that they explicitly fallback to builtin types. This has several advantages:

  • We now understand that Type::ModuleLiteral("typing") is a subtype of types.ModuleType
  • We now understand that Type::SliceLiteral([:2]) is a subtype of builtins.slice
  • We now understand that all heterogeneous tuple types are subtypes of Type::Instance(Tuple) (which represents the static set of all possible tuples, so in Python type annotations it would be written as tuple[object, ...])
  • We will naturally understand that all string literal types are subtypes of typing.Sequence as soon as we understand generics -- we won't have to make any manual updates to this function
  • We can get rid of the explicit handling of builtins.object being the top type from the function; we now naturally understand this without any special-casing due to the fact that we understand that all classes have object in their MROs.

Test Plan

Several new subtyping tests have been added, and all existing tests pass.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Dec 11, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood force-pushed the alex/subtype-of-exhaustive branch 3 times, most recently from 7c953a8 to 41ba61a Compare December 12, 2024 18:18
@AlexWaygood AlexWaygood marked this pull request as ready for review December 12, 2024 18:23
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!!

@AlexWaygood AlexWaygood force-pushed the alex/subtype-of-exhaustive branch from 41ba61a to cdc9015 Compare December 13, 2024 11:20
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 13, 2024

Thanks again! Would appreciate it if somebody could take another quick look before I land, since this is all quite subtle stuff

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Did you re-run the property tests on the latest version of the PR?

@AlexWaygood AlexWaygood force-pushed the alex/subtype-of-exhaustive branch from 8951d9b to c433e88 Compare December 13, 2024 19:15
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 13, 2024

Okay, I had to add a couple of new branches to is_disjoint_from to make the property tests happy (we weren't recognising that tuple[()] was disjoint from Literal[object], which was causing is_subtype_from not to be transitive w.r.t. intersections, since the Type::Intersection branch in is_subtype_of calls is_disjoint_from if there are negative elements in the intersection).

This PR now causes no new property-test failures relative to main! I tested using this invocation:

QUICKCHECK_TESTS=100000 cargo test -p red_knot_python_semantic -- --ignored types::property_tests::stable

The types::property_tests::stable::assignable_to_is_reflexive still fails instantly on this branch, but it also fails on main, so I didn't look at that too much. All other property tests pass.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@AlexWaygood AlexWaygood enabled auto-merge (squash) December 13, 2024 19:30
@AlexWaygood AlexWaygood merged commit 4b2b126 into main Dec 13, 2024
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/subtype-of-exhaustive branch December 13, 2024 19:31
dcreager added a commit that referenced this pull request Dec 13, 2024
* main:
  [red-knot] Use `type[Unknown]` rather than `Unknown` as the fallback metaclass for invalid classes (#14961)
  [red-knot] Make `is_subtype_of` exhaustive (#14924)
  [red-knot] Alphabetize rules (#14960)
  [red-knot] Understand `Annotated` (#14950)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants