[ty] Support type annotation for legacy typing aliases for generic classes #18404
Conversation
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
lipefree
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Member
|
Thanks! Could you resolve the merge conflicts? It's caused by #18350 -- you'll just need to change |
Contributor
Author
|
I did not see it, I will do it now ! |
Contributor
|
dcreager
reviewed
Jun 2, 2025
Member
dcreager
left a comment
dcreager
left a comment
There was a problem hiding this comment.
This looks great! Just one final recommendation related to the suggestion to use to_specialized_instance's diagnostics for now, but that we'd want more custom diagnostics as follow-on work
| reveal_type(list_bare) # revealed: list[Unknown] | ||
| # TODO: revealed: list[int] | ||
| reveal_type(list_parametrized) # revealed: list[Unknown] | ||
| reveal_type(list_parametrized) # revealed: list[int] |
Member
There was a problem hiding this comment.
Can you add an example of each of these aliases being specialized with the wrong number of arguments?
Contributor
Author
There was a problem hiding this comment.
Yes, I will add them !
| self.infer_type_expression(arguments_slice); | ||
| KnownClass::Dict.to_instance(db) | ||
| } | ||
| SpecialFormType::ChainMap => match arguments_slice { |
Member
There was a problem hiding this comment.
And here add a TODO comment that we want to add more specific diagnostics here for arity mismatches.
(And it doesn't need to be included in the comment text, but there's a fair bit of repetition in all of these match arms. So when we do tackle this follow-on work and add diagnostics, I could see it being enough at that point to be worth factoring out into a helper method.)
AlexWaygood
reviewed
Jun 2, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thank you! This looks like it's on the right track to me
Comment on lines
8894
to
8905
| SpecialFormType::Dict => match arguments_slice { | ||
| ast::Expr::Tuple(t) => { | ||
| let args_ty = t.elts.iter().map(|elt| self.infer_type_expression(elt)); | ||
| let ty = KnownClass::Dict.to_specialized_instance(db, args_ty); | ||
| self.store_expression_type(arguments_slice, ty); | ||
| ty | ||
| } | ||
| _ => { | ||
| self.infer_type_expression(arguments_slice); | ||
| KnownClass::Dict.to_instance(db) | ||
| } | ||
| }, |
Member
There was a problem hiding this comment.
this gets us the right answer if we have the right number of type arguments provided. Unfortunately, however, it doesn't cause us to emit a diagnostic if the wrong number of type arguments is provided. If one or three arguments are provided to Dict (Dict[int] or Dict[str, bytes, bool]), we need to emit an error telling the user that they made a mistake.
There's the same issue for all of these branches, but each branch expects a different number of arguments, so I suggest you do something like this:
diff --git a/crates/ty_python_semantic/src/types/diagnostic.rs b/crates/ty_python_semantic/src/types/diagnostic.rs
index e931013755..85d8ab92e9 100644
--- a/crates/ty_python_semantic/src/types/diagnostic.rs
+++ b/crates/ty_python_semantic/src/types/diagnostic.rs
@@ -1849,6 +1849,21 @@ pub(crate) fn report_invalid_arguments_to_annotated(
));
}
+pub(crate) fn report_invalid_arguments_to_legacy_alias(
+ context: &InferContext,
+ subscript: &ast::ExprSubscript,
+ alias: SpecialFormType,
+ expected_number: usize,
+ actual_number: usize,
+) {
+ let Some(builder) = context.report_lint(&INVALID_TYPE_FORM, subscript) else {
+ return;
+ };
+ builder.into_diagnostic(format_args!(
+ "Legacy alias `{alias}` expected exactly {expected_number} arguments, got {actual_number}"
+ ));
+}
+
pub(crate) fn report_bad_argument_to_get_protocol_members(
context: &InferContext,
call: &ast::ExprCall,
diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs
index 12e455c9bf..4f24714fda 100644
--- a/crates/ty_python_semantic/src/types/infer.rs
+++ b/crates/ty_python_semantic/src/types/infer.rs
@@ -104,7 +104,8 @@ use super::diagnostic::{
INVALID_METACLASS, INVALID_OVERLOAD, INVALID_PROTOCOL, REDUNDANT_CAST, STATIC_ASSERT_ERROR,
SUBCLASS_OF_FINAL_CLASS, TYPE_ASSERTION_FAILURE, report_attempted_protocol_instantiation,
report_bad_argument_to_get_protocol_members, report_duplicate_bases,
- report_index_out_of_bounds, report_invalid_exception_caught, report_invalid_exception_cause,
+ report_index_out_of_bounds, report_invalid_arguments_to_legacy_alias,
+ report_invalid_exception_caught, report_invalid_exception_cause,
report_invalid_exception_raised, report_invalid_or_unsupported_base,
report_invalid_type_checking_constant, report_non_subscriptable,
report_possibly_unresolved_reference,
@@ -8869,6 +8870,15 @@ impl<'db> TypeInferenceBuilder<'db> {
SpecialFormType::ChainMap => match arguments_slice {
ast::Expr::Tuple(t) => {
+ if t.len() != 2 {
+ report_invalid_arguments_to_legacy_alias(
+ &self.context,
+ subscript,
+ SpecialFormType::ChainMap,
+ 2,
+ t.len(),
+ );
+ }
let args_ty = t.elts.iter().map(|elt| self.infer_type_expression(elt));
let ty = KnownClass::ChainMap.to_specialized_instance(db, args_ty);
self.store_expression_type(arguments_slice, ty);
@@ -8876,6 +8886,13 @@ impl<'db> TypeInferenceBuilder<'db> {
}
_ => {
self.infer_type_expression(arguments_slice);
+ report_invalid_arguments_to_legacy_alias(
+ &self.context,
+ subscript,
+ SpecialFormType::ChainMap,
+ 2,
+ 1,
+ );
KnownClass::ChainMap.to_instance(db)
}
},
Member
There was a problem hiding this comment.
oops, I see I posted this simultaneously to @dcreager's review comments! I'm okay with just adding a TODO regarding the diagnostics, as he suggests, instead of adding them now
Member
There was a problem hiding this comment.
this gets us the right answer if we have the right number of type arguments provided. Unfortunately, however, it doesn't cause us to emit a diagnostic if the wrong number of type arguments is provided. If one or three arguments are provided to
Dict(Dict[int]orDict[str, bytes, bool]), we need to emit an error telling the user that they made a mistake.
I think we are emitting something, it's just that we're relying on a diagnostic coming from to_specialized_instance, which actually produces an ERROR-level log message instead of a proper diagnostic. (The intent was that this is more likely the result of a custom typeshed installing e.g. a list that is not parameterized with one typevar, and so we only wanted to output the message once instead of every single place that list appears.)
Also, @lipefree did this at my suggestion (astral-sh/ty#548 (comment)). I agree that we need to address this but had suggested it as a follow-on — but if you feel that it should be folded into this PR I'm 👍 to that too.
Member
There was a problem hiding this comment.
Nope, I'm definitely okay with leaving diagnostics to a followup, as long as we keep track of the TODO!
Contributor
Author
There was a problem hiding this comment.
If the snippet @AlexWaygood provided above is enough (checking arity and provide the new diagnosis report_invalid_arguments_to_legacy_alias), then I am more than happy to cover the case of arity mismatch in this PR aswell and adapt my tests. I feel like having a TODO when I could revolve the task now just adds more review time for everyone.
Contributor
Author
|
I feel like factoring in a helper function as @dcreager suggested may already be useful but I don't know where to define it |
AlexWaygood
approved these changes
Jun 3, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks @lipefree!!
This was a bit trickier than I realised when I wroteup the issue. I ended up refactoring your logic a little bit to make it a bit less repetitive. But you had the basic idea exactly right!
Contributor
Author
|
Thank you for helping me closing this issue @AlexWaygood ! |
dcreager
reviewed
Jun 3, 2025
Comment on lines
+8664
to
+8666
| (Either::Left(t), t.len()) | ||
| } else { | ||
| (Either::Right([arguments]), 1) |
Member
There was a problem hiding this comment.
nit: I think you could also have done this with std::slice::from_ref for the non-tuple case, so that in both cases you would end up with a slice iterator. Then you wouldn't need Either. (That would be a probably-not-actually-noticeable performance improvement, since iterating over an Either requires a conditional check on whether it's Left or Right at each step.)
dcreager
added a commit
that referenced
this pull request
Jun 3, 2025
…aration * origin/main: [ty] Infer `list[T]` when unpacking non-tuple type (#18438) [ty] Meta-type of type variables should be type[..] (#18439) [`pyupgrade`] Make fix unsafe if it deletes comments (`UP050`) (#18390) [`pyupgrade`] Make fix unsafe if it deletes comments (`UP004`) (#18393) [ty] Support using legacy typing aliases for generic classes in type annotations (#18404) Use ty's completions in playground (#18425) Update editor setup docs about Neovim and Vim (#18324) Update NPM Development dependencies (#18423) Infer `list[T]` for starred target in unpacking (#18401) [`refurb`] Mark `FURB180` fix unsafe when class has bases (#18149) [`fastapi`] Avoid false positive for class dependencies (`FAST003`) (#18271)
dhruvmanila
reviewed
Jun 3, 2025
carljm
added a commit
to mtshiba/ruff
that referenced
this pull request
Jun 4, 2025
* main: [ty] Add tests for empty list/tuple unpacking (astral-sh#18451) [ty] Argument type expansion for overload call evaluation (astral-sh#18382) [ty] Minor cleanup for `site-packages` discovery logic (astral-sh#18446) [ty] Add generic inference for dataclasses (astral-sh#18443) [ty] dataclasses: Allow using dataclasses.dataclass as a function. (astral-sh#18440) [ty] Create separate `FunctionLiteral` and `FunctionType` types (astral-sh#18360) [ty] Infer `list[T]` when unpacking non-tuple type (astral-sh#18438) [ty] Meta-type of type variables should be type[..] (astral-sh#18439) [`pyupgrade`] Make fix unsafe if it deletes comments (`UP050`) (astral-sh#18390) [`pyupgrade`] Make fix unsafe if it deletes comments (`UP004`) (astral-sh#18393) [ty] Support using legacy typing aliases for generic classes in type annotations (astral-sh#18404) Use ty's completions in playground (astral-sh#18425) Update editor setup docs about Neovim and Vim (astral-sh#18324) Update NPM Development dependencies (astral-sh#18423)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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
Fixes astral-sh/ty#548.
This PR implements type annotations for legacy typing aliases for generic classes.
Namely:
typing.List[T]typing.Dict[T, U]typing.Set[T]typing.FrozenSet[T]typing.ChainMap[T, U]typing.Counter[T]typing.DefaultDict[T, U]typing.Deque[T]typing.OrderedDict[T, U]Here is an example for
Dict:Thank you @dcreager and @carljm for helping me out!
Test Plan
I modified existing mdtest. I can add more cases if necessary!