-
-
Couldn't load subscription status.
- 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?
Conversation
|
|
||
|
|
||
| [case testDeprecatedClassInitMethod] | ||
| [case testDeprecatedClassConstructor] |
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 changed this test to check implicit calls to class constructors.
The previous test didn't seem to directly check for __init__ methods; instead, the error is reported for any usage of C (including a plain expression statement, C on its own line), and did not require accessing __init__ (or any other attribute):
from typing_extensions import deprecated
@deprecated("Warning")
class C: ...
C # E: class __main__.C is deprecated: Warning
This comment has been minimized.
This comment has been minimized.
mypy/checkexpr.py
Outdated
| # item so deprecation checks are not duplicated. | ||
| if isinstance(callable_node, RefExpr) and isinstance(callable_node.node, TypeInfo): | ||
| self.chk.check_deprecated(callable_node.node.get_method("__new__"), context) | ||
| self.chk.check_deprecated(callable_node.node.get_method("__init__"), context) |
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?
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 @deprecated activates, and I gathered the following:
- If a function/class is
@deprecated, anyRefExpr(regardless whether it is further used in aCallExpr) triggers a report (seevisit_name_exprandvisit_member_expr); - If a function/class
CallExprhas overload implementations, then reports are triggered during overload resolution (this includes overloaded class constructors). - If it's in a type annotation, it's done during semantic analysis.
IMO @deprecated class 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_call because it kind of mirrors where the same check is done for overloads (check_overload_call)
Lines 2770 to 2774 in 11dbe33
| self.chk.warn_deprecated(c.definition, context) | |
| return unioned_result | |
| if inferred_result is not None: | |
| if isinstance(c := get_proper_type(inferred_result[1]), CallableType): | |
| self.chk.warn_deprecated(c.definition, context) |
Another place could be visit_call_expr_inner
Line 507 in 11dbe33
| def visit_call_expr_inner(self, e: CallExpr, allow_none_return: bool = False) -> Type: |
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:
from typing_extensions import deprecated
class A:
@deprecated("don't add As")
def __add__(self, o: object) -> int:
return 5
a = A()
a + a # warning here
... 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 :^)
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).
I assume there's some lookup on the callable node to get e.g.
__add__or__init__?
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 then the member access utility should mark a callable as deprecated if it's from the
__init__if the__new__is deprecated (?).
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_expr results in
from typing_extensions import deprecated
class A:
@deprecated("")
def __init__(self) -> None: ...
A # E: ... [deprecated]This is because mypy uses CallableType to represent a class A in a lot of expression contexts without the user having any intention of actually making the call A(), but this CallableType is synthesised by <Class>.__init__ or <Class>.__new__. So an expression A automatically triggers type checking paths for A.__init__ or A.__new__ because of mypy implementation details, then further triggers a deprecation report, even if the user only intended to mean A and not A().
This is unlike __add__, because only a + a implies access to .__add__ (not a itself).
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!
mypy/checkexpr.py
Outdated
| # item so deprecation checks are not duplicated. | ||
| if isinstance(callable_node, RefExpr) and isinstance(callable_node.node, TypeInfo): | ||
| self.chk.check_deprecated(callable_node.node.get_method("__new__"), context) | ||
| self.chk.check_deprecated(callable_node.node.get_method("__init__"), context) |
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!
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.
While you're at this, side question: how difficult would it be to flag transitive calls where a class is passed as Callable[..., T]? Something like
from typing import Callable, TypeVar
from typing_extensions import ParamSPec, deprecated
T = TypeVar("T")
P = ParamSpec("P")
# operator.call
def call(fn: Callable[P, T], *args: P.args, **kwargs: P.kwargs) -> T: ...
class A:
@deprecated("...")
def __init__(self) -> None: ...
call(A)I'm sure we do that for functions, so maybe constructors can be trivially included as well? (or does this PR already handle this case?)
We don't actually do this for functions. What really happens is that any reference to a # mypy: disable-error-code=empty-body
from typing import Callable, TypeVar
from typing_extensions import ParamSpec, deprecated
T = TypeVar("T")
P = ParamSpec("P")
# operator.call
def call(fn: Callable[P, T], *args: P.args, **kwargs: P.kwargs) -> T: ...
@deprecated("...")
def f(a: int, b: str) -> None: ...
f # E: [deprecated]
call(
f, # E: [deprecated]
1,
""
)It's not the I originally thought of another case while implementing this, which is whether to report calls like |
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.
The idea of marking only __init__ or __new__ as deprecated did not occur to me when I started working on this. I think it is clearly a valuable extension to the current behaviour. Thanks!
| C() # E: class __main__.C is deprecated: use C2 instead | ||
| C.__init__(c) # E: class __main__.C is deprecated: use C2 instead | ||
| C() # E: function __main__.C.__init__ is deprecated: call `make_c()` 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 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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Huh, thanks! I did not realize it's that simple...
If something accepts a
And this makes sense. Maybe at least literal |
|
Sorry, I really didn't mean to suggest doing it right here and right now - IMO this PR is good to go as-is, checking against Callable will better fit as a follow-up improvement unless it's really a one-line addition, but that doesn't seem to be the case:) |
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@sterliakov Thank you for the suggestion for supporting transitive callables, I'm +0.5 on this not being interpreted as a false positive, and you're also right that it won't be a one-liner so I won't add this to the PR. While prototyping transitive callables I realised that old-style type aliases with deprecated class constructors weren't being reported. I've summarised the changes in the top comment. |
| # separately during overload resolution. `callable_node` is `None` for an overload | ||
| # item so deprecation checks are not duplicated. | ||
| callable_info: TypeInfo | None = None | ||
| if isinstance(callable_node.node, TypeInfo): |
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.
Since you don't actually use callable_info later, this could make a great refers_to_class_callable(node) -> bool helper?
|
|
||
| @deprecated("use C2 instead") | ||
| class C: | ||
| @deprecated("call `make_c()` 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.
Could you add a similar test (if there isn't one) in incremental mode? .definition is a little magic (it isn't serialized in cache and is restored in fixup.py instead) - that should not be problematic here, but better have it than discover yet another cache trouble a week later
| class A: | ||
| @deprecated("call `self.initialise()` instead") | ||
| def __init__(self) -> None: ... | ||
| def initialise(self) -> 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.
Could you also check self.__init__() inside some A (should be flagged) and B (should not be flagged) methods? And maybe also standalone
a = A()
a.__init__()
|
Also, what happens if you do the following? @deprecated("xxx")
class A:
@deprecated("yyy")
def __init__(self) -> None: ...
A()I don't see any reason to actually have them both deprecated, just wonder what the output will be - both deprecation warnings stacked on the same line? |
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 seem to be some more cases that need consideration. You could extend your logic further, of course, but maybe you could perform the deprecation check at a more convenient place. However, you mentioned above that you already took different places into account, and during a quick look at the code, nothing obvious came to my mind, too, so I am not sure this is possible.
| if isinstance(alias_target, Instance) and isinstance(alias_target.type, TypeInfo): | ||
| callable_info = alias_target.type | ||
| if callable_info is not None: | ||
| self.chk.check_deprecated(callee.definition, context) |
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.
Calling warn_deprecated should be sufficient here, since __init__ and __new__ are methods, not independently importable functions.
| A_alias | ||
| A_explicit_alias | ||
| B_alias | ||
| B_explicit_alias |
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 are more cases should be checked. For example, I think it is likely that this test extension would fail at the moment:
class C: ...
def f(ab: Type[Union[A, C]]) -> Union[A, C]:
return ab()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.
From this discussion, I don't think we should warn deprecation from type[ClassWithDeprecatedConstructor] because type[...] may be a join of subclasses which have non-deprecated constructors with the same signature as their superclass.
Fixes #20103
Enables reporting
@deprecated()when non-overloaded class constructors (__init__,__new__) are implicitly invoked (Class()).Previously, to get deprecation warnings when constructing a class,
@deprecated()required to be decorated on the class. However, this requirement also means any usage of the class (in annotations or subclassing) also reported deprecations, which is not always desirable.Now,
@deprecated()can be used to report specifically class construction (rather than all usages of a class). This allows reporting to the user that alternative factory functions or methods exist for building instances.Overloaded class constructors already show deprecation reports (fixed in #19588) and is not addressed here.
Last review was for 53622b1 and since then I've updated the PR with the following:
Only use the constructor which mypy chose from
__init__and__new__(8f08c60). This causes the following false negative:This saves 2 MRO walks as well as keeping type-check reporting consistent (mypy chooses the lowest-indexed item in the MRO out of
__new__and__init__for the signature and subsequent error reports, and currently doesn't warn if__new__and__init__don't match (playground)). A more principled approach would be to build a union of the first__new__and__init__found in the MRO as well as the metaclass's__call__, but that applies generally to constructor signature type-checking and beyond the scope of this PR.Allow deprecation reporting when old-style type aliases (so assignments
A = MyClassorA: TypeAlias = MyClass) are used directly as class constructors (b0ffd6b).