Fix bug around resetting presentation compilers. #1590
Conversation
Previously, Metals didn't correctly reset all presentation compiler after a successful compilation. Users could observe this bug when completions don't return symbols from other files that have been recently changed. Now, Metals resets all presentation compilers that may be affected by a successful compile notification making it less likely that completions/hover/signatureHelp return stale results. This root cause of this bug was discovered in scalameta#1523 by Tomasz. The problem was that Metals only collected the leaf build targets that depend on the compiled target. Now, we reset all inverse dependencies regardless if they're leaf targets or not.
|
Kudos to @tgodzik for tracking down the root cause of this bug. I have observed this bug for a long time but didn't know how to reliably reproduce it or fix it. |
| "package a; object A" | ||
| ) | ||
| _ = assertNoDiagnostics() | ||
| _ = assertEquals(0, server.server.loadedPresentationCompilerCount()) |
There was a problem hiding this comment.
Shouldn't we try to change both files to contain new names and assert that both new names are picked up from another file after only compiling a?
I think that would test the same thing and not require the additional loadedPresentationCompilerCount
There was a problem hiding this comment.
I agree with the principle that we should reproduce the user-facing situation instead of testing against implementation details. My concern with reproducing the exact scenario is that we'll end up with a flaky test.
It's critical that we reset the presentation compiler for getting good user experience and I think it will be helpful in the future to have isLoaded() as a toolkit to test related fixes.
Previously, Metals didn't correctly reset all presentation compiler
after a successful compilation. Users could observe this bug when
completions don't return symbols from other files that have been
recently changed.
Now, Metals resets all presentation compilers that may be affected by a
successful compile notification making it less likely that
completions/hover/signatureHelp return stale results.
This root cause of this bug was discovered in #1523 by Tomasz. The
problem was that Metals only collected the leaf build targets that
depend on the compiled target. Now, we reset all inverse dependencies
regardless if they're leaf targets or not.