Fix diagnostics update bug #959

pepeiborra · 2020-12-20T15:10:15Z

Given a FOI F with non null typechecking diagnostics D, imagine the following scenario:

An edit notification for F is received, creating a new version
GetModTime is executed, producing 0 diagnostics.
2.1 updateFileDiagnostics is called
2.2 setStageDiagnostics is called
2.3 LSP.updateDiagnostics is called with a new version, resetting all the
diagnostics for F
2.4 newDiags=[] in updateFileDiagnostics, which is different from D (the last
published diagnostics), which enqueues a new publishDiagnostics [] in the
Debouncer
An edit notification for F is received before typechecking has a chance to
run which undoes the previous edit
The debouncer publishes the empty set of diagnostics after waiting 0.1s
GetFileContents runs and since the contents of the file haven't changed since
the last time it ran, early cutoff skips everything donwstream

Since TypeCheck is skipped, the empty set of diagnostics stays published until
another edit comes. This is a bug.

The goal of this change is to prevent setStageDiagnostics from losing
diagnostics from other stages. To achieve this, we recover the old diagnostics
for all stages and merge them with the new stage.

Fixes #949

The test suite includes a new custom command that can be used to wait for arbitrary Shake actions, which can be used to implement the improvements outlined in #955

withMVar' is used to update the shakeSession var and it's crucial that the third argument is not interrupted. 'mask' can still be interrupted for I/O actions and, while we were careful to ensure none was used, if it ever breaks it will lead to very hard to debug problems.

Closes #955

Since the contents of the buffer are not tracked by the fingerprint.

Given a FOI F with non null typechecking diagnostics D, imagine the following scenario: 1. An edit notification for F is received, creating a new version 2. GetModTime is executed, producing 0 diagnostics. 2.1 updateFileDiagnostics is called 2.2 setStageDiagnostics is called 2.3 LSP.updateDiagnostics is called with a new version, resetting all the diagnostics for F 2.4 newDiags=[] in updateFileDiagnostics, which is different from D (the last published diagnostics), which enqueues a new publishDiagnostics [] in the Debouncer 3. An edit notification for F is received before typechecking has a chance to run which undoes the previous edit 4. The debouncer publishes the empty set of diagnostics after waiting 0.1s 5. GetFileContents runs and since the contents of the file haven't changed since the last time it ran, early cutoff skips everything donwstream Since TypeCheck is skipped, the empty set of diagnostics stays published until another edit comes. The goal of this change is to prevent setStageDiagnostics from losing diagnostics from other stages. To achieve this, we recover the old diagnostics for all stages and merge them with the new stage.

src/Development/IDE/Core/Rules.hs

src/Development/IDE/Core/Shake.hs

wz1000 · 2020-12-20T16:40:04Z

Looks good, must have been a tough one to crack!

pepeiborra · 2020-12-20T17:09:50Z

It was quite tricky, yeah. I have a vague memory of the diagnostics store semantics for updates causing trouble in the past. @cocreature @ndmitchell do you recall this? To summarise, the problem is that updating the diagnostics of file F for one source, say "GetFileContents", throws away all the F diagnostics if the version is newer than the version in the store.

jneira

Thanks for improve test infra on the way

tomsmeding · 2020-12-20T22:43:12Z

I have used this branch (well, aad22e6 to be precise) for an hour or two and it seems my problems reported in #949 are indeed fixed! Thanks a lot for the hard work!

These tests were underspecified and broke with the recent improvements to ghcide diagnostics in haskell/ghcide#959 and included in this merge. Fixed by waiting specifically for the typecheck diagnostics and by being less prescriptive in the number and order of code actions

