Skip to content

New semantic analyzer: fix ambiguity between submodule and local def - #7017

Merged
JukkaL merged 1 commit into
masterfrom
semanal-submodule2
Jun 18, 2019
Merged

JukkaL merged 1 commit into
masterfrom
semanal-submodule2

Conversation

@JukkaL

@JukkaL JukkaL commented Jun 18, 2019

Copy link
Copy Markdown
Collaborator

Now imports are biased to target the submodule of current package,
but elsewhere a local definition with the same name as a submodule
takes precedence. Even though this doesn't quite reflect what
happens at runtime, this seems to cover the typical use cases well,
and the implementation is simple.

Fixes #6828.

…nition

Now imports are biased to target the submodule of current package
target, but elsewhere a local definition with the same name as a
submodule takes precedence.  Even though this doesn't quite reflect
what happens at runtime, this seems to cover the typical use cases
well, and the implementation is simple.

Fixes #6828.
@JukkaL
JukkaL requested a review from ilevkivskyi June 18, 2019 15:34

@ilevkivskyi ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! I have a suggestion for removal of some potentially dead code.

# precedence, but doesn't seem to be important in most use cases.
node = SymbolTableNode(GDEF, self.modules[possible_module_id])
else:
node = module.names.get(id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we still need the special case couple dozen lines below (can't comment there) that checks for submodules added to parent module? (The one that has # Special case: allow replacing submodules with variables.)

It seems to be it should be safe to remove it now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'll look at it later and create a separate PR if it makes sense.

@JukkaL
JukkaL merged commit ec60360 into master Jun 18, 2019
gvanrossum pushed a commit that referenced this pull request Jun 18, 2019
…nition (#7017)

Now imports are biased to target the submodule of current package
target, but elsewhere a local definition with the same name as a
submodule takes precedence.  Even though this doesn't quite reflect
what happens at runtime, this seems to cover the typical use cases
well, and the implementation is simple.

Fixes #6828.
JukkaL added a commit that referenced this pull request Jun 18, 2019
@JelleZijlstra
JelleZijlstra deleted the semanal-submodule2 branch June 18, 2019 16:23
JukkaL added a commit that referenced this pull request Jun 18, 2019
PattenR pushed a commit to PattenR/mypy that referenced this pull request Jun 23, 2019
…nition (python#7017)

Now imports are biased to target the submodule of current package
target, but elsewhere a local definition with the same name as a
submodule takes precedence.  Even though this doesn't quite reflect
what happens at runtime, this seems to cover the typical use cases
well, and the implementation is simple.

Fixes python#6828.
PattenR pushed a commit to PattenR/mypy that referenced this pull request Jun 23, 2019
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.

Uncertainty between submodule and local definition in new semantic analyzer

2 participants