Skip to content
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.

Sign up for GitHub

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

Support @deprecated() on overloaded __get__ for generic descriptors #18333

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support @deprecated() on overloaded __get__ for generic descriptors #18333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7714,7 +7714,12 @@ def warn_deprecated(self, node: Node | None, context: Context) -> None:
warn(deprecated, context, code=codes.DEPRECATED)

def warn_deprecated_overload_item(
self, node: Node | None, context: Context, *, target: Type, selftype: Type | None = None
self,
node: Node | None,
context: Context,
*,
target: Type,
selftype: Instance | None = None,
) -> None:
"""Warn if the overload item corresponding to the given callable is deprecated."""
target = get_proper_type(target)
Expand All @@ -7724,7 +7729,9 @@ def warn_deprecated_overload_item(
candidate := item.func.type, CallableType
):
if selftype is not None:
candidate = bind_self(candidate, selftype)
candidate = bind_self(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised about this bind_self, actually. I cannot find where inferred_dunder_get_type gets passed to bind_self... I'm only looking at the code though. I'm certain it's used but... where?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, in analyze_descriptor_access, we pass inferred_dunder_get_type as target:

mypy/mypy/checkmember.py

Lines 726 to 728 in 9ff9946

mx.chk.warn_deprecated_overload_item(
dunder_get, mx.context, target=inferred_dunder_get_type, selftype=descriptor_type
)

but not sure what you mean exactly

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I'd expect some sort of symmetry: we repeat the actions we did to get target on each overload member. But I can't find where we bind_self.

To be specific, in analyze_descriptor_access (or in a function it calls) I expect a bind_self call as dunder_get turns into inferred_dunder_get_type... but I can't find one. Is it just not necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

analyze_descriptor_access calls analyze_decorator_or_funcbase_access (eventually) calls bind_self. Is that what you are looking for?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Does that mean bind_self and expand_type_by_instance should be swapped (IE pass the result of bind_self to expand_type_by_instance) in this PR?

I'm not sure if there's a difference anyways but I think it's better to imitate what's elsewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without spending much time, I can also only speculate if these different orders can make a difference. But I guess you are right. Synchronising both steps as well as possible should do no harm and might be beneficial in some cases. (At least, it frees the next one looking at the code from asking himself the same question.)

bind_self gets descriptor_type but expand_type_by_instance gets typ. Same question here?

expand_type_by_instance(candidate, selftype), selftype
)
if candidate == target:
self.warn_deprecated(item.func, context)

Expand Down
16 changes: 15 additions & 1 deletion test-data/unit/check-deprecated.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,9 +506,11 @@ C().g = "x" # E: function __main__.C.g is deprecated: use g2 instead \
[case testDeprecatedDescriptor]
# flags: --enable-error-code=deprecated

from typing import Any, Optional, Union
from typing import Any, Generic, Optional, TypeVar, Union
from typing_extensions import deprecated, overload

T = TypeVar("T")

@deprecated("use E1 instead")
class D1:
def __get__(self, obj: Optional[C], objtype: Any) -> Union[D1, int]: ...
Expand Down Expand Up @@ -536,10 +538,19 @@ class D3:
def __set__(self, obj: C, value: str) -> None: ...
def __set__(self, obj: C, value: Union[int, str]) -> None: ...

class D4(Generic[T]):
@overload
def __get__(self, obj: None, objtype: Any) -> T: ...
@overload
@deprecated("deprecated instance access")
def __get__(self, obj: C, objtype: Any) -> T: ...
def __get__(self, obj: Optional[C], objtype: Any) -> T: ...

class C:
d1 = D1() # E: class __main__.D1 is deprecated: use E1 instead
d2 = D2()
d3 = D3()
d4 = D4[int]()

c: C
C.d1
Expand All @@ -554,6 +565,9 @@ C.d3 # E: overload def (self: __main__.D3, obj: None, objtype: Any) -> __main__
c.d3 # E: overload def (self: __main__.D3, obj: __main__.C, objtype: Any) -> builtins.int of function __main__.D3.__get__ is deprecated: use E3.__get__ instead
c.d3 = 1
c.d3 = "x" # E: overload def (self: __main__.D3, obj: __main__.C, value: builtins.str) of function __main__.D3.__set__ is deprecated: use E3.__set__ instead

C.d4
c.d4 # E: overload def (self: __main__.D4[T`1], obj: __main__.C, objtype: Any) -> T`1 of function __main__.D4.__get__ is deprecated: deprecated instance access
[builtins fixtures/property.pyi]


Expand Down
Loading