* Preventively switch to uninterruptible mask in withMVar' withMVar' is used to update the shakeSession var and it's crucial that the third argument is not interrupted. 'mask' can still be interrupted for I/O actions and, while we were careful to ensure none was used, if it ever breaks it will lead to very hard to debug problems. * refactor: move to RuleTypes * Add a TestRequest to wait for arbitrary ide actions Closes haskell/ghcide#955 * expectCurrentDiagnostics * Add a test suite for cancellation * Introduce --test-no-kick to fix cancellation tests reliability * delete unsafeClearDiagnostics (unused) * GetModSummaryWithoutTimestamps - remove StringBuffer Since the contents of the buffer are not tracked by the fingerprint. * Fix diagnostics bug Given a FOI F with non null typechecking diagnostics D, imagine the following scenario: 1. An edit notification for F is received, creating a new version 2. GetModTime is executed, producing 0 diagnostics. 2.1 updateFileDiagnostics is called 2.2 setStageDiagnostics is called 2.3 LSP.updateDiagnostics is called with a new version, resetting all the diagnostics for F 2.4 newDiags=[] in updateFileDiagnostics, which is different from D (the last published diagnostics), which enqueues a new publishDiagnostics [] in the Debouncer 3. An edit notification for F is received before typechecking has a chance to run which undoes the previous edit 4. The debouncer publishes the empty set of diagnostics after waiting 0.1s 5. GetFileContents runs and since the contents of the file haven't changed since the last time it ran, early cutoff skips everything donwstream Since TypeCheck is skipped, the empty set of diagnostics stays published until another edit comes. The goal of this change is to prevent setStageDiagnostics from losing diagnostics from other stages. To achieve this, we recover the old diagnostics for all stages and merge them with the new stage. * Fix hlint * Use Map.insert for clarity * Fix redundant imports * Fix "code actions after edit" experiment"

These tests were underspecified and broke with the recent improvements to ghcide diagnostics in haskell/ghcide#959 and included in this merge. Fixed by waiting specifically for the typecheck diagnostics and by being less prescriptive in the number and order of code actions

* Preventively switch to uninterruptible mask in withMVar' withMVar' is used to update the shakeSession var and it's crucial that the third argument is not interrupted. 'mask' can still be interrupted for I/O actions and, while we were careful to ensure none was used, if it ever breaks it will lead to very hard to debug problems. * refactor: move to RuleTypes * Add a TestRequest to wait for arbitrary ide actions Closes haskell/ghcide#955 * expectCurrentDiagnostics * Add a test suite for cancellation * Introduce --test-no-kick to fix cancellation tests reliability * delete unsafeClearDiagnostics (unused) * GetModSummaryWithoutTimestamps - remove StringBuffer Since the contents of the buffer are not tracked by the fingerprint. * Fix diagnostics bug Given a FOI F with non null typechecking diagnostics D, imagine the following scenario: 1. An edit notification for F is received, creating a new version 2. GetModTime is executed, producing 0 diagnostics. 2.1 updateFileDiagnostics is called 2.2 setStageDiagnostics is called 2.3 LSP.updateDiagnostics is called with a new version, resetting all the diagnostics for F 2.4 newDiags=[] in updateFileDiagnostics, which is different from D (the last published diagnostics), which enqueues a new publishDiagnostics [] in the Debouncer 3. An edit notification for F is received before typechecking has a chance to run which undoes the previous edit 4. The debouncer publishes the empty set of diagnostics after waiting 0.1s 5. GetFileContents runs and since the contents of the file haven't changed since the last time it ran, early cutoff skips everything donwstream Since TypeCheck is skipped, the empty set of diagnostics stays published until another edit comes. The goal of this change is to prevent setStageDiagnostics from losing diagnostics from other stages. To achieve this, we recover the old diagnostics for all stages and merge them with the new stage. * Fix hlint * Use Map.insert for clarity * Fix redundant imports * Fix "code actions after edit" experiment"

These tests were underspecified and broke with the recent improvements to ghcide diagnostics in haskell/ghcide#959 and included in this merge. Fixed by waiting specifically for the typecheck diagnostics and by being less prescriptive in the number and order of code actions

