-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Support @deprecated() on overloaded __get__ for generic descriptors
#18333
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
This comment has been minimized.
This comment has been minimized.
3e0684b to
8ca2a14
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| ): | ||
| if selftype is not None: | ||
| candidate = bind_self(candidate, selftype) | ||
| candidate = bind_self( |
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'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?
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.
Hum, in analyze_descriptor_access, we pass inferred_dunder_get_type as target:
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
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.
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?
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.
analyze_descriptor_access calls analyze_decorator_or_funcbase_access (eventually) calls bind_self. Is that what you are looking for?
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.
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.
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.
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?
|
Presumably superseded by #19556 (see also #18682 (comment)). |
Fixes #18323.
Follow up on #18090.
Notes:
analyze_descriptor_access, there is a distinction betweentypanddescriptor_type:mypy/mypy/checkmember.py
Lines 687 to 688 in 9ff9946
map_instance_to_supertypereturnsdescriptor_typeunchanged. The reason I'm mentioning this is thatexpand_type_by_instanceusestypand notdescriptor_type. Inwarn_deprecated_overload_item, we have access todescriptor_type(passed asselftype):mypy/mypy/checkmember.py
Lines 726 to 728 in 9ff9946
expand_type_by_instance(candidate, selftype). Is this going to be an issue?selftypetoInstance. Is this going to be an issue?