Skip to content

Fix incorrect name lookup for decorated methods #8175

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

Merged
merged 9 commits into from
Dec 20, 2019
Merged

Fix incorrect name lookup for decorated methods #8175

merged 9 commits into from
Dec 20, 2019

Conversation

TH3CHARLie
Copy link
Collaborator

@TH3CHARLie 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.

@TH3CHARLie TH3CHARLie changed the title [WIP] Fix incorrect name lookup for decorated methods Dec 19, 2019
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

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

@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Dec 19, 2019

oops, I made some stupid mistakes

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

@TH3CHARLie
Copy link
Collaborator Author

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
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks for updates! Here are couple more comments.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

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

@TH3CHARLie
Copy link
Collaborator 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

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Almost ready now, just couple small comments.

@TH3CHARLie
Copy link
Collaborator Author

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
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

LGTM! (assuming the tests pass)

@ilevkivskyi ilevkivskyi merged commit 25c993b into python:master Dec 20, 2019
@TH3CHARLie TH3CHARLie deleted the fix-8161 branch December 21, 2019 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypy incorrectly resolves names inside decorated methods
2 participants