From dc8bc31a771df802ff44cc76c9d2978e9fe9025a Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Wed, 14 Jun 2017 16:35:35 -0700 Subject: [PATCH 1/2] Fix accessing qualified import in incremental mode This commit fixes https://github.com/python/mypy/issues/3274 The problem was that mypy was previously doing the following, given an empty cache: 1. Analyze the SCCs (ignoring the builtins) in this exact order: `['c.d']`, then `['c']`, then `['b']`, then `['a']`. No issues here. 2. Parse, typecheck, and write `c.d` to cache -- also no issues here. 2. Parse, typecheck, and write `c` to cache. The error occurs here -- mypy doesn't recognize that `c` has any submodules, and so will not record `c.d` into `c`'s symbol table. This means the saved cache for `c` will be essentially empty. 3. When parsing `b`, mypy *will* recognize that `c.d` is a submodule of `c` due to the import. During the semantic analysis phase, mypy will then actually modify `c`'s symbol table to include `d`. This exact process takes place in `SemanticAnalyzer.add_submodules_to_parent_modules`. This is why typechecking succeeds for `a` and `b` during a fresh However, this change wasn't ever written to the cache, so won't be remembered in the next run! 4. Will parse and typecheck `a`, using the modified (but not preserved) symbol table. Or to put it more succinctly, the code sometimes seems to be relying on the assumption that a symbol table for a given module will not be modified after that SCC is processed. However, this invariant is false due to the 'parent patching' mechanism. It's worth nothing that this patching also occurs during Python's runtime -- it isn't just an artifact of mypy's parsing process. This commit opts for a relatively conservative course of action by simply re-running this patching process when handling fresh SCCs. Other potential fixes include deferring writing to cache until *all* SCCs are processed (which initially seemed like a more robust solution but broke multiple tests when I tried it), or replacing the current parent patching mechanism with something entirely different (which seems like the sort of thing that could subtly break code). --- mypy/build.py | 20 ++++++++++++++++++++ test-data/unit/check-serialize.test | 3 +-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/mypy/build.py b/mypy/build.py index e4b202c4e72b..0729a8b0cb8d 100644 --- a/mypy/build.py +++ b/mypy/build.py @@ -1478,6 +1478,24 @@ def calculate_mros(self) -> None: fixup_module_pass_two(self.tree, self.manager.modules, self.manager.options.quick_and_dirty) + def patch_dependency_parents(self): + """ + In Python, if a and a.b are both modules, running `import a.b` will + modify not only the current module's namespace, but a's namespace as + well -- see SemanticAnalyzer.add_submodules_to_parent_modules for more + details. + + However, this patching process can occur after `a` has been parsed and + serialized during increment mode. Consequently, we need to repeat this + patch when deserializing a cached file. + + This function should be called only when processing fresh SCCs -- the + semantic analyzer will perform this patch for us when processing stale + SCCs. + """ + for dep in self.dependencies: + self.manager.semantic_analyzer.add_submodules_to_parent_modules(dep, True) + def fix_suppressed_dependencies(self, graph: Graph) -> None: """Corrects whether dependencies are considered stale in silent mode. @@ -2010,6 +2028,8 @@ def process_fresh_scc(graph: Graph, scc: List[str]) -> None: graph[id].fix_cross_refs() for id in scc: graph[id].calculate_mros() + for id in scc: + graph[id].patch_dependency_parents() def process_stale_scc(graph: Graph, scc: List[str], manager: BuildManager) -> None: diff --git a/test-data/unit/check-serialize.test b/test-data/unit/check-serialize.test index 2504fe6aa1c5..9576d95c7eef 100644 --- a/test-data/unit/check-serialize.test +++ b/test-data/unit/check-serialize.test @@ -1103,8 +1103,7 @@ main:3: error: Too few arguments for "g" [out2] main:3: error: Too few arguments for "g" -[case testSerializeQualifiedImport-skip] -# This fails currently: https://github.com/python/mypy/issues/3274 +[case testSerializeQualifiedImport] import b b.c.d.f() b.c.d.g() From 0eebc37f4f64b196d9b16141811f913c1a5ebc0d Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Wed, 14 Jun 2017 17:07:02 -0700 Subject: [PATCH 2/2] Add missing type annotation --- mypy/build.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypy/build.py b/mypy/build.py index 0729a8b0cb8d..9e4e3d7ebd22 100644 --- a/mypy/build.py +++ b/mypy/build.py @@ -1478,7 +1478,7 @@ def calculate_mros(self) -> None: fixup_module_pass_two(self.tree, self.manager.modules, self.manager.options.quick_and_dirty) - def patch_dependency_parents(self): + def patch_dependency_parents(self) -> None: """ In Python, if a and a.b are both modules, running `import a.b` will modify not only the current module's namespace, but a's namespace as