-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix annotated with function as type keyword list parameter #20094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix annotated with function as type keyword list parameter #20094
Conversation
This comment has been minimized.
This comment has been minimized.
|
||
def something() -> None: ... | ||
|
||
type A = list[Annotated[str, something()]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this test to test-data/unit/check-python312.test
: --python-version
does not override syntax errors (so won't help here), and that file only runs on 3.12+
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code change makes sense to me though it's a bit surprising that it fixes a bug!
return expr_to_unanalyzed_type(args[0], options, allow_new_syntax, expr) | ||
base.args = tuple( | ||
expr_to_unanalyzed_type(arg, options, allow_new_syntax, expr, allow_unpack=True) | ||
expr_to_unanalyzed_type( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's other recursion happening here, do you think it's a good idea to preemptively thread through lookup_qualified
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to pretend I have a good understanding of the consequences. But I can trigger what is basically the same behaviour by nesting annotations. And to me it seems that the cause is indeed that lookup_qualified
is not being passed on in the recursions.
This test passes.
[case testAnnotatedWithCallableAsParameterTypeAliasDeeper]
from typing_extensions import Annotated, TypeAlias
def something() -> None: ...
A: TypeAlias = list[Annotated[Annotated[str, something()], something()]]
a: A
reveal_type(a) # N: Revealed type is "builtins.list[builtins.str]"
[builtins fixtures/tuple.pyi]
This one does not (even after changes currently in this PR).
[case testAnnotatedWithCallableAsParameterTypeKeywordDeeper]
from typing_extensions import Annotated
def something() -> None: ...
type A = list[Annotated[Annotated[str, something()], something()]]
a: A
reveal_type(a) # N: Revealed type is "builtins.list[builtins.str]"
[builtins fixtures/tuple.pyi]
Can make the second one pass by pushing lookup_qualified
through to another recursive call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe check blame to quickly look at the PR that introduced things (maybe it addresses this), but it sounds like it would be good to pass lookup_qualified
through.
I suspect:
- the recursive calls were added before the
lookup_qualified
- the
lookup_qualified
PR was part of a larger PR so this got missed
Unfortunately I'm on mobile so blame UI kinda sucks :(
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Fixes #20090