[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
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

Fix incorrect name lookup for decorated methods #8175

Merged
merged 9 commits into from Dec 20, 2019

Conversation

Copy link
Contributor

TH3CHARLie commented Dec 19, 2019

Resolves #8161

According to comments of lookup in mypy/semanal.py, when we look up a class attribute, we require that it is defined textually before the reference statement, thus line number is used for comparison.

When function has decorators, its line number is determined by the top decorator instead of the def. That's why #8161's code fails because on line 8, the A in Type[A] has the line number of 8 while the @staticmethod function A has the line number of 7 due to the decorator.

Thus we need to properly handle this by introducing the number of decorators when deciding textural precedence.

fix
TH3CHARLie changed the title [WIP] Fix incorrect name lookup for decorated methods Dec 19, 2019
TH3CHARLie added 2 commits Dec 19, 2019
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! Generally looks good, I have few minor comments.

mypy/semanal.py Outdated Show resolved Hide resolved
test-data/unit/check-classes.test Show resolved Hide resolved
test-data/unit/check-classes.test Show resolved Hide resolved
TH3CHARLie requested a review from ilevkivskyi Dec 19, 2019

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Dec 19, 2019

oops, I made some stupid mistakes

And seems like the @overload case does not apply to this fix

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Dec 19, 2019

I add new logic in semanal.py to check if the class attribute we are accessing is an overloaded function if so, we check whether we are referencing it from a function with the same name, i.e. a variant of these overloads

Copy link
Collaborator

ilevkivskyi left a comment

Thanks for updates! Here are couple more comments.

test-data/unit/check-classes.test Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved
Copy link
Collaborator

ilevkivskyi left a comment

This is still not ready, I have few more comments.

mypy/semanal.py Outdated Show resolved Hide resolved
test-data/unit/check-classes.test Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Dec 20, 2019

Good catch and thanks for the careful review! I check the logic again and indeed there are some mistakes, here's the update:

  • update logic in is_textually_before_statement to ensure if we are referencing an overloaded item, the function return False
  • add negative test
  • For the logic in is_overloaded_item, I have to point out that neither my previous solution nor the solution purposed by you work without modification.
    In this case, we need to check if a FuncDef is one of the overloaded variants and these variants are expressed in either Decorator or FuncDef. The former one is tricky as we need one more step to extract its decorated function for checking. I think we can safely ignore the case when statement i s a Decorator here
TH3CHARLie requested a review from ilevkivskyi Dec 20, 2019
Copy link
Collaborator

ilevkivskyi left a comment

Almost ready now, just couple small comments.

mypy/semanal.py Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Dec 20, 2019

To address that node.impl can also be a Decorator, the logic would be long enough to make it hard to understand, so I deliberately separate single return into two booleans to make the code easy to understand.

Copy link
Collaborator

ilevkivskyi left a comment

LGTM! (assuming the tests pass)

ilevkivskyi merged commit 25c993b into python:master Dec 20, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
TH3CHARLie deleted the TH3CHARLie:fix-8161 branch Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants