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

refactor: decouple diagnostics from handlers into a hook #714

Merged
merged 4 commits into from
Nov 17, 2021

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Nov 17, 2021

Background

Originally we would only publish diagnostics directly from the relevant (RPC request) handlers, such as from textDocument/didOpen, textDocument/didChange or textDocument/didClose.

This in principle introduced the requirement on the task scheduler to support blocking/synchronous execution as we didn't want to be publishing stale diagnostics before the parsing and decoding is actually finished. However, this execution combined with the amount of operations we need to run in different contexts makes this a complex implementation hard to reason about and harder to maintain. Consequently there is at least one bug 🐛 where duplicate operations are attempted to be scheduled and one of them will remain blocking forever, effectively causing a deadlock 🙀 . This bug was first discovered in #700 which adds more operations to ensure cross-module references are discovered and therefore makes the deadlock situation more likely to occur.

This PR therefore aims to remove the whole concept of blocking/sync execution and assume that all operations will be executed asynchronously.

UX Implications

This comes with a (subtle?) UX difference where we would publish diagnostics for all files as they are indexed (and changed), not just the ones which are open on the client. We could still provide the existing UX if we wanted to, but there's no clear benefit we'd gain by the additional complexity.

Also we now avoid publishing any diagnostics for non-autoloaded tfvars files, which more or less aligns the behaviour with gopls ' treatment of Go build tags. I will create a separate issue to track this problem.

See #715

LSP

There is currently no clear guidance in LSP spec around whether servers should publish diagnostics for unopened files. The only related part of the spec talks about clearing of diagnostics on close:

Diagnostics are “owned” by the server so it is the server’s responsibility to clear them if necessary. The following rule is used for VS Code servers that generate diagnostics:

  • if a language is single file only (for example HTML) then diagnostics are cleared by the server when the file is closed. Please note that open / close events don’t necessarily reflect what the user sees in the user interface. These events are ownership events. So with the current version of the specification it is possible that problems are not cleared although the file is not visible in the user interface since the client has not closed the file yet.
  • if a language has a project system (for example C#) diagnostics are not cleared when a file closes. When a project is opened all diagnostics for all files are recomputed (or read from a cache).

When a file changes it is the server’s responsibility to re-compute diagnostics and push them to the client. If the computed set is empty it has to push the empty array to clear former diagnostics. Newly pushed diagnostics always replace previously pushed diagnostics. There is no merging that happens on the client side.

Language Clients & gopls

Although LSP doesn't provide clear guidance, it is clear that sending diagnostics for unopened files is a common practice in gopls. Clients account for this possibility differently, probably catering different user bases with different expectations:

  • Sublime Text LSP will hide any diagnostics which are not for unopened files
  • VS Code will display all diagnostics

Future TODOs

Versioned diagnostics

LSP allows us to send diagnostics with document versions, which presumably aids the client as it can just drop the ones which are associated with an old version of the document and effectively prevent ever publishing a stale diagnostic.

Attaching the document version at this point would not be trivial as we'd probably need to start tracking document versions in memdb alongside diagnostics. This PR therefore does increase the likelihood of us publishing outdated diagnostics, but they would always get updated/cleared as part of another module change hook execution anyway, so it's rather a "temporary blip".

See #716

Only publishing changed diagnostics

In an ideal scenario we would only publish diagnostics when there are different ones to publish and avoid publishing the same ones multiple times. In order to achieve this however we would need to check whether all diagnostics are equal. Currently we use the upstream hcl.Diagnostic to represent a diagnostic and the upstream implementation does not have an equality function itself. We could implement all this relatively easily downstream, but we can't actually take advantage of it until we figure out a better way of publishing and clearing validate diagnostics, which currently get just cleared whenever other diagnostics are published, to avoid stale diagnostics.

See #717

When a (1) capped channel has more than one consumer, one of them will
effectively remain blocking as the other consumes the message from
the channel. Closing the channel unblocks any/all consumers.

Here we also unpublish previously public ModuleOperation.Done()
to further prevent deadlocks in the future, i.e. effectively
to prevent any place that schedules tasks to wait.

The only reason we keep the channel there for now is to make testing
*within the package* easier.
@rchl
Copy link

rchl commented Nov 17, 2021

* Sublime Text LSP will hide any diagnostics which are not for unopened files

This is no longer true (fixed by sublimelsp/LSP#1881).

ST LSP still hides diagnostics for files that are outside of the workspace but that's also not entirely wanted so might change in the future.

@radeksimko radeksimko requested a review from a team November 17, 2021 12:42
@radeksimko
Copy link
Member Author

@rchl Thanks for the note, that further supports the theory that people do want to see diagnostics even for unopened files then 👍🏻

Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work 🌟 The diagnostics have improved and there is even less code now

CleanShot 2021-11-17 at 14 20 38@2x

There is one strange behavior I need to show you, please don't merge yet

Previously we would (mistakenly) not parse module in walker,
only when it's opened (as part of didOpen or didChange).

This fixes that to help publish diagnostics from all modules
at startup time.
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7611112 fixes the before mentioned strange behavior 🎉

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement New feature or request textDocument/publishDiagnostics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants