[ty] Only unsoundly upcast type[] types to their constructor Callable type during assignability checks, not during redundancy/subtyping checks#23834
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 87.10%. The percentage of expected errors that received a diagnostic held steady at 77.81%. The number of fully passing files held steady at 63/131. |
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
|
…ble` type during assignability checks, not during redundancy/subtyping checks
a689da6 to
6c58dfc
Compare
|
| static_assert(is_subtype_of(type[A], Callable[[int], A])) | ||
| static_assert(not is_subtype_of(type[A], Callable[[str], A])) | ||
|
|
||
| static_assert(is_subtype_of(type[B], Callable[[str], B])) | ||
| static_assert(not is_subtype_of(type[B], Callable[[int], B])) |
There was a problem hiding this comment.
the assignability relation still needs to hold here, but it still has good coverage in type_properties/is_assignable_to.md. I considered putting the new tests in this file, since that's where the old tests being removed were, but my new tests intermingle is_subtype_of, is_assignable_to and union-simplification assertions, so I don't think they're a great fit for this file.
| let upcasted = if let Type::SubclassOf(inner) = self { | ||
| match relation { | ||
| TypeRelation::Subtyping | ||
| | TypeRelation::SubtypingAssuming | ||
| | TypeRelation::Redundancy { .. } => { |
There was a problem hiding this comment.
optional nit: The two fallback cases below are the same, so you could merge this into
| let upcasted = if let Type::SubclassOf(inner) = self { | |
| match relation { | |
| TypeRelation::Subtyping | |
| | TypeRelation::SubtypingAssuming | |
| | TypeRelation::Redundancy { .. } => { | |
| let upcasted = if let Type::SubclassOf(inner) = self | |
| && matches!(relation, | |
| TypeRelation::Subtyping | |
| | TypeRelation::SubtypingAssuming | |
| | TypeRelation::Redundancy { .. }) { |
(cargo fmt will want to rearrange that whitespace a bit)
There was a problem hiding this comment.
yeah, but I wanted an exhaustive match here so we don't forget to update it if/when we add more type relations in the future. I can refactor to avoid the repeated fallback logic while keeping the exhaustive match, though
There was a problem hiding this comment.
hmm, actually getting rid of the duplication is a bit annoying and the repetition is pretty minimal, so I'm inclined to leave it as is? Happy to fix it up in a followup if you disagree, though :-)
Fixes astral-sh/ty#2981
Test plan
QUICKCHECK_TESTS=1000000 cargo test --profile=profiling -p ty_python_semantic -- --ignored types::property_tests::stablelocally, observed no failures