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
New semantic analyzer: don't add submodules to symbol tables #7005
Conversation
This affected stubs for requests.
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.
Thanks for the cleanup, looks good! I have couple requests for tests/comments.
Also edit the PR description before merging otherwise this will close #6828.
mypy/newsemanal/semanal.py
Outdated
else: | ||
if self.is_incomplete_namespace(file.fullname()): | ||
self.record_incomplete_ref() | ||
# Handle 'module.foo'. |
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.
Doesn't this comment just duplicate the one two lines above?
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.
Removed one of the comments.
mypy/newsemanal/semanal.py
Outdated
"""Look up a symbol from a module. | ||
|
||
Return None if no matching symbol could be bound. | ||
""" | ||
module = node.fullname() | ||
names = node.names | ||
# Rebind potential references to old version of current module in | ||
# fine-grained incremental mode. | ||
if module == self.cur_mod_id: | ||
names = self.globals |
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.
Since we are here again, could you please add a test that would fail without these two lines. Currently all tests pass if I remove them.
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 couldn't come up with a test that would require this. It's possible that fine-grained dependencies will deal with all the cases that this was supposed to handle. I've removed this now.
var_type = AnyType(TypeOfAny.from_unimported_type) | ||
v = Var(name, type=var_type) | ||
v._fullname = fullname | ||
sym = SymbolTableNode(GDEF, v) |
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.
Maybe add a comment explaining why we create a new Var
every time for technically the same missing module? (IIUC the logic is the same as explained in the docstring for create_getattr_var()
).
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.
Added a comment.
…7005) Previously we added each submodule implicitly to the the symbol table of the parent package. This PR removes this and instead we look up names from the modules dictionary if they aren't found in the symbol table. The change only affects the new semantic analyzer. This provides a foundation that should make python#6828 much easier to address. It also arguably cleans up the code. Also refactor some related code to avoid duplication. This isn't a pure refactor since some error messages are slightly different.
Previously we added each submodule implicitly to the the symbol table of
the parent package. This PR removes this and instead we look up names from
the modules dictionary if they aren't found in the symbol table. The change
only affects the new semantic analyzer.
This provides the foundation that should make it easier to fix #6828. It also
arguably cleans up the code.
Also refactor some related code to avoid duplication.
This isn't a pure refactor since some error messages are slightly different.