Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix annotated with function as type keyword list parameter#20094
Uh oh!
There was an error while loading.Please reload this page.
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 totest-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.
A5rocks left a comment
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!
| returnexpr_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 throughlookup_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 thatlookup_qualified is not being passed on in the recursions.
This test passes.
[casetestAnnotatedWithCallableAsParameterTypeAliasDeeper]fromtyping_extensionsimportAnnotated,TypeAliasdefsomething()->None: ...A:TypeAlias=list[Annotated[Annotated[str,something()],something()]]a:Areveal_type(a)# N: Revealed type is "builtins.list[builtins.str]"[builtinsfixtures/tuple.pyi]
This one does not (even after changes currently in this PR).
[casetestAnnotatedWithCallableAsParameterTypeKeywordDeeper]fromtyping_extensionsimportAnnotateddefsomething()->None: ...typeA=list[Annotated[Annotated[str,something()],something()]]a:Areveal_type(a)# N: Revealed type is "builtins.list[builtins.str]"[builtinsfixtures/tuple.pyi]
Can make the second one pass by pushinglookup_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 passlookup_qualified through.
I suspect:
- the recursive calls were added before the
lookup_qualified - the
lookup_qualifiedPR was part of a larger PR so this got missed
Unfortunately I'm on mobile so blame UI kinda sucks :(
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.
PR that addedlookup_qualified doesn't address passing through the argument.
But as you said, the recursion was there already. An oversight sound plausible to me.
@A5rocks , should I add the passing through to this PR?
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.
Yes, please do.@JukkaL
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.
Yeah this looks like an oversight.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sterliakov left a comment
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.
Yes,lookup_qualified should almost certainly be passed everywhere, looks like a simple omission.
According tomypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
8e2ce96 intopython:masterUh oh!
There was an error while loading.Please reload this page.
Fixes#20090