-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Don't treat functions with reversible dunder names as such #5421
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
Conversation
Previously, functions that had the same name as a reversible dunder method (for example, __rpow__) were checked as if they were such a method. This could lead to crashes and false positives. Closes: python#5419
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.
Looks good to me -- my comments below are just me nitpicking. I'll merge once you've had a chance to look through the feedback.
mypy/messages.py
Outdated
@@ -997,6 +997,7 @@ def overloaded_signatures_ret_specific(self, index: int, context: Context) -> No | |||
def operator_method_signatures_overlap( | |||
self, reverse_class: TypeInfo, reverse_method: str, forward_class: Type, | |||
forward_method: str, context: Context) -> None: | |||
assert reverse_class is not 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.
I think we can remove this assert -- we already do a check and an assert up above. It'll also be more consistent this way: we don't really perform checks like these in other similar message methods.
(In fact, it actually ought to be impossible for reverse_class
to be None at this point since we type-check mypy itself under strict-optional mode: the fact that it sometimes could be is an unfortunate consequence of this cast. I'd rather we eventually fix that TODO so we don't have to keep adding asserts -- but that's definitely very much out-of-scope for this PR.)
test-data/unit/check-modules.test
Outdated
@@ -2495,3 +2495,6 @@ def __getattr__(attr: str) -> Any: ... | |||
class Cls: ... | |||
[builtins fixtures/module.pyi] | |||
[out] | |||
|
|||
[case testFunctionWithReversibleDunderName] | |||
def __rpow__(self) -> int: ... |
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 we're at it, maybe leave a test case checking to see what happens if we make this function an overload? And what happens if we define a top-level in-place operator method (like __iadd__
)?
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.
Good idea. I will add tests for __add__
, __radd__
, and __iadd__
, so we have covered all three cases.
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 ran into this when running mypy against the CPython code base where it was invalidly complained about. Thank you for fixing this!
Thanks! |
Previously, functions that had the same name as a reversible dunder method (for example,
__rpow__
) were checked as if they were such a method. This could lead to crashes and false positives.Closes: #5419