Skip to content
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

Wingman: Fix subsequent tactics #2479

Closed
wants to merge 1 commit into from
Closed

Conversation

isovector
Copy link
Collaborator

@isovector isovector commented Dec 13, 2021

Fixes #2448

@isovector
Copy link
Collaborator Author

This new test should pass if the bug is fixed.

@pepeiborra I added the changes you suggested in #2448 (comment), but they don't seem to fix the problem. How did you test it?

@pepeiborra
Copy link
Collaborator

Maybe I just got lucky :( - using the repro, the fix worked once. But I didn't double check, and it seems the problem is non deterministic.

@pepeiborra
Copy link
Collaborator

One super useful tool to debug this is Shake profiling. Enable it using the flag --shake-profiling=/tmp/hls or the env var GHCIDE_BUILD_PROFILING=/tmp/hls and you will get one html report for every build, telling you:

  1. Which keys were marked as dirty before the build
  2. Which keys were actually built
  3. Which keys were visited but not built because the build system thought they were clean (no dependencies changed)

@pepeiborra
Copy link
Collaborator

FYI your test cannot pass, I just found a bug in lsp-test handling of commands that apply edits: it sends the TextDocumentDidChange notification before updating the version number in the VFS, which means that the IDE doesn't "see" the edit at all:

https://github.com/pepeiborra/lsp/blob/4df0a86e813a267add82a28f4a0c41b5aaacc85b/lsp-test/src/Language/LSP/Test/Session.hs#L373-L385

pepeiborra added a commit that referenced this pull request Dec 18, 2021
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
pepeiborra added a commit that referenced this pull request Dec 18, 2021
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
pepeiborra added a commit that referenced this pull request Dec 18, 2021
Grab and test haskell/lsp#376 as we want to include this bug fix for #2479
@pepeiborra
Copy link
Collaborator

The test passes for me locally after fixing the bug in lsp-test (haskell/lsp#376) which suggests that it doesn't quite capture the bug identified in #2448

My rebase: #2509

@pepeiborra
Copy link
Collaborator

I have been looking at this and I believe the issue is in this code:

data FileContext = FileContext
{ fc_uri :: Uri
, fc_nfp :: NormalizedFilePath
, fc_range :: Maybe (Tracked 'Current Range)
-- ^ For code actions, this is 'Just'. For code lenses, you'll get
-- a 'Nothing' in the request, and a 'Just' in the response.
}
deriving stock (Eq, Ord, Show, Generic)
deriving anyclass (A.ToJSON, A.FromJSON)
deriving anyclass instance A.ToJSON NormalizedFilePath
deriving anyclass instance A.ToJSON NormalizedUri
deriving anyclass instance A.FromJSON NormalizedFilePath
deriving anyclass instance A.FromJSON NormalizedUri

Embedding NormalizedFilePath values in LSP messages is unsupported and leads to unspecified behaviour due to the embedded hash. I don't remember the details, but see e.g. #1202

Prior to 682386d this unspecified behaviour caused no other observable issues than probably space leaks. Now it additionally creates stale values than never get refreshed.

@isovector
Copy link
Collaborator Author

Very odd. Thanks for continuing to dig in here, @pepeiborra.

@isovector
Copy link
Collaborator Author

Subsumed by #2580

@isovector isovector closed this Jan 12, 2022
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.

Wingman: TODO(sandy) error pops up after the first tactic
2 participants