Theia integration for clangd-built-in "clang-tidy" linter

[lmcbout]

Signed-off-by: Jacques Bouthillier jacques.bouthillier@ericsson.com
Fix issue #4579

Add preferences to handle clang-tidy functionality when clangd v9+ is used.
This is a new linter for C/C++. There are two ways to initiate the linter:

• From the preferences
• Adding a new file located in the same folder of the files or a parent folder: .clang-tidy

[benoitf]

hello @akosyakov do you know why gitpod status is red (I noticed it on some others PR ) ?

[marcdumais-work]

@lmcbout: please update the commit message to reflect content of commit

update: PR description as well please (can be the same)

[vince-fugnitto]

Looks good to me and works on my end.

@thegecko are you fine with the changes?
I suggested making the new clangTidy CppStartParameters optional as to not break any extenders.

[marcdumais-work]

A couple of things I have noticed, that I think are on the clangd/clang-tidy side:

• suppressing diagnostics does not seem to work. See here for details how to use them. As far as I can tell, the suppression comments are being ignored. (note: it's possible that the clang-tidy built-in with clangd is not the same version as the one documented in the page above?)
• Contrary to most Language Servers I have seen so far, clangd does not clear "Problem Markers"/"diagnostics" for a file, after its editor is closed. The Problems markers known to Theia remain the same. This means that these diagnostics, for closed files, will be dangling; they will not be updated until the file is re-opened in a Theia editor. If one uses an alternate editor to update an un-opened file, updated diagnostics will not be provided to Theia until the file is re-opened in a Theia editor.
• (maybe) We have not yet found a way to configure clangd-bundled clang-tidy checks to generate errors instead of warning-type diagnostics. There seems to be a way to do this with slandalone clang-tidy, but we have to confirm this does what we want. Then check if this can work from clangd.

None of these is a show-stopper, I think.

[marcdumais-work]

One issue more on the Theia side: ATM we do not watch the .clang-tidy file(s), so we do not react when they are modified, added or deleted. Since there can be many of those files in different project folders, it may not be easy to track them all.

But it seems we should at least monitor for the potential one in the project's root folder? Or maybe there would be a simple way to find and watch-for all/any such file in the project?

[thegecko]

I don't believe these changes affect us. We are only now considering clangd 8, let alone 9 :)

@arekzaluski ?

[arekzaluski]

That is correct. At the moment we are using release 7.0.0 and are waiting for the official release of clangd 8 for all 3 platforms (Linux, Mac, Windows). It is delayed but should be available soon.
It is however good to see features from clangd 9 appearing in Theia. My only suggestion would be to allow also to override the location of .clang-tidy file. It may be sensible to search for this file in the same location as compile_commands.json.

[paul-marechal]

My only suggestion would be to allow also to override the location of .clang-tidy file. It may be sensible to search for this file in the same location as compile_commands.json.

This seems like logic from clang-tidy itself: https://clang.llvm.org/extra/clang-tidy/

Configuration files:
  clang-tidy attempts to read configuration for each source file from a
  .clang-tidy file located in the closest parent directory of the source
  file. If any configuration options have a corresponding command-line
  option, command-line option takes precedence. The effective
  configuration can be inspected using -dump-config

Linting sounds to be more source-specific rather than build-specific, unlike compile_commands.json, just my thoughts (as I am not a strong C/C++ developer).

[marcdumais-work]

• Contrary to most Language Servers I have seen so far, clangd does not clear "Problem Markers"/"diagnostics" for a file, after its editor is closed.

I have confirmed that the TypeScript LS does this as I expect, using yesterday's Theia master branch: upon closing an editor, the diagnostics for that file are cleared.

@lmcbout could you troubleshoot, maybe with the debugger or by enabling cpp LS traces?

• confirm if clangd sends an empty diagnostics update message upon an editor being closed?
• if so, why is it not handled in Theia, so that the markers for that file are cleared?

[marcdumais-work]

ATM we do not watch the .clang-tidy file(s), so we do not react when they are modified, added or deleted.

In any case, for a first version, it could be good enough to document that changing the clang-tidy config file(s) requires a client restart (that will trigger a LS restart, with the new instance reading the updated configuration).

[paul-marechal]

requires a client restart (that will trigger a LS restart [...]

I prefer using F1 > C/C++: Restart Language Server, but result is the same :)

[vince-fugnitto]

ATM we do not watch the .clang-tidy file(s), so we do not react when they are modified, added or deleted.

In any case, for a first version, it could be good enough to document that changing the clang-tidy config file(s) requires a client restart (that will trigger a LS restart, with the new instance reading the updated configuration).

It might not even be necessary to do a complete client restart, we have commands to restart language servers. In the case of C/C++ we have the command C/C++: Restart Language Server.

[HighCommander4]

• Contrary to most Language Servers I have seen so far, clangd does not clear "Problem Markers"/"diagnostics" for a file, after its editor is closed.

I have confirmed that the TypeScript LS does this as I expect, using yesterday's Theia master branch: upon closing an editor, the diagnostics for that file are cleared.

Here's what the LSP spec says about textDocument/publishDiagnostics:

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.
• 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).

