[ty] support NewTypes of float and complex#21886
Merged
oconnor663 merged 5 commits intomainfrom Dec 12, 2025
Merged
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
oconnor663
commented
Dec 10, 2025
oconnor663
commented
Dec 10, 2025
crates/ty_python_semantic/resources/mdtest/annotations/new_types.md
Outdated
Show resolved
Hide resolved
|
oconnor663
commented
Dec 10, 2025
oconnor663
force-pushed
the
newtype_float
branch
from
0b66863 to
22f0db0
Compare
oconnor663
changed the title
NewTypes of float and complexNewTypes of float and complex
oconnor663
force-pushed
the
newtype_float
branch
from
22f0db0 to
88ddd96
Compare
oconnor663
commented
Dec 11, 2025
oconnor663
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
|
AlexWaygood
approved these changes
Dec 11, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Nice, I like the semantics and the implementation here!
carljm
approved these changes
Dec 11, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
I also like these semantics and this implementation; nice work!
| } | ||
| // If we get here, there is no `ClassType` (because this newtype is cyclic), and we don't | ||
| // call `f` at all. | ||
| // If we get here, there is no `ClassType` (because this newtype is either float/complex or |
Contributor
There was a problem hiding this comment.
It might be worth explicitly commenting here that mapping base class types is used for normalization and applying type mappings, and neither of these apply to int/float/complex (they are already fully normalized and not generic), so it's fine to ignore them here.
Comment on lines
13908
to
13954
| /// Returns true if this union is equivalent to `int | float`, which is what `float` expands | ||
| /// into in type position. | ||
| pub(crate) fn is_int_float(self, db: &'db dyn Db) -> bool { | ||
| let elements = self.elements(db); | ||
| if elements.len() != 2 { | ||
| return false; | ||
| } | ||
| let mut has_int = false; | ||
| let mut has_float = false; | ||
| for element in elements { | ||
| if let Type::NominalInstance(nominal) = element | ||
| && let Some(known) = nominal.known_class(db) | ||
| { | ||
| match known { | ||
| KnownClass::Int => has_int = true, | ||
| KnownClass::Float => has_float = true, | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
| has_int && has_float | ||
| } | ||
|
|
||
| /// Returns true if this union is equivalent to `int | float | complex`, which is what | ||
| /// `complex` expands into in type position. | ||
| pub(crate) fn is_int_float_complex(self, db: &'db dyn Db) -> bool { | ||
| let elements = self.elements(db); | ||
| if elements.len() != 3 { | ||
| return false; | ||
| } | ||
| let mut has_int = false; | ||
| let mut has_float = false; | ||
| let mut has_complex = false; | ||
| for element in elements { | ||
| if let Type::NominalInstance(nominal) = element | ||
| && let Some(known) = nominal.known_class(db) | ||
| { | ||
| match known { | ||
| KnownClass::Int => has_int = true, | ||
| KnownClass::Float => has_float = true, | ||
| KnownClass::Complex => has_complex = true, | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
| has_int && has_float && has_complex | ||
| } |
Contributor
There was a problem hiding this comment.
This could be a single method which returns an option of an enum, since these are mutually exclusive possibilities that require effectively the same work to check... that would be slightly less redundant and slightly more efficient? But I think it doesn't really matter, this way is totally fine.
oconnor663
force-pushed
the
newtype_float
branch
from
737beba to
2689148
Compare
Contributor
Author
|
This auto-merge was ultimately correct, but I'm concerned that it happened before CI was finished running on the last "clippy fix" commit. I guess I don't understand the rules for auto-merge? Edit: Ah Brent set me straight, this is a settings thing: 
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
astral-sh/ty#1818
There is a known-failing test case in this first draft, which is related to a very hacky bit that I'm pretty sure is wrong. Feedback needed :) Comments below.Second pass: allow the two special-cased unions as
NewTypeBasevariants, and remove the presumption that the concrete base type is aClassType.