-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
PEP 702 (@deprecated): improve the handling of explicit type annotations of assignment statements #17899
PEP 702 (@deprecated): improve the handling of explicit type annotations of assignment statements #17899
Conversation
* Analyse the explicit type annotations of "normal" assignment statements (that have an rvalue). * Dive into nested type annotations of assignment statements.
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Only a single new note is reported by the primer, but at least it is correct. @ilevkivskyi: A part of this PR addresses this remark you made. |
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.
Thanks! Here are couple comments.
mypy/checker.py
Outdated
self.check_deprecated(typ.type, s) | ||
for arg in typ.args: | ||
self.search_deprecated(arg, s, visited) | ||
elif isinstance(typ, (UnionType, TupleType)): |
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 is not a robust way to do it. You should write a simple visitor instead.
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.
I replaced search_deprecated
with InstanceDeprecatedVisitor
, which additionally considers CallableType
and TypeAliasType
. Do you see other relevant cases?
x5: Tuple[Tuple[D, C], E] # N: class __main__.C is deprecated: use C2 instead | ||
x6: List[C] # N: class __main__.C is deprecated: use C2 instead | ||
x7: List[List[C]] # N: class __main__.C is deprecated: use C2 instead | ||
x8: List[Optional[Tuple[Union[List[C], int]]]] # N: class __main__.C is deprecated: use C2 instead |
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 add a test case involving type aliases (including generic and/or recursive ones).
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.
Done.
…`CallableType` and `TypeAliasType`)
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mypy/checker.py
Outdated
@@ -287,6 +290,96 @@ class PartialTypeScope(NamedTuple): | |||
is_local: bool | |||
|
|||
|
|||
class InstanceDeprecatedVisitor(TypeVisitor[None]): |
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.
Sorry, this is not what I meant, you should inherit TypeTraverserVisitor
instead, and override only visit_instance()
and maybe visit_type_alias_type()
(the latter is only needed if you want to show an error at each use place, instead of only at the type alias definition r.h.s.)
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.
No problem. TypeTraverserVisitor
is the more convenient base class, of course. Now I know it exists.
I also changed the name visited
to seen_aliases
(in agreement with TypeArgumentAnalyzer
).
I was unsure whether emitting deprecation notes due to using type alias definitions is a good idea. I had a slight tendency towards it, so I implemented it this way for now. Maybe @JelleZijlstra has a better thought-out opinion on this?
…d of `TypeVisitor`.
for more information, see https://pre-commit.ci
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 is almost ready, I have couple more comments.
I was unsure whether emitting deprecation notes due to using type alias definitions is a good idea.
It looks like PEP is intentionally vague on the details. If you want to know my opinion, I would say we should only warn once on the type alias definition.
I tried to make it concrete, but likely missed some cases. In general, I'd want people to only get one deprecation notice per deprecated thing they use. I think what you're talking about here is an alias like |
Would you say the same for cases where import lib
x: lib.Alias # no deprecation warning? Then, the author of A highly exceptional case, of course... |
I would. PEP 702 is limited in the kinds of things it lets you mark as deprecated, and type aliases aren't on the list. We'd need a new PEP with new syntax to mark type aliases as deprecated. |
Okay, I did as you suggested. Thanks for your sharing your opinion, @JelleZijlstra, and doing the review, @ilevkivskyi! |
Diff from mypy_primer, showing the effect of this PR on open source code: anyio (https://github.com/agronholm/anyio)
+ src/anyio/_backends/_asyncio.py:1063: note: class asyncio.unix_events.AbstractChildWatcher is deprecated: Deprecated as of Python 3.12; will be removed in Python 3.14 [deprecated]
|
@tyralla After some more thinking it seems to me moving this visitor to earlier stages of analysis (i.e. to typeanal.py) will allow uniformly using it in all positions a type can appear. Otherwise you will chase a lot possible cases, like function annotations, type aliases targets, cast targets, type variable bounds etc. |
Anyway, since I already merged this one, you can do it in a follow-up PR. |
Thanks for the tip! I will try it. |
Two improvements of the current PEP 702 implementation (deprecated):
(I intend to continue working on PEP 702 and would prefer to do so in small steps if this is okay for the maintainers/reviewers.)