So, it sounds like the behaviour is up to the server, but they have documented the choice made by the VSCode server. Perhaps the behaviour you observed was because TypeScript is treated as the first category ("single file only" language)?

[HighCommander4]

Regarding this use case:

This means that these diagnostics, for closed files, will be dangling; they will not be updated until the file is re-opened in a Theia editor. If one uses an alternate editor to update an un-opened file, updated diagnostics will not be provided to Theia until the file is re-opened in a Theia editor.

I think this can be handled in a different way.

It looks like Theia (and probably language clients in general) "watch" files in the project, and send a workspace/didChangeWatchedFiles notification if a file is changed, even if the change happens via a different editor.

Clangd, in turn, listens to this notification, and sends updated diagnostics -- but only if the file is open in the language client. (So, if you fix an error in vim while a file is open in Theia, the error automatically goes away in Theia.)

So, perhaps this use case can be addressed by modifying clangd to always send updated diagnostics in response to workspace/didChangeWatchedFiles, even if the file is not open in the language client?

I think the resulting user experience would be better, than if the server cleared the diagnostics on file-close. (Because this way, the user still sees that are problems in the project, even if the files that have the problems are closed.)

[paul-marechal]

I think the resulting user experience would be better, than if the server cleared the diagnostics on file-close. (Because this way, the user still sees that are problems in the project, even if the files that have the problems are closed.)

IIUC, it would also cover the case where updating one file fixes an issue (or creates a new one) in another file which might not be opened anymore? If so it would be nice indeed.

But that's on clangd, nothing that could be addressed here right?

[marcdumais-work]

But that's on clangd, nothing that could be addressed here right?

That's my understanding too: I tested and Theia (Theia LS client?) already sends workspace/didChangeWatchedFiles to clangd, when a C/C++ file (at least .c, .cpp and .h) is modified/created/deleted, even outside Theia:

root ERROR C/C++: I[10:42:24.156] <-- workspace/didChangeWatchedFiles

[marcdumais-work]

Hi @HighCommander4 !

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.
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).

Thanks for this relevant reference. It sounds to me like clangd is currently somewhere in the middle, but behaves closer to the "single file" case: it's not taking "full charge" of the project, building it and sending project-wide diagnostics, like jdt.ls does.

It's my understanding that to get the full list of diagnostics for a C/C++ project, we should trigger a project build and parse the output, creating ProblemMarkers for any error/warning found. IoW we should not rely on clangd to do a full-project analysis for us, correct?

If clangd is going to evolve towards the "project system" case, could it make sense for it to send diagnostics whenever any file is analysed, e.g. if a .cpp file is opened, probably all included .h files will also be analysed? Would it make sense to send diagnostics updates for these files also, even if never opened and not modified?