* Preventively switch to uninterruptible mask in withMVar' withMVar' is used to update the shakeSession var and it's crucial that the third argument is not interrupted. 'mask' can still be interrupted for I/O actions and, while we were careful to ensure none was used, if it ever breaks it will lead to very hard to debug problems. * refactor: move to RuleTypes * Add a TestRequest to wait for arbitrary ide actions Closes haskell/ghcide#955 * expectCurrentDiagnostics * Add a test suite for cancellation * Introduce --test-no-kick to fix cancellation tests reliability * delete unsafeClearDiagnostics (unused) * GetModSummaryWithoutTimestamps - remove StringBuffer Since the contents of the buffer are not tracked by the fingerprint. * Fix diagnostics bug Given a FOI F with non null typechecking diagnostics D, imagine the following scenario: 1. An edit notification for F is received, creating a new version 2. GetModTime is executed, producing 0 diagnostics. 2.1 updateFileDiagnostics is called 2.2 setStageDiagnostics is called 2.3 LSP.updateDiagnostics is called with a new version, resetting all the diagnostics for F 2.4 newDiags=[] in updateFileDiagnostics, which is different from D (the last published diagnostics), which enqueues a new publishDiagnostics [] in the Debouncer 3. An edit notification for F is received before typechecking has a chance to run which undoes the previous edit 4. The debouncer publishes the empty set of diagnostics after waiting 0.1s 5. GetFileContents runs and since the contents of the file haven't changed since the last time it ran, early cutoff skips everything donwstream Since TypeCheck is skipped, the empty set of diagnostics stays published until another edit comes. The goal of this change is to prevent setStageDiagnostics from losing diagnostics from other stages. To achieve this, we recover the old diagnostics for all stages and merge them with the new stage. * Fix hlint * Use Map.insert for clarity * Fix redundant imports * Fix "code actions after edit" experiment"

These tests were underspecified and broke with the recent improvements to ghcide diagnostics in haskell/ghcide#959 and included in this merge. Fixed by waiting specifically for the typecheck diagnostics and by being less prescriptive in the number and order of code actions

pepeiborra added 9 commits December 20, 2020 14:50

refactor: move to RuleTypes

a5b96a8

Add a TestRequest to wait for arbitrary ide actions

f1f00b4

Closes #955

expectCurrentDiagnostics

3ab366d

Add a test suite for cancellation

34832be

Introduce --test-no-kick to fix cancellation tests reliability

0b9d0ca

delete unsafeClearDiagnostics (unused)

199d9e5

GetModSummaryWithoutTimestamps - remove StringBuffer

6588302

Since the contents of the buffer are not tracked by the fingerprint.

pepeiborra force-pushed the fix-cancellation-clean branch from cfe62e1 to f7f0bfb Compare December 20, 2020 15:12

wz1000 reviewed Dec 20, 2020

View reviewed changes

src/Development/IDE/Core/Rules.hs Show resolved Hide resolved

wz1000 reviewed Dec 20, 2020

View reviewed changes

src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved

wz1000 approved these changes Dec 20, 2020

View reviewed changes

pepeiborra added 2 commits December 20, 2020 17:07

Fix hlint

7936e66

Use Map.insert for clarity

aad22e6

Fix redundant imports

b467e37

jneira approved these changes Dec 20, 2020

View reviewed changes

Fix "code actions after edit" experiment"

d72b032

pepeiborra merged commit 0d4e3b9 into master Dec 21, 2020

pepeiborra deleted the fix-cancellation-clean branch December 21, 2020 06:06

This was referenced Jan 8, 2021

Sticky diagnostics haskell/haskell-language-server#1171

Closed

Fix sticky diagnostics haskell/haskell-language-server#1188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix diagnostics update bug #959

Fix diagnostics update bug #959

pepeiborra commented Dec 20, 2020

wz1000 commented Dec 20, 2020

pepeiborra commented Dec 20, 2020 •

edited

Loading

jneira left a comment

tomsmeding commented Dec 20, 2020

Fix diagnostics update bug #959

Fix diagnostics update bug #959

Conversation

pepeiborra commented Dec 20, 2020

wz1000 commented Dec 20, 2020

pepeiborra commented Dec 20, 2020 • edited Loading

jneira left a comment

Choose a reason for hiding this comment

tomsmeding commented Dec 20, 2020

pepeiborra commented Dec 20, 2020 •

edited

Loading