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

Fix accessing qualified import in incremental mode #3548

Merged
merged 2 commits into from Jun 20, 2017

Conversation

Projects
None yet
2 participants
@Michael0x2a
Collaborator

Michael0x2a commented Jun 15, 2017

This pull request fixes #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.
  3. 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.
  4. When parsing b, mypy will recognize that c.d is a submodule of c due to the import. During the semantic analysis phase (more precisely, in SemanticAnalyzer.add_submodules_to_parent_modules), mypy will actually modify c's symbol table to include d, which is why typechecking succeeds for b and a during a fresh run. However, this change wasn't ever written to the cache, so won't be remembered when re-running incremental mode!
  5. 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.

This commit opts for a relatively conservative course of action by simply re-running this patching process when handling fresh SCCs.

Other potential fixes I considered included deferring writing to cache until all SCCs are processed to try and preserve this info and restore the above invariant (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).

Michael0x2a added some commits Jun 14, 2017

Fix accessing qualified import in incremental mode
This commit fixes #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).

@JukkaL JukkaL requested a review from gvanrossum Jun 20, 2017

@gvanrossum gvanrossum merged commit 9ff0f0b into python:master Jun 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 20, 2017

Member

Thanks! I agree with your choice of fix.

Member

gvanrossum commented Jun 20, 2017

Thanks! I agree with your choice of fix.

@Michael0x2a Michael0x2a deleted the Michael0x2a:fix-qualified-imports-in-incremental-mode branch Jun 20, 2017

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