Skip to content

Fix crash in astdiff.py related to ec11a500f. #5126

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

Merged
merged 1 commit into from
Jun 1, 2018

Conversation

whiten
Copy link
Contributor

@whiten 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

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 02:13
@msullivan
Copy link
Collaborator

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

@msullivan msullivan mentioned this pull request Jun 1, 2018
@msullivan
Copy link
Collaborator

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
msullivan pushed a commit that referenced this pull request Jun 1, 2018
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants