Skip to content

Allow aliases to qualified imports #3680

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 19, 2017
Merged

Allow aliases to qualified imports #3680

merged 7 commits into from
Jul 19, 2017

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Jul 8, 2017 •
edited
Loading

Fixes #3669

This is slightly hacky, since I didn't find anything similar to checker.disable_errors() in semanal and I want to avoid duplicate errors like x has no attribute y and name x.y is not defined.

@ilevkivskyi ilevkivskyi requested a review from JukkaL July 8, 2017 04:58
@ilevkivskyi ilevkivskyi mentioned this pull request Jul 8, 2017
Closed
JelleZijlstra
JelleZijlstra reviewed Jul 8, 2017
View reviewed changes
mypy/semanal.py Outdated
@@ -3381,15 +3381,16 @@ def tvar_scope_frame(self, frame: TypeVarScope) -> Iterator[None]:
yield
self.tvar_scope = old_scope

def lookup(self, name: str, ctx: Context, suppres_errors: bool = False) -> SymbolTableNode:
def lookup(self, name: str, ctx: Context,
suppres_errors: bool = False) -> Optional[SymbolTableNode]:
Copy link
Member

Choose a reason for hiding this comment

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

suppress, not suppres

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 10, 2017

This causes a crash on one of the internal Dropbox codebases. Here's a traceback:

Traceback (most recent call last):
  File "/Users/jukka/src/mypy/scripts/mypy", line 6, in <module>
    main(__file__)
  File "/Users/jukka/src/mypy/mypy/main.py", line 50, in main
    res = type_check_only(sources, bin_dir, options)
  File "/Users/jukka/src/mypy/mypy/main.py", line 97, in type_check_only
    options=options)
  File "/Users/jukka/src/mypy/mypy/build.py", line 196, in build
    graph = dispatch(sources, manager)
  File "/Users/jukka/src/mypy/mypy/build.py", line 1769, in dispatch
    process_graph(graph, manager)
  File "/Users/jukka/src/mypy/mypy/build.py", line 2012, in process_graph
    process_stale_scc(graph, scc, manager)
  File "/Users/jukka/src/mypy/mypy/build.py", line 2107, in process_stale_scc
    graph[id].semantic_analysis()
  File "/Users/jukka/src/mypy/mypy/build.py", line 1664, in semantic_analysis
    self.manager.semantic_analyzer.visit_file(self.tree, self.xpath, self.options, patches)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 295, in visit_file
    self.accept(d)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 3641, in accept
    node.accept(self)
  File "/Users/jukka/src/mypy/mypy/nodes.py", line 754, in accept
    return visitor.visit_class_def(self)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 668, in visit_class_def
    defn.defs.accept(self)
  File "/Users/jukka/src/mypy/mypy/nodes.py", line 815, in accept
    return visitor.visit_block(self)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 1522, in visit_block
    self.accept(s)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 3641, in accept
    node.accept(self)
  File "/Users/jukka/src/mypy/mypy/nodes.py", line 859, in accept
    return visitor.visit_assignment_stmt(self)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 1585, in visit_assignment_stmt
    self.process_module_assignment(s.lvalues, s.rvalue, s)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 2577, in process_module_assignment
    rnode = self.lookup_type_node(rval)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 3266, in lookup_type_node
    n = self.lookup_qualified(t.name, expr, suppress_errors=True)
  File "/Users/jukka/src/mypy/mypy/semanal.py", line 3447, in lookup_qualified
    if isinstance(n.node, TypeInfo):
AttributeError: 'NoneType' object has no attribute 'node'

I'll try to come up with a repro after the release.

We can go ahead with the release without this PR, but if that happens I'd rather not announce module aliases as a new feature yet.

@ilevkivskyi
Copy link
Member Author

@JukkaL Obvious bug from my side, the problem is that break appeared under if not suppress_errors while only the error reporting should depend on this. I have fixed this now, you can decide now what to do.

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 10, 2017

Thanks! The latest change fixed the crash. If I have to time to review this carefully, this may get included in the release. I'll need to finish a few other tasks first.

@ilevkivskyi ilevkivskyi mentioned this pull request Jul 18, 2017
Closed
gvanrossum
gvanrossum reviewed Jul 18, 2017
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG, just some style nits.

mypy/semanal.py Outdated
"""Look up an unqualified name in all active namespaces."""
implicit_name = False
# 1a. Name declared using 'global x' takes precedence
if name in self.global_decls[-1]:
if name in self.globals:
return self.globals[name]
else:
elif not suppress_errors:
Copy link
Member

Choose a reason for hiding this comment

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

I think this ought to be if -- it matches the other chunks in this diff and there if block ends with return.

test-data/unit/check-modules.test Outdated
@@ -1641,6 +1641,40 @@ m = n # E: Cannot assign multiple modules to name 'm' without explicit 'types.M

[builtins fixtures/module.pyi]

[case testModuleAliasToQualifiedImport]
import os.path

Copy link
Member

Choose a reason for hiding this comment

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

I recommend way fewer blank lines in test cases.

test-data/unit/check-modules.test Outdated
@@ -1641,6 +1641,40 @@ m = n # E: Cannot assign multiple modules to name 'm' without explicit 'types.M

[builtins fixtures/module.pyi]

[case testModuleAliasToQualifiedImport]
import os.path
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if picking a different package.module name than os.path makes more sense here? I was confused by the whatever() call until I realized this has nothing to do with the stdlib os.path.

@ilevkivskyi
Copy link
Member Author

@gvanrossum Thanks! I implemented your comments in a new commit.

@gvanrossum gvanrossum merged commit a37788b into python:master Jul 19, 2017
@gvanrossum
Copy link
Member

(Whoops, committed without editing the commit message.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

@gvanrossum gvanrossum gvanrossum left review comments

@JukkaL JukkaL Awaiting requested review from JukkaL

Assignees

@JukkaL JukkaL

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ilevkivskyi @JukkaL @gvanrossum @JelleZijlstra