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.

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): "normal" overloaded methods #18477

Merged
merged 12 commits into from
Jan 28, 2025

Conversation

tyralla
Copy link
Collaborator

@tyralla tyralla commented Jan 15, 2025

Fixes #18474

It seems I covered overloaded functions, descriptors, and special methods so far but completely forgot about "normal" methods (thanks to @sobolevn for pointing this out). This addition should do the trick.

tyralla and others added 2 commits January 15, 2025 21:38
Fixes python#18474

It seems I covered overloaded functions, descriptors, and special methods so far but completely forgot about "normal" methods (thanks to @sobolevn for pointing this out).  This addition should do the trick.
@cdce8p cdce8p added topic-pep-702 PEP 702, @deprecated topic-overloads labels Jan 15, 2025

This comment has been minimized.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! What about @staticmethod and @classmethod?

Comment on lines 1490 to 1493
(node is None)
and (member is not None)
and isinstance(object_type, Instance)
and ((symbol := object_type.type.names.get(member)) is not None)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(node is None)
and (member is not None)
and isinstance(object_type, Instance)
and ((symbol := object_type.type.names.get(member)) is not None)
node is None
and member is not None
and isinstance(object_type, Instance)
and (symbol := object_type.type.names.get(member)) is not None

Copy link
Collaborator Author

@tyralla tyralla Jan 16, 2025

Choose a reason for hiding this comment

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

I will have a look at it later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It became a one-liner followed by a loop.

@tyralla
Copy link
Collaborator Author

tyralla commented Jan 16, 2025

Inheritance is also an issue: If the (overloaded) function is defined by a base class of object_type, object_type.type.names.get(member) cannot work.

The base class is not directly available. Going through the MRO might be necessary (at least for static methods).

…ods' into narrowing/overloaded_normal_methods

# Conflicts:
#	mypy/checkexpr.py
@tyralla
Copy link
Collaborator Author

tyralla commented Jan 17, 2025

@sobolevn: classmethod already worked, staticmethod not (needed to skip bind_self). Inheritance now also works. Searching through the MRO makes it even nastier, in my opinion, but I found no other place where all information is directly available.

This comment has been minimized.

@tyralla tyralla requested a review from sobolevn January 17, 2025 17:53

This comment has been minimized.

@tyralla tyralla requested a review from sobolevn January 19, 2025 06:54
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! Just two minor nitpicks :)

mypy/checker.py Outdated Show resolved Hide resolved
mypy/checkexpr.py Outdated Show resolved Hide resolved

a = A()
a.f(1) # E: overload def (self: __main__.A, v: builtins.int) of function __main__.A.f is deprecated: pass `str` instead
a.f("x")
Copy link
Member

Choose a reason for hiding this comment

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

I would also test this case:

Suggested change
a.f("x")
a.f("x")
int_or_str: Union[int, str]
a.f(int_or_str)

It should not raise if all is good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not raise a warning, but why do you think it should not?

(There is not even a warning for a.h, where the implementation is marked as deprecated, which is inconsistent with how functions are handled.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(There is not even a warning for a.h, where the implementation is marked as deprecated, which is inconsistent with how functions are handled.)

I made it consistent in c95f936. However, I added the in my opinion missing warnings to the test case, to prevent us from merging this too early by accident. I am curious to hear why you think the current behaviour is correct. (regarding int_or_str).

tyralla and others added 3 commits January 19, 2025 21:40

This comment has been minimized.

…n analysing "normal" methods (like we do when in other cases).

This comment has been minimized.

@tyralla tyralla requested a review from sobolevn January 20, 2025 06:09
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

There's a failing test now, please take a look. From the brief looks it is related to the Union type change.

It is unspecified in https://typing.readthedocs.io/en/latest/spec/directives.html#deprecated so we can ping @JelleZijlstra to see if that is correct.

@tyralla
Copy link
Collaborator Author

tyralla commented Jan 20, 2025

Yes, the test now fails intentionally (as written somewhere above). I will wait for Jelle's response and adjust either warn_deprecated_overload_item or the TestDeprecatedOverloadedInstanceMethods test case afterwards

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jan 26, 2025

I think it would make sense to issue the warning for the union, since that's the "sound" thing to do. I also checked against pyanalyze and pyright (since they were the reference impl's for PEP 702) and both of them issue a warning. pyanalyze is by Jelle, so that's probably his opinion on this

(If we do this, we should also add a test case for overloaded functions — mypy doesn't currently complain about passing a union that matches a deprecated overload and whatever we decide to do, we should be consistent)

@JelleZijlstra
Copy link
Member

Just so I understand correctly, the question is about whether code like this should warn about the deprecation:

from typing import overload
from warnings import deprecated


@overload
@deprecated("use str")
def f(x: int) -> str: ...
@overload
def f(x: str) -> str: ...
def f(x: int | str) -> str:
    return str(x)

def caller(x: int | str):
    f(x)

And my answer is yes (as Shantanu surmised above). My rule of thumb is: if there would be an error if the deprecation was carried out (i.e., the int overload was removed), then there should be a deprecation warning now.

@tyralla
Copy link
Collaborator Author

tyralla commented Jan 26, 2025

Thank you very much for sharing your opinions on this. I will adjust warn_deprecated_overload_item tomorrow and add corresponding test cases for overloaded functions and descriptors.

This comment has been minimized.

@tyralla
Copy link
Collaborator Author

tyralla commented Jan 27, 2025

Hmm, things are a little different (and eventually more complicated) than I expected. I have to study the responses of check_overload_call in more detail to (hopefully) cover all possible cases. I removed the int_or_str tests so that we do not mix-up things and the current state can be merged (according the the first review) as is (and eventually be cherry-picked into Mypy 1.15?).

I hope to propose a fix for Unions in the next few days.

@tyralla tyralla requested a review from hauntsaninja January 27, 2025 18:56
self.chk.warn_deprecated_overload_item(e.callee.node, e, target=callee_type)
node = e.callee.node
if node is None and member is not None and isinstance(object_type, Instance):
for base in object_type.type.mro:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think we can use object_type.type.get(member)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I adjusted it.

Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you! Let's continue the Union work in the next PR.

@sobolevn sobolevn merged commit 93d1ce4 into python:master Jan 28, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overloaded deprecated method use is not reported
5 participants