-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: report @deprecated() on non-overloaded class constructors
#20104
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -315,18 +315,57 @@ class E: ... | |
| [builtins fixtures/tuple.pyi] | ||
|
|
||
|
|
||
| [case testDeprecatedClassInitMethod] | ||
| [case testDeprecatedClassConstructor] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this test to check implicit calls to class constructors. The previous test didn't seem to directly check for from typing_extensions import deprecated
@deprecated("Warning")
class C: ...
C # E: class __main__.C is deprecated: Warning |
||
| # flags: --enable-error-code=deprecated | ||
|
|
||
| from typing_extensions import deprecated | ||
|
|
||
| @deprecated("use C2 instead") | ||
| class C: | ||
| @deprecated("call `make_c()` instead") | ||
| def __init__(self) -> None: ... | ||
| @classmethod | ||
| def make_c(cls) -> C: ... | ||
|
|
||
| c: C # E: class __main__.C is deprecated: use C2 instead | ||
| C() # E: class __main__.C is deprecated: use C2 instead | ||
| C.__init__(c) # E: class __main__.C is deprecated: use C2 instead | ||
| class C2(C): ... | ||
|
|
||
| C() # E: function __main__.C.__init__ is deprecated: call `make_c()` instead | ||
| C2() # E: function __main__.C.__init__ is deprecated: call `make_c()` instead | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you want to add a test line this? (here and similar below) class CC(C): ...
CC() # E: function __main__.C.__init__ is deprecated: call `make_c()` instead |
||
| class D: | ||
| @deprecated("call `make_d()` instead") | ||
| def __new__(cls) -> D: ... | ||
| @classmethod | ||
| def make_d(cls) -> D: ... | ||
|
|
||
| class D2(D): ... | ||
|
|
||
| D() # E: function __main__.D.__new__ is deprecated: call `make_d()` instead | ||
| D2() # E: function __main__.D.__new__ is deprecated: call `make_d()` instead | ||
|
|
||
| [builtins fixtures/tuple.pyi] | ||
|
|
||
|
|
||
| [case testDeprecatedSuperClassConstructor] | ||
| # flags: --enable-error-code=deprecated | ||
|
|
||
| from typing_extensions import deprecated, Self | ||
|
|
||
| class A: | ||
| @deprecated("call `self.initialise()` instead") | ||
| def __init__(self) -> None: ... | ||
| def initialise(self) -> None: ... | ||
|
|
||
| class B(A): | ||
| def __init__(self) -> None: | ||
| super().__init__() # E: function __main__.A.__init__ is deprecated: call `self.initialise()` instead | ||
|
|
||
| class C: | ||
| @deprecated("call `object.__new__(cls)` instead") | ||
| def __new__(cls) -> Self: ... | ||
|
|
||
| class D(C): | ||
| def __new__(cls) -> Self: | ||
| return super().__new__(cls) # E: function __main__.C.__new__ is deprecated: call `object.__new__(cls)` instead | ||
|
|
||
| [builtins fixtures/tuple.pyi] | ||
|
|
||
|
|
||
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 feels like the wrong place for this. But I don't remember where would be better. Where is overloaded
__init__checked for deprecation?Uh oh!
There was an error while loading. Please reload this page.
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.
Overloaded
__init__doesn't have a special place - it is checked with all other overloaded function/method calls.I had a look through the situations where
@deprecatedactivates, and I gathered the following:@deprecated, anyRefExpr(regardless whether it is further used in aCallExpr) triggers a report (seevisit_name_exprandvisit_member_expr);CallExprhas overload implementations, then reports are triggered during overload resolution (this includes overloaded class constructors).IMO
@deprecatedclass constructors aren't similar to any of the 3 situations above, so this implementation can't be placed adjacent to where any of the 3 situations above activates.I placed it in
check_callable_callbecause it kind of mirrors where the same check is done for overloads (check_overload_call)mypy/mypy/checkexpr.py
Lines 2770 to 2774 in 11dbe33
Another place could be
visit_call_expr_innermypy/mypy/checkexpr.py
Line 507 in 11dbe33
but any other suggestions are welcome.
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 would have wanted it to be in the same place as the thing that warns in this case:
... but there is no warning!
I assume there's some lookup on the callable node to get e.g.
__add__or__init__? IMO the deprecated check should go after that. And then the member access utility should mark a callable as deprecated if it's from the__init__if the__new__is deprecated (?).I haven't tried to implement this so maybe this is completely off base and what you have is correct :^)
Uh oh!
There was an error while loading. Please reload this page.
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.
For
@deprecated()to activate you have to turn it on somewhere on a configuration, your example does show a warning (see mypy Playground).Thank you, I'll take another look - yes, implicit dunder activation might be a more natural place to put this. (Not
__init__though, that never got resolved by itself before this PR; it only got resolved as part of an overload without special casing).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.
So, I think this can only be implemented in a place which specifically deals with call expressions.
I had a look and doing it this way
and I believe we will end up with a lot of false positives. A dummy implementation by placing it at the end of this range in
ExpressionChecker.analyze_ref_exprresults inThis is because mypy uses
CallableTypeto represent a classAin a lot of expression contexts without the user having any intention of actually making the callA(), but thisCallableTypeis synthesised by<Class>.__init__or<Class>.__new__. So an expressionAautomatically triggers type checking paths forA.__init__orA.__new__because of mypy implementation details, then further triggers a deprecation report, even if the user only intended to meanAand notA().This is unlike
__add__, because onlya + aimplies access to.__add__(notaitself).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 for digging into this!