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 crash in astdiff.py related to ec11a500f. #5126

Merged
merged 1 commit into from Jun 1, 2018

Conversation

Projects
None yet
2 participants
@whiten
Contributor

whiten commented May 31, 2018

I had been seeing the same crash that patch aimed to fix running against Quip's
codebase, but have been unable to reduce it to a test case. With this patch,
we're now able to use dmypy.

Traceback from crash after ec11a50:

File "/usr/local/lib/python3.6/site-packages/mypy/server/update.py", line 760, in propagate_changes_using_dependencies
triggered |= reprocess_nodes(manager, graph, id, nodes, deps)
File "/usr/local/lib/python3.6/site-packages/mypy/server/update.py", line 920, in reprocess_nodes
new_symbols_snapshot = snapshot_symbol_table(file_node.fullname(), file_node.names)
File "/usr/local/lib/python3.6/site-packages/mypy/server/astdiff.py", line 159, in snapshot_symbol_table
result[name] = snapshot_definition(node, common)
File "/usr/local/lib/python3.6/site-packages/mypy/server/astdiff.py", line 214, in snapshot_definition
symbol_table = snapshot_symbol_table(prefix, node.names)
File "/usr/local/lib/python3.6/site-packages/mypy/server/astdiff.py", line 154, in snapshot_symbol_table
assert symbol.kind != UNBOUND_IMPORTED

Fix crash in astdiff.py related to ec11a50.
I had been seeing the same crash that patch aimed to fix running against Quip's
codebase, but have been unable to reduce it to a test case. With this patch,
we're now able to use dmypy.

Traceback from crash after ec11a50:

  File "/usr/local/lib/python3.6/site-packages/mypy/server/update.py", line 760, in propagate_changes_using_dependencies
    triggered |= reprocess_nodes(manager, graph, id, nodes, deps)
  File "/usr/local/lib/python3.6/site-packages/mypy/server/update.py", line 920, in reprocess_nodes
    new_symbols_snapshot = snapshot_symbol_table(file_node.fullname(), file_node.names)
  File "/usr/local/lib/python3.6/site-packages/mypy/server/astdiff.py", line 159, in snapshot_symbol_table
    result[name] = snapshot_definition(node, common)
  File "/usr/local/lib/python3.6/site-packages/mypy/server/astdiff.py", line 214, in snapshot_definition
    symbol_table = snapshot_symbol_table(prefix, node.names)
  File "/usr/local/lib/python3.6/site-packages/mypy/server/astdiff.py", line 154, in snapshot_symbol_table
    assert symbol.kind != UNBOUND_IMPORTED

@gvanrossum gvanrossum requested a review from msullivan May 31, 2018

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Jun 1, 2018

This looks totally plausible. I'll try to see if I can reverse-engineer a testcase.

@msullivan msullivan referenced this pull request Jun 1, 2018

Closed

Release 0.610 planning #5096

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Jun 1, 2018

I'm going to merge this because I believe it to be likely to do more good than harm, though I haven't been able to track down a testcase.

Part of the reason I'm willing to do this is that I think that it probably isn't actually necessary for aststrip to reset the symbol table entries for imports. I think it might be fine to just delete them all. aststrip generally tries to restore things to the state that they were in after semanal pass1, and semanal pass1 needs to populate the symbol table before semanal pass 2 so the names are visible when there are import cycles. But in our case, that doesn't obtain: any of the other modules are already loaded, so it should be fine to just delete the entries and let them get fixed when we do semanal pass 2 again. In testing this seemed to work. @JukkaL, thoughts?

That is a bigger change, though, and I'm more willing to cherry-pick this one, so I'm going to do that and follow-up on it later.

@msullivan msullivan merged commit eb1bb06 into python:master Jun 1, 2018

2 checks passed

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

msullivan added a commit that referenced this pull request Jun 1, 2018

Fix crash in astdiff.py related to ec11a50. (#5126)
This fixes a crash reported in Quip's codebase related to UNBOUND_IMPORTED symbols.
Unfortunately we don't have a nice test case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment