Fix dmypy traversal of missing graph dependencies - #21818
Open
dnguy078 wants to merge 2 commits into
Open
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dnguy078
marked this pull request as ready for review
August 6, 2026 22:42
The "checked N source files" summary line varies with the bundled typeshed version, which would make the test fail spuriously after a typeshed sync. Start the daemon with --no-error-summary so the test asserts only on the diagnostics that the fix actually affects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #20279.
Root cause
fix_module_deps()runs at the end of every follow-imports increment andre-derives each state's edges so that
state.dependenciesonly ever containsIDs still present in the graph — anything dropped is moved to
state.suppressed. It does not rewritestate.ancestors.find_reachable_changed_modules()traversesstate.dependencies + ancestorsand dereferences
graph[dep]unconditionally. Because ancestors are neverpruned, a stale ancestor ID survives into the next command and crashes the
daemon.
Concretely, with
pkg.testsexcluded from the build, the sequence is:dmypy recheck(first) dropspkg.testsfrom the graph as unreached, butpkg.tests.test_somethingkeepsancestors = ['pkg.tests'].fix_module_deps()cleansdependencies/suppressedand leaves thatdangling ancestor edge in place.
dmypy recheck(second) walks the edge and raisesKeyError: 'pkg.tests'.That two-step sequencing is why the issue reports the crash on repeated
rechecks rather than the first one.
The change
Skip IDs that are no longer in the graph when traversing dependency and
ancestor edges.
direct_imports()gets the same guard: it performs theidentical
graph[dep]dereference, and it is called from inside thefollow-imports worklist loop after
fine_grained_manager.update()calls thatcan remove modules and before
fix_module_deps()runs at the end of theincrement, so its input is not guaranteed clean either. I could not construct a
repro for that path, so that half is defensive symmetry rather than a
demonstrated crash — happy to drop it if you would prefer the minimal diff.
A deeper alternative would be to have
fix_module_deps()pruneancestorstheway it prunes
dependencies. I did not do that here becauseancestorsalsofeeds
refresh_suppressed_submodules(), so pruning it is a wider behaviouralchange than this crash warrants. Glad to take that route instead if you think
the invariant belongs there.
Test
testDaemonRecheckMissingGraphDependencyreproduces the crash via a packageexcluded with
follow_imports = skip. Verified it fails without the fix withexactly the reported
KeyError: 'pkg.tests'(return code 2) and passes withit. The daemon is started with
--no-error-summarydeliberately: thechecked N source filessummary tracks the bundled typeshed and would make thetest fail spuriously after a typeshed sync.
Note that #20279 also has a report of the same
KeyErrorreached through adifferent route (mixing relative and absolute paths across projects). This
patch fixes the dangling-edge dereference itself, so it should cover that
route too, but the test here only exercises the excluded-package case.
Checks run locally
pytest -n0 -q mypy/test/testdaemon.py— 38 passedpytest -n4 -q mypy/test/testfinegrained.py— 755 passed, 27 skippedpython runtests.py self— no issues in 341 source filespre-commit run --files mypy/dmypy_server.py test-data/unit/daemon.test— passedLLM assistance was used while investigating and preparing this change.