Never compute the DocMap with stale data#4936
Open
fendor wants to merge 3 commits into
Open
Conversation
fendor
force-pushed
the
wip/overlapping-instances
branch
from
f18d717 to
c383e1c
Compare
GetDocMap reads TypeCheck, GhcSessionDeps and GetHieAst via independent useWithStale_ calls. An edit that breaks typechecking lets GhcSessionDeps re-evaluate with a fresh (narrower) HPT while TypeCheck and GetHieAst fall back to their last-successful values. mkDocMap then runs with a fresh HscEnv whose HPT is missing modules that the stale RefMap still references; getDocsBatch calls loadSysInterface on those modules; loadInterface, finding them absent from the HUG, puts the home-module interfaces -- with their instance environments -- into the shared EPS IORef. The EPS never evicts, so subsequent typechecks that legitimately have those modules in the HPT see each ClsInst twice (via ie_global from the EPS and ie_local from hptInstancesBelow) and GHC reports "Overlapping instance" with both matches pointing at the same source location. The test edits away an import to provoke the pollution, then restores the import and asserts the repaired file typechecks without an overlapping- instance diagnostic. Wrapped in expectFailBecause so CI stays green until a fix lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
As the regression test 'eps-pollution' demonstrates, changing the imports of a module with class instances can poison the HLS session. To fix this, we simply shouldn't use stale results for computing the `DocMap`. Frankly, there is no reason to use stale results when we are not even using the `PositionMapping`, this sounds like a recipe for disaster in general. Especially, since the source locations matter.
fendor
force-pushed
the
wip/overlapping-instances
branch
from
c383e1c to
05b03ab
Compare
Collaborator
Author
|
Interesting data point, the test doesn't fail on GHC 9.14.1, but it fails on GHC 9.10.3 🤔 |
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
2 participants
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.
As the regression test 'eps-pollution' demonstrates, changing the
imports of a module with class instances can poison the HLS session.
To fix this, we simply shouldn't use stale results for computing the
DocMap.Frankly, there is no reason to use stale results when we are not even
using the
PositionMapping, this sounds like a recipe for disaster ingeneral. Especially, since the source locations matter in this instance.
The test has been contributed by @mniip and claude, thank you very much for identifying the issue!
I removed simplified the tests slightly, removing irrelevant details, but it is more or less the same.
I also refactored
checkHoverto add an additional assertion to the test.This gets rid of a couple of lines of code, which is nice.
Should close #3694