Perhaps the behaviour you observed was because TypeScript is treated as the first category ("single file only" language)?

+1.

[HighCommander4]

It looks like Theia (and probably language clients in general) "watch" files in the project, and send a workspace/didChangeWatchedFiles notification if a file is changed, even if the change happens via a different editor.

Clangd, in turn, listens to this notification, and sends updated diagnostics -- but only if the file is open in the language client. (So, if you fix an error in vim while a file is open in Theia, the error automatically goes away in Theia.)

After some further experimentation, it looks like I was mistaken about this. It appeared from the LSP traffic that the server was responding to the workspace/didChangeWatchedFiles notification, but in fact it was responding to the textDocument/didChange notification, which Theia also sends, even for changes made in other editors, but only for files which are open in Theia. Clangd currently ignores workspace/didChangeWatchedFiles.

This explains why it was only sending updated diagnostics for files which are open in Theia.

[HighCommander4]

I think the resulting user experience would be better, than if the server cleared the diagnostics on file-close. (Because this way, the user still sees that are problems in the project, even if the files that have the problems are closed.)

IIUC, it would also cover the case where updating one file fixes an issue (or creates a new one) in another file which might not be opened anymore? If so it would be nice indeed.

Based on the above observations, clangd does not currently do this. I agree it would be nice if it did.

[HighCommander4]

It's my understanding that to get the full list of diagnostics for a C/C++ project, we should trigger a project build and parse the output, creating ProblemMarkers for any error/warning found. IoW we should not rely on clangd to do a full-project analysis for us, correct?

That's my understanding too.

If clangd is going to evolve towards the "project system" case, could it make sense for it to send diagnostics whenever any file is analysed, e.g. if a .cpp file is opened, probably all included .h files will also be analysed? Would it make sense to send diagnostics updates for these files also, even if never opened and not modified?

I think that would make sense, and I believe this is actually in the works, based on this recent review request.

[HighCommander4]

To summarize the discussion of diagnostics for files which are closed, it seems to me we have the following options on the clangd side:

1. Have clangd clear diagnostics when a file is closed, as Marc originally suggested.
2. Have clangd react to workspace/didChangeWatchedFiles and update diagnostics for the files that changed.

(1) is probably slightly easier to implement. (2) would, I think, lead to a better user experience as discussed above.

We could also do (1) as an interim measure while we work on (2).

[HighCommander4]

• suppressing diagnostics does not seem to work. See here for details how to use them. As far as I can tell, the suppression comments are being ignored. (note: it's possible that the clang-tidy built-in with clangd is not the same version as the one documented in the page above?)

I investigated this a bit. The handling of the suppression comments in clang-tidy is handled by a component called the DiagnosticConsumer, which is also responsible for presenting the diagnostics (e.g. displaying them on the command-line).

Clangd uses its own DiagnosticConsumer, as it needs different "presentation" behavior (it will send the diagnostics over the network rather than display them on the command-line), but as a side effect the logic to handle suppression comments is skipped as well.

• (maybe) We have not yet found a way to configure clangd-bundled clang-tidy checks to generate errors instead of warning-type diagnostics. There seems to be a way to do this with slandalone clang-tidy, but we have to confirm this does what we want. Then check if this can work from clangd.

With standalone clang-tidy, this can be done using the WarningsAsErrors option in the .clang-tidy config file, which allows listing the checks which should be treated as errors.

However, the logic for applying this setting is again in the DiagnosticConsumer, and so will not happen with clangd.

[HighCommander4]

I filed some clangd issues for topics discussed above:

• stale diagnostics
• suppression comments and warnings-as-errors

[marcdumais-work]

I filed some clangd issues for topics discussed above:

• stale diagnostics
• suppression comments and warnings-as-errors

Thanks @HighCommander4 for the feedback and opening those issues!

[lmcbout]

Code tested with latest master and works fine, so merged it now.