From 438f50539fecf640a19a1ffc753bc968d353d3d4 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Thu, 25 Aug 2016 17:03:30 -0700 Subject: [PATCH 1/4] Remove the patch parent phase This commit removes the "patch_parent" phase. The logic within those phases are now run only when a file is actually importing something, instead of being done automatically. This fixes some irregularities with how imports are handled. In particular, the "patch parent" stage was interacting poorly with implicit module references since the automatic patch would add back in an explicit module reference even when it was removed, which could preserve an implicit reference that should have instead been broken. That is, the presense of "patch parent" was sometimes concealing errors that should have been caught. --- mypy/build.py | 17 ----------------- mypy/semanal.py | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/mypy/build.py b/mypy/build.py index fe40e3d0895aa..ff02c49392ef9 100644 --- a/mypy/build.py +++ b/mypy/build.py @@ -1399,19 +1399,6 @@ def parse_file(self) -> None: self.dep_line_map = dep_line_map self.check_blockers() - def patch_parent(self) -> None: - # Include module in the symbol table of the enclosing package. - if '.' not in self.id: - return - manager = self.manager - modules = manager.modules - parent, child = self.id.rsplit('.', 1) - if parent in modules: - manager.trace("Added %s.%s" % (parent, child)) - modules[parent].names[child] = SymbolTableNode(MODULE_REF, self.tree, parent) - else: - manager.log("Hm... couldn't add %s.%s" % (parent, child)) - def semantic_analysis(self) -> None: with self.wrap_context(): self.manager.semantic_analyzer.visit_file(self.tree, self.xpath) @@ -1682,8 +1669,6 @@ def process_fresh_scc(graph: Graph, scc: List[str]) -> None: """Process the modules in one SCC from their cached data.""" for id in scc: graph[id].load_tree() - for id in scc: - graph[id].patch_parent() for id in scc: graph[id].fix_cross_refs() for id in scc: @@ -1697,8 +1682,6 @@ def process_stale_scc(graph: Graph, scc: List[str]) -> None: # If the former, parse_file() is a no-op. graph[id].parse_file() graph[id].fix_suppressed_dependencies(graph) - for id in scc: - graph[id].patch_parent() for id in scc: graph[id].semantic_analysis() for id in scc: diff --git a/mypy/semanal.py b/mypy/semanal.py index 995610521f5f9..7559b619d497e 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -894,6 +894,24 @@ def visit_import(self, i: Import) -> None: base = id.split('.')[0] self.add_module_symbol(base, base, module_public=module_public, context=i) + self.add_parent_modules(id, module_public) + + def add_parent_modules(self, id: str, module_public: bool) -> None: + """Adds all parent modules to the symbol table given a fully qualified import. + + It turns out that when you have imports of the form `import a.b.c`, Python will + actually import not only module `a.b.c`, but modules `a.b` and `a` as well. This + method adds in those checks. + + Note that this does NOT happen when you have imports of the form + `import a.b.c as foo` -- here, only module `a.b.c` is loaded under the name `foo`.""" + while '.' in id: + parent, child = id.rsplit('.', 1) + if parent in self.modules and id in self.modules: + sym = SymbolTableNode(MODULE_REF, self.modules[id], parent, + module_public=module_public) + self.modules[parent].names[child] = sym + id = parent def add_module_symbol(self, id: str, as_id: str, module_public: bool, context: Context) -> None: @@ -910,6 +928,15 @@ def visit_import_from(self, imp: ImportFrom) -> None: module = self.modules[import_id] for id, as_id in imp.names: node = module.names.get(id) + + # If the module does not contain a symbol with the name 'id', + # try checking if it's a module instead. + if id not in module.names or node.kind == UNBOUND_IMPORTED: + possible_module_id = import_id + '.' + id + mod = self.modules.get(possible_module_id) + if mod is not None: + node = SymbolTableNode(MODULE_REF, mod, import_id) + if node and node.kind != UNBOUND_IMPORTED: node = self.normalize_type_alias(node, imp) if not node: From 6364a0bdf334fd213af6641a6fee4d1535b56b32 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Thu, 25 Aug 2016 17:06:54 -0700 Subject: [PATCH 2/4] Fix bugs revealed by patch_parent fix This commit tweaks some subtly broken imports that were not caught before due to the automatic patching patch_parent performed. --- mypy/checkexpr.py | 5 ++--- mypy/checkstrformat.py | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index f5b20138f88f9..11abe5446ae40 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -36,7 +36,6 @@ from mypy.constraints import get_actual_type from mypy.checkstrformat import StringFormatterChecker from mypy.expandtype import expand_type -import mypy.checkexpr from mypy import experiments @@ -61,7 +60,7 @@ class ExpressionChecker: # This is shared with TypeChecker, but stored also here for convenience. msg = None # type: MessageBuilder - strfrm_checker = None # type: mypy.checkstrformat.StringFormatterChecker + strfrm_checker = None # type: StringFormatterChecker def __init__(self, chk: 'mypy.checker.TypeChecker', @@ -69,7 +68,7 @@ def __init__(self, """Construct an expression type checker.""" self.chk = chk self.msg = msg - self.strfrm_checker = mypy.checkexpr.StringFormatterChecker(self, self.chk, self.msg) + self.strfrm_checker = StringFormatterChecker(self, self.chk, self.msg) def visit_name_expr(self, e: NameExpr) -> Type: """Type check a name expression. diff --git a/mypy/checkstrformat.py b/mypy/checkstrformat.py index 952256c34afc0..23ec912ecf809 100644 --- a/mypy/checkstrformat.py +++ b/mypy/checkstrformat.py @@ -13,6 +13,7 @@ if False: # break import cycle only needed for mypy import mypy.checker + import mypy.checkexpr from mypy import messages from mypy.messages import MessageBuilder From a1dd1929f68e0130b6b0adc22ffec55401195c87 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Fri, 26 Aug 2016 16:37:27 -0700 Subject: [PATCH 3/4] Test check parent patching works for all imports Previously, my modifications to the parent patching process only took place when loading imports of the form `import a.b.c`. This commit adds more extensive tests to confirm the parent patching happens in all kinds of imports (i.e. `from a.b import c`). --- test-data/unit/check-modules.test | 163 +++++++++++++++++++++++++++++ test-data/unit/fixtures/module.pyi | 1 + 2 files changed, 164 insertions(+) diff --git a/test-data/unit/check-modules.test b/test-data/unit/check-modules.test index 1fe861e40dc62..c2566c98ef992 100644 --- a/test-data/unit/check-modules.test +++ b/test-data/unit/check-modules.test @@ -711,6 +711,169 @@ import m.a [out] +-- Checks dealing with submodules and different kinds of imports +-- ------------------------------------------------------------- + +[case testSubmoduleRegularImportAddsAllParents] +import a.b.c +reveal_type(a.value) # E: Revealed type is 'builtins.int' +reveal_type(a.b.value) # E: Revealed type is 'builtins.str' +reveal_type(a.b.c.value) # E: Revealed type is 'builtins.float' +b.value # E: Name 'b' is not defined +c.value # E: Name 'c' is not defined + +[file a/__init__.py] +value = 3 +[file a/b/__init__.py] +value = "a" +[file a/b/c.py] +value = 3.2 +[out] + +[case testSubmoduleImportAsDoesNotAddParents] +import a.b.c as foo +reveal_type(foo.value) # E: Revealed type is 'builtins.float' +a.value # E: Name 'a' is not defined +b.value # E: Name 'b' is not defined +c.value # E: Name 'c' is not defined + +[file a/__init__.py] +value = 3 +[file a/b/__init__.py] +value = "a" +[file a/b/c.py] +value = 3.2 +[out] + +[case testSubmoduleImportFromDoesNotAddParents] +from a import b +reveal_type(b.value) # E: Revealed type is 'builtins.str' +b.c.value # E: "module" has no attribute "c" +a.value # E: Name 'a' is not defined + +[file a/__init__.py] +value = 3 +[file a/b/__init__.py] +value = "a" +[file a/b/c.py] +value = 3.2 +[builtins fixtures/module.pyi] +[out] + +[case testSubmoduleImportFromDoesNotAddParents2] +from a.b import c +reveal_type(c.value) # E: Revealed type is 'builtins.float' +a.value # E: Name 'a' is not defined +b.value # E: Name 'b' is not defined + +[file a/__init__.py] +value = 3 +[file a/b/__init__.py] +value = "a" +[file a/b/c.py] +value = 3.2 +[out] + +[case testSubmoduleRegularImportNotDirectlyAddedToParent] +import a.b.c +def accept_float(x: float) -> None: pass +accept_float(a.b.c.value) + +[file a/__init__.py] +value = 3 +b.value +a.b.value + +[file a/b/__init__.py] +value = "a" +c.value +a.b.c.value + +[file a/b/c.py] +value = 3.2 +[out] +main:1: note: In module imported here: +tmp/a/b/__init__.py:2: error: Name 'c' is not defined +tmp/a/b/__init__.py:3: error: Name 'a' is not defined +tmp/a/__init__.py:2: error: Name 'b' is not defined +tmp/a/__init__.py:3: error: Name 'a' is not defined + +[case testSubmoduleMixingLocalAndQualifiedNames] +from a.b import MyClass +val1 = None # type: a.b.MyClass # E: Name 'a' is not defined +val2 = None # type: MyClass + +[file a/__init__.py] +[file a/b.py] +class MyClass: pass +[out] + +[case testSubmoduleMixingImportFrom] +import parent.child + +[file parent/__init__.py] + +[file parent/common.py] +class SomeClass: pass + +[file parent/child.py] +from parent.common import SomeClass +from parent import common +foo = parent.common.SomeClass() + +[builtins fixtures/module.pyi] +[out] +main:1: note: In module imported here: +tmp/parent/child.py:3: error: Name 'parent' is not defined + +[case testSubmoduleMixingImportFromAndImport] +import parent.child + +[file parent/__init__.py] + +[file parent/common.py] +class SomeClass: pass + +[file parent/unrelated.py] +class ShouldNotLoad: pass + +[file parent/child.py] +from parent.common import SomeClass +import parent + +# Note, since this might be unintuitive -- when `parent.common` is loaded in any way, +# shape, or form, it's added to `parent`'s namespace, which is why the below line +# succeeds. +foo = parent.common.SomeClass() +reveal_type(foo) +bar = parent.unrelated.ShouldNotLoad() + +[builtins fixtures/module.pyi] +[out] +main:1: note: In module imported here: +tmp/parent/child.py:8: error: Revealed type is 'parent.common.SomeClass' +tmp/parent/child.py:9: error: "module" has no attribute "unrelated" + +[case testSubmoduleMixingImportFromAndImport2] +import parent.child + +[file parent/__init__.py] + +[file parent/common.py] +class SomeClass: pass + +[file parent/child.py] +from parent import common +import parent +foo = parent.common.SomeClass() +reveal_type(foo) + +[builtins fixtures/module.pyi] +[out] +main:1: note: In module imported here: +tmp/parent/child.py:4: error: Revealed type is 'parent.common.SomeClass' + + -- Misc [case testInheritFromBadImport] diff --git a/test-data/unit/fixtures/module.pyi b/test-data/unit/fixtures/module.pyi index 38fe4cc318a3b..e79ca284de225 100644 --- a/test-data/unit/fixtures/module.pyi +++ b/test-data/unit/fixtures/module.pyi @@ -5,3 +5,4 @@ class type: pass class function: pass class int: pass class str: pass +class bool: pass From a4588af8cdbefad204c7b208c26c4ce3688232df Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Fri, 26 Aug 2016 16:39:42 -0700 Subject: [PATCH 4/4] Correct parent patching to work for all imports My previous modification to parent patching took place only when doing imports of the form `import a.b.c`. This commit changes this so parent patching happens when doing any kind of import. --- mypy/semanal.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 7559b619d497e..7ba20cf4877eb 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -894,20 +894,26 @@ def visit_import(self, i: Import) -> None: base = id.split('.')[0] self.add_module_symbol(base, base, module_public=module_public, context=i) - self.add_parent_modules(id, module_public) + self.add_submodules_to_parent_modules(id, module_public) - def add_parent_modules(self, id: str, module_public: bool) -> None: - """Adds all parent modules to the symbol table given a fully qualified import. + def add_submodules_to_parent_modules(self, id: str, module_public: bool) -> None: + """Recursively adds a reference to a newly loaded submodule to its parent. - It turns out that when you have imports of the form `import a.b.c`, Python will - actually import not only module `a.b.c`, but modules `a.b` and `a` as well. This - method adds in those checks. + When you import a submodule in any way, Python will add a reference to that + submodule to its parent. So, if you do something like `import A.B` or + `from A import B` or `from A.B import Foo`, Python will add a reference to + module A.B to A's namespace. - Note that this does NOT happen when you have imports of the form - `import a.b.c as foo` -- here, only module `a.b.c` is loaded under the name `foo`.""" + Note that this "parent patching" process is completely independent from any + changes made to the *importer's* namespace. For example, if you have a file + named `foo.py` where you do `from A.B import Bar`, then foo's namespace will + be modified to contain a reference to only Bar. Independently, A's namespace + will be modified to contain a reference to `A.B`. + """ while '.' in id: parent, child = id.rsplit('.', 1) - if parent in self.modules and id in self.modules: + modules_loaded = parent in self.modules and id in self.modules + if modules_loaded and child not in self.modules[parent].names: sym = SymbolTableNode(MODULE_REF, self.modules[id], parent, module_public=module_public) self.modules[parent].names[child] = sym @@ -926,6 +932,7 @@ def visit_import_from(self, imp: ImportFrom) -> None: import_id = self.correct_relative_import(imp) if import_id in self.modules: module = self.modules[import_id] + self.add_submodules_to_parent_modules(import_id, True) for id, as_id in imp.names: node = module.names.get(id) @@ -936,6 +943,7 @@ def visit_import_from(self, imp: ImportFrom) -> None: mod = self.modules.get(possible_module_id) if mod is not None: node = SymbolTableNode(MODULE_REF, mod, import_id) + self.add_submodules_to_parent_modules(possible_module_id, True) if node and node.kind != UNBOUND_IMPORTED: node = self.normalize_type_alias(node, imp) @@ -1018,6 +1026,7 @@ def visit_import_all(self, i: ImportAll) -> None: i_id = self.correct_relative_import(i) if i_id in self.modules: m = self.modules[i_id] + self.add_submodules_to_parent_modules(i_id, True) for name, node in m.names.items(): node = self.normalize_type_alias(node, i) if not name.startswith('_') and node.module_public: