[ty] Make tuple instantiations sound#18987
Merged
AlexWaygood merged 2 commits intomainfrom Jun 27, 2025
Merged
Conversation
Contributor
|
AlexWaygood
force-pushed
the
alex/sound-tuples
branch
from
2e0d2fb to
0b3b067
Compare
Member
Author
|
The new errors on |
carljm
approved these changes
Jun 27, 2025
dcreager
reviewed
Jun 27, 2025
| | Self::Float | ||
| | Self::Enum | ||
| | Self::ABCMeta | ||
| | KnownClass::Iterable |
Member
There was a problem hiding this comment.
nit: Self::Iterable to be consistent with the other arms
| Some(KnownClass::Tuple) if overload_index == 1 => { | ||
| if let [Some(argument)] = overload.parameter_types() { | ||
| let overridden_return = | ||
| argument.into_tuple().map(Type::Tuple).unwrap_or_else(|| { |
Member
There was a problem hiding this comment.
I think this would avoid unwrapping the argument type just to then rewrap it:
Suggested change
| argument.into_tuple().map(Type::Tuple).unwrap_or_else(|| { | |
| argument.filter(Type::is_tuple).unwrap_or_else(|| { |
(though it depends on an is_tuple method that may not exist yet!)
Member
Author
There was a problem hiding this comment.
argument is a type rather than an Option here so I think this would have to be
Suggested change
| argument.into_tuple().map(Type::Tuple).unwrap_or_else(|| { | |
| argument.into_tuple().filter(Type::is_tuple).unwrap_or_else(|| { |
Adding a Type::is_tuple method just for that feels a bit unnecessary to me? No strong opinion though
| Type::GenericAlias(alias) => { | ||
| let instantiated = Type::instance(db, ClassType::from(alias)); | ||
|
|
||
| let parameters = if alias.origin(db).is_known(db, KnownClass::Tuple) { |
Member
There was a problem hiding this comment.
I think I follow the constructors here, but can you put a comment with the Python syntax of the signature you're trying to construct here?
| let spec = alias.specialization(db).tuple(db); | ||
| let mut parameter = | ||
| Parameter::positional_only(Some(Name::new_static("iterable"))) | ||
| .with_annotated_type(instantiated); |
Member
There was a problem hiding this comment.
With the Tuple::resize method that #18948 introduces, we can make this work for any tuple, not just one with exactly the same size as the type being constructed. I also have plans to have try_iterator return a tuple spec describing the return values of the iterator, which would then give us what we need for this to work for any iterable type.
Member
Author
There was a problem hiding this comment.
With the
Tuple::resizemethod that #18948 introduces, we can make this work for any tuple, not just one with exactly the same size as the type being constructed.
Hmm, not sure I totally follow this. Can you give an example of something that should be covered in this PR but isn't? :-)
Member
There was a problem hiding this comment.
Sorry, wasn't requesting any changes here with this comment, just leaving me/others a breadcrumb for a future improvement
dcreager
added a commit
that referenced
this pull request
Jun 27, 2025
AlexWaygood
added a commit
that referenced
this pull request
Jun 27, 2025
AlexWaygood
added a commit
that referenced
this pull request
Jun 27, 2025
This was referenced Jul 21, 2025
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.
Summary
Ensure that we correctly infer calls such as
tuple((1, 2)),tuple(range(42)), etc. Ensure that we emit errors on invalid calls such astuple[int, str]().Test Plan
Mdtests