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

Fix module alias as instance attribute #8259

Merged
merged 4 commits into from Jan 21, 2020
Merged

Conversation

@xhochy
Copy link
Contributor

xhochy commented Jan 8, 2020

Fixes #4291

This is only a partial fix. As I'm new to the codebase, I'm unaware of the way an assignment correctly works. Thus also one of the newly added tests still fail. See the inline comment on the code change for the problem.

continue
# respect explicitly annotated type
if (isinstance(lval.node, Var) and lval.node.type is not None):
continue
lnode = self.current_symbol_table().get(lval.name)
if lnode is None and self.type is not None:

This comment has been minimized.

Copy link
@xhochy

xhochy Jan 8, 2020

Author Contributor

The symbol table selection doesn't work correctly. In the case of the the self.<var> = x definitions, the result of current_symbol_table returns the warnings variable the import statement generated when run in the context of the __init__ function. But we actually want to have lnode = self.type.names.get(lval.name). Is there a better way to get the correct symbol table for the lval?

This comment has been minimized.

Copy link
@xhochy

xhochy Jan 8, 2020

Author Contributor

Would lval.node.info.names.get(lval.name) be a better one?

Copy link
Collaborator

msullivan left a comment

I went to dive into this code to try to produce some guidance to give you, and it ended up being that approximately the minimal way for me to figure out what advice to give was to just go do it. (Though probably I /should/ have been able to figure it out looking at it, but...)

The issue was that the decision for which symbol table to look in needed to be made in a more principled way than "it wasn't present in the local table". Instead it needs to be made by actually checking whether it is a reference to self or not, so I went and did that.

@msullivan msullivan requested a review from ilevkivskyi Jan 15, 2020
Copy link
Collaborator

ilevkivskyi left a comment

LG! There are test failures however.

@msullivan msullivan force-pushed the xhochy:issue-4291 branch from 21c2658 to 8a8a9d3 Jan 21, 2020
@msullivan msullivan merged commit 2d3a1bf into python:master Jan 21, 2020
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
@xhochy

This comment has been minimized.

Copy link
Contributor Author

xhochy commented Jan 22, 2020

@msullivan Thanks!

@xhochy xhochy deleted the xhochy:issue-4291 branch Jan 22, 2020
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.

3 participants
You can’t perform that action at this time.