[clangd] DidChangeConfiguration Notification

[simark]

Summary

Implementation of DidChangeConfiguration notification handling in clangd.
This currently only supports changing one setting: the path of the compilation database to be used for the current project
In other words, it is no longer necessary to restart clangd with a different command line argument in order to change
the compilation database.

[malaperle]

clangd/ClangdLSPServer.cpp

51 ↗

(On Diff #121388)

Are you sure the existing tests don't fail? usually when we change this, some tests have to be adjusted.

199 ↗

(On Diff #121388)

I'm thinking, maybe it's better not to have the map and just pass along the ClangdConfigurationParams to the server (instead of the map). I think ClangdConfigurationParams is more useful as a structure than a "flat" map with all the keys being at the same level. In ClangdConfigurationParams, we'll be able to add sub-configurations sections (index, code-completion, more?) which is well suited to reflect the JSON format.

Unless perhaps you had another use case for the map that I'm not thinking about?

201 ↗

(On Diff #121388)

Won't getValue crash if compilationDatabasePath was not set since it's optional?

203 ↗

(On Diff #121388)

Why is this line needed? Maybe there should be a comment?
I think its meant to reload the actual CompilationDatabase object? I think you could call this in setCompileCommandsDir and not have to make getCompilationDatabase public. You also woudn't need getCompileCommandsDir at all.

208 ↗

(On Diff #121388)

Pass Params.settings? see previous comment.

clangd/ClangdServer.cpp

441 ↗

(On Diff #121388)

I think we need to discuss what should happen when the compilation database changes. Since this is mainly for switching "configuration", I think it should invalidate all previously parsed units. Otherwise, if the user has a file open in the editor, it will report diagnostics based on the old configuration and similarly code completion will be outdated (until Clangd is restarted?). Would it be unreasonable to reparse all units in memory? Perhaps ClangdServer::forceReparse would be useful for that?
I'm not sure what is the right approach but we should make sure the editors are not left in a confusing state.

clangd/GlobalCompilationDatabase.h

55 ↗

(On Diff #121388)

remove?

56 ↗

(On Diff #121388)

call getCompilationDatabase from here?

58 ↗

(On Diff #121388)

move back to private?

clangd/Protocol.cpp

534 ↗

(On Diff #121388)

I think this needs a test, perhaps create did-change-configuration.test? It can also test the ClangdConfigurationParams::parse code
If you do, I think it's a good idea to test a few failing cases:

• "settings" is not a mapping node. You can test this with a scalar node like "settings": ""
• Something else than "settings" is present, so that it goes through logIgnoredField
• "settings" is not set at all. In this case, because it's mandatory in the protocol, return llvm::None. This can be checked after the loop after all key/values were read.

There are similar tests in execute-command.test if you'd like an example.
And of course also add a "successful" case as well :)

561 ↗

(On Diff #121388)

If "settings" was not set in the end, return llvm::None, because this is mandatory in the protocol.

578 ↗

(On Diff #121388)

Here there should be a test when "compilationDatabasePath" is not a scalar node. You can test this with a scalar node like "compilationDatabasePath": {}

582 ↗

(On Diff #121388)

else {

  logIgnoredField(KeyValue, Logger);
}

?
Plus in the test this should be easy to cover.
settings {

"customField": ""

}

clangd/Protocol.h

276 ↗

(On Diff #121388)

I think there should to be comment to mention that this is a Clangd extension.

285 ↗

(On Diff #121388)

I don't think you need this constructor?

287 ↗

(On Diff #121388)

Maybe remove this to make it more consistent with the other interfaces/struct. We usually just assign to fields after the struct is instantiated, not in the constructor.

290 ↗

(On Diff #121388)

I think there should to be comment to mention that this is a Clangd extension.
Here, it would also explain that the protocol specifies "any" and that using a predefined ClangdConfigurationParams struct is both easier to use and safer.

[ilya-biryukov]

I don't think we should pass the very general configuration map to ClangdServer. Especially given that we can easily update DirectoryBaseCompilationDatabase instance in ClangdLSPServer itself.
What are your thoughts on this?

clangd/ClangdLSPServer.cpp

199 ↗

(On Diff #121388)

I don't think ClangdServer should handle changes to compilation database path at all.
It accepts GlobalCompilationDatabase as a parameter and does not own it, so ClangdLSPServer can mutate it properly.

clangd/ClangdServer.h

288 ↗

(On Diff #121388)

NIT: Add full stop at the end of comments.

289 ↗

(On Diff #121388)

This function is way too general for ClangdServer's interface, can we have more fine-grained settings here (i.e. setCompletionParameters etc?) and handle the "general" case in ClangdLSPServer (i.e., unknown setting names, invalid setting parameters, json parsing, etc.)?

I suggest we remove this function altogether.

[Nebiroth]

clangd/ClangdLSPServer.cpp

51 ↗

(On Diff #121388)

I'll verify this when I will have the time.

clangd/ClangdServer.h

289 ↗

(On Diff #121388)

So if I understand correctly, we would have the most generic workspace/didChangeConfiguration handler located in ClangdLSPServer
with a name perhaps similar to changeConfiguration that would pass valid settings to more specific functions inside ClangdServer ?

clangd/Protocol.cpp

534 ↗

(On Diff #121388)

Yes, I was planning on adding a simple test in the coming days. I imagine this test will be greatly expanded on once more settings become available to change on the server side.

[ilya-biryukov]

clangd/ClangdServer.h

289 ↗

(On Diff #121388)

Exactly!

[Nebiroth]

clangd/Protocol.h

285 ↗

(On Diff #121388)

I do inside parse() for DidChangeConfigurationParams.

[Nebiroth]

Added test for didChangeConfiguration notification.
Compilation database and extra file flags are now properly reloaded when changing database path.
ClangdConfigurationParams now used instead of map.
changeConfiguration removed from ClangdServer. This class will now handle more specific settings.
Added more checks when parsing notification from client..

[ilya-biryukov]

clangd/ClangdServer.cpp

580 ↗

(On Diff #123069)

Could we have a method in DraftStore that returns all active files in a vector<Path> instead?
The reason is that DraftStore has a thread-safe API and adding getDrafts() to it totally breaks the contract.

581 ↗

(On Diff #123069)

NIT: single-statement blocks don't need {}

clangd/ClangdServer.h

310 ↗

(On Diff #123069)

NIT: description of what function does seems more useful than a particular use-case. Maybe swap the two sentences in the comments?

Also, DraftMgr is an implementation detail. Let's simply mentioned "all currently opened files" instead of the DraftMgr.
And let's add a note that this method may be really expensive.

312 ↗

(On Diff #123069)

Maybe place declaration of reparseOpenedFiles right after the declaration of forceReparse?
There close relation to each other is obvious, so it feels they should live side-by-side.

312 ↗

(On Diff #123069)

forceReparse() exposes a future to wait to its completion. We should probably do the same in reparseOpenedFiles.
The problem there is that it's probably impossible to expose using a single future with the API available in the STL (we want when_all for that).
Maybe return a vector of futures instead?

clangd/DraftStore.h

44 ↗

(On Diff #123069)

We shouldn't expose this in the interface, as it breaks the thread-safety of DraftStore.

clangd/GlobalCompilationDatabase.cpp

108 ↗

(On Diff #123069)

Accidental change?

clangd/GlobalCompilationDatabase.h

57 ↗

(On Diff #123069)

NIT: We don't need to call StringRef explicitly constructor here.

57 ↗

(On Diff #123069)

Why do we need to call getCompilationDatabase here? Why not simply set the CompileCommandsDir and clear the CompilationDatabases cache?

64 ↗

(On Diff #123069)

Accidental change? Why do we need to swap the order of these functions?

clangd/Protocol.h

295 ↗

(On Diff #123069)

Maybe call it ClangdConfigurationParamsChange to make it clear those are diffs, not the actual params?

300 ↗

(On Diff #123069)

Why do we overload operator!? Can't we use llvm::Optional<ClangdConfigurationParams> where appropriate instead?

test/clangd/initialize-params-invalid.test

1 ↗

(On Diff #123069)

Accidental newline?

22 ↗

(On Diff #123069)

NIT: the indent seems off by one character

test/clangd/initialize-params.test

1 ↗

(On Diff #123069)

Accidental newline?

22 ↗

(On Diff #123069)

NIT: the indent seems off by one character

[malaperle]

clangd/Protocol.h

295 ↗

(On Diff #123069)

The idea was that we can reuse the same struct for InitializeParams.initializationOptions

[ilya-biryukov]

clangd/Protocol.h

295 ↗

(On Diff #123069)

Since InitializeParams.initializationOptions may also have unset values (llvm::None), it also seems fine to treat those as a "diff" between the default parameters and the new ones.
The reasoning behind naming for me is that if we allow only a subset of fields to be set and use the ones that were set override the corresponding values, it really feels like an entity describing a change to the configuration parameters, not the parameters themselves.

I don't have a strong opinion on this one, though. If you'd prefer to keep the current name, it's totally fine with me.

[malaperle]

clangd/Protocol.h

295 ↗

(On Diff #123069)

That makes sense the way you explained it. I think ClangdConfigurationParamsChange is good.

[Nebiroth]

Fixed DraftStore thread-safe API being broken
Removed superfluous getCompilationDatabase call
Changed name of struct to ClangDConfigurationParamsChange
Removed operator ! overload for struct
Minor code cleanup

[Nebiroth]

clangd/GlobalCompilationDatabase.cpp

108 ↗

(On Diff #123069)

Twine(File) and "in overriden directory" did not have a space to separate otherwise.

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

250 ↗

(On Diff #124024)

Replace Settings instead of Params.settings here?
Or remove the Settings variable altogether.

clangd/ClangdServer.cpp

579 ↗

(On Diff #124024)

NIT: remote empty line

581 ↗

(On Diff #124024)

DonePromise and DoneFuture are never used. Remove them?

587 ↗

(On Diff #124024)

Use "const auto&" or StringRef instead to avoid copies?

clangd/ClangdServer.h

244 ↗

(On Diff #124024)

NIT: remove this empty line

clangd/GlobalCompilationDatabase.cpp

108 ↗

(On Diff #123069)

Right. Sorry, my mistake.

clangd/Protocol.h

294 ↗

(On Diff #124024)

NIT: remote empty line

296 ↗

(On Diff #124024)

NIT: remote empty line

303 ↗

(On Diff #124024)

NIT: remove empty line

304 ↗

(On Diff #124024)

NIT: Use = default instead

305 ↗

(On Diff #124024)

We need to initialize settings field with settings parameter.

Maybe even better to remove both constructors to align with other classes in this file?

test/clangd/did-change-configuration.test

15 ↗

(On Diff #124024)

Do we want to add a # CHECK: for that output?

33 ↗

(On Diff #124024)

clangd won't see this file. didOpen only sets contents for diagnostics, not any other features.
You would rather want to add more # RUN: directives at the top of the file to create compile_commands.json, etc.

Writing it under root ('/') is obviously not an option. Lit tests allow you to use temporary paths, this is probably an approach you could take. See lit docs for more details.

test/clangd/initialize-params-invalid.test

2 ↗

(On Diff #124024)

I am still seeing this newline

[Nebiroth]

test/clangd/did-change-configuration.test

33 ↗

(On Diff #124024)

Are there examples available on how to use this? I have to use a # RUN: to create a file and then use it's path in a workspace/didChangeConfiguration notification?

[Nebiroth]

Merged with latest llvm + clang
Minor code cleanup

[Nebiroth]

Removed some more empty lines

[ilya-biryukov]

It seems this patch is out of date, we need to merge it with the latests head.

clangd/DraftStore.cpp

45 ↗

(On Diff #126449)

Why do we need this assert? I don't see how empty file is worse than any other invalid value.

55 ↗

(On Diff #126449)

Why do we need this assert? I don't see how empty file is worse than any other invalid value.

clangd/GlobalCompilationDatabase.h

65 ↗

(On Diff #126449)

We need to lock the Mutex here!

clangd/Protocol.cpp

368 ↗

(On Diff #126449)

NIT: maybe remove this empty line?

test/clangd/did-change-configuration.test

33 ↗

(On Diff #124024)

After thinking about it, I really don't see an easy way to test it currently. We just don't have the infrastructure in place for that.
I think it's fine to add a FIXME to the body of ClangdLSPServer::onChangeConfiguration that we need to test it and remove this test.

[Nebiroth]

Removed test file
Added mutex lock when changing CDB
Minor code cleanup

[simark]

I just got familiar with the patch, I cleaned up the bits that I thought were
unnecessary for this change. For example, we don't need a toJSON function for
DidChangeConfigurationParams, or explicit constructors to
DidChangeConfigurationParams, etc.

[simark]

clangd/ClangdLSPServer.cpp

78 ↗

(On Diff #131051)

I find configurationChangeProvider a bit weird. It makes it sound like clangd can provide configuration changes. In reality, it can accept configuration changes. So I think this should be named something else.

[Nebiroth]

clangd/ClangdLSPServer.cpp

78 ↗

(On Diff #131051)

Agreed, perhaps configurationChangeManager would be more appropriate then?

[simark]

clangd/ClangdLSPServer.cpp

78 ↗

(On Diff #131051)

I'm thinking of removing it for the time being. Since it's not defined in the protocol what types of configuration changes exist (it's specific to each language server), it not very useful to simply advertise that we support configuration changes. We would need to advertise that we support compilation database changes in particular. I think this can be done later.

[simark]

Fix formatting, remove capability

• Fix some formatting nits
• Remove the entry in the capabilities object

[ilya-biryukov]

Hi @simark , thanks for picking up this change.

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

Are you planning to to address this FIXME before checking the code in?

clangd/GlobalCompilationDatabase.h

64 ↗

(On Diff #131131)

Maybe move definition to .cpp file?

[simark]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

Following what you said here:

https://reviews.llvm.org/D39571?id=124024#inline-359345

I have not really looked into what was wrong with the test, and what is missing in the infrastructure to make it work. But I assumed that the situation did not change since then. Can you enlighten me on what the problem was, and what is missing?

clangd/GlobalCompilationDatabase.h

64 ↗

(On Diff #131131)

Will do.

[simark]

Move implementation of setCompileCommandsDir to .cpp

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

We usually write unittests for that kind of thing, since they allow to plug an in-memory filesystem, but we only test ClangdServer (examples are in unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not allow to plug in a virtual filesystem (vfs). Even if we add vfs, it's still hard to unit-test because we'll have to match the json input/output directly.

This leaves us with an option of a lit test that runs clangd directly, similar to tests in test/clangd.
The lit test would need to create a temporary directory, create proper compile_commands.json there, then send the LSP commands with the path to the test to clangd.
One major complication is that in LSP we have to specify the size of each message, but in our case the size would change depending on created temp path. It means we'll have to patch the test input to setup proper paths and message sizes.
If we choose to go down this path, clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar setup (create temp-dir, patch up some configuration files to point into the temp directory, etc) and could be used as a starting point.

It's not impossible to write that test, it's just a bit involved. Having a test would be nice, though, to ensure we don't break this method while doing other things. Especially given that this functionality is not used anywhere in clangd.

[simark]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

We usually write unittests for that kind of thing, since they allow to plug an in-memory filesystem, but we only test ClangdServer (examples are in unittests/clangd/ClangdTests.cpp). ClangdLSPServer does not allow to plug in a virtual filesystem (vfs). Even if we add vfs, it's still hard to unit-test because we'll have to match the json input/output directly.

What do you mean by "we'll have to match the json input/output directly"? That we'll have to match the complete JSON output textually? Couldn't the test parse the JSON into some data structures, then we could assert specific things, like that this particular field is present and contains a certain substring, for example?

This leaves us with an option of a lit test that runs clangd directly, similar to tests in test/clangd.
The lit test would need to create a temporary directory, create proper compile_commands.json there, then send the LSP commands with the path to the test to clangd.
One major complication is that in LSP we have to specify the size of each message, but in our case the size would change depending on created temp path. It means we'll have to patch the test input to setup proper paths and message sizes.
If we choose to go down this path, clang-tools-extra/test/clang-tidy/vfsoverlay.cpp does a similar setup (create temp-dir, patch up some configuration files to point into the temp directory, etc) and could be used as a starting point.

Ok, I see the complication with the Content-Length. I am not familiar with lit yet, so I don't know what it is capable of. But being able to craft and send arbitrary LSP messages would certainly be helpful in the future for all kinds of black box test, so having a framework that allows to do this would be helpful, I think. I'm not familiar enough with the ecosystem to do this right now, but I'll keep it in mind.

One question about this particular test. Would there be some race condition here? If we do:

1. Start clangd with compile_commands.json #1
2. Ask for the definition of a function, expecting a result
3. Change the configuration to compile_commands.json #2
4. Ask for the definition of the same function, expecting a different result

Since clangd is multi-threaded and does work in the background, are we sure that we'll get the result we want in #4?

It's not impossible to write that test, it's just a bit involved. Having a test would be nice, though, to ensure we don't break this method while doing other things. Especially given that this functionality is not used anywhere in clangd.

I agree. For the time being, is it fine to leave the FIXME there? We can work on improving the test frameworks to get rid of it.

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

78 ↗

(On Diff #131051)

"configurationChangeProvider" is not in the LSP, right?
There's experimental field in the specification, let's put it under that field if you want to advertise that clangd supports this spec to your clients.

302 ↗

(On Diff #131131)

What do you mean by "we'll have to match the json input/output directly"? That we'll have to match the complete JSON output textually? Couldn't the test parse the JSON into some data structures, then we could assert specific things, like that this particular field is present and contains a certain substring, for example?

The interface to interact with ClangdLSPServer is JSONOutput, which only allows you to pass the output of requests to the stream at the moment. That means not only parsing json, but also finding the individual responses in the combined output.

One question about this particular test. Would there be some race condition here? If we do:

Technically clangd can do everything in parallel, but we have a flag -run-synchronously that will make it do all the work on the main thread and we use that in the tests.

LLVM has lots of tests that do substring matches, there's a special tool, called FileCheck, to make writing them simpler. See the test from my previous message (specifically the # CHECK: lines), they should be enough to get started.
Lit itself is a set of python scripts that allow, among other things, to specify directly in the test file which commands the test needs to run. Again, see the example tests (specifically, # RUN: clangd .... lines).

I agree. For the time being, is it fine to leave the FIXME there? We can work on improving the test frameworks to get rid of it.

FIXME looks fine for now, but make sure to test this code does really work for your use-case.

[simark]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

FIXME looks fine for now, but make sure to test this code does really work for your use-case.

So far I have just tested with some small hello-world projects, but I'll take the time to test with some bigger project (gdb).

[simark]

clangd/ClangdLSPServer.cpp

78 ↗

(On Diff #131051)

I've removed it for now, we can add it later. We should think about how to express what we support. It's not enough to say we support the didChangeConfiguration notification, we should also express what element of configuration we support (the location of the compile commands directory, in this case).

302 ↗

(On Diff #131131)

FIXME looks fine for now, but make sure to test this code does really work for your use-case.

I've tested many scenarios with a project of good size. It works as expected, but I've hit the assert in ClangdServer::forceReparse maybe twice. I don't know how to reproduce it. I think we can go ahead with the patch and maybe the problem will become clearer with time. At least even if this feature is not completely stable, it won't affect you if you don't use it.

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

78 ↗

(On Diff #131051)

SG. Let's not add extra stuff not in the LSP if the clients can live without it.

302 ↗

(On Diff #131131)

What was the assertion? "forceReparse called for non-added document"?

[simark]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

What was the assertion? "forceReparse called for non-added document"?

Exactly.

[simark]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

I am not sure how it can happen. The main thread calls DraftMgr.getActiveFiles(), which returns the keys of the Drafts map. These keys are then passed to forceReparse, which asserts that passed key is in Drafts. The only way for it not to be in Drafts would be if it was removed in the mean time. But only the main thread can remove something from Drafts, if the didClose notification is received. And we know it didn't, because it's busy handling the didChangeConfiguration notification.

So I'm confused.

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

I think I got it.
DraftMgr is confusing.... getActiveFiles() currently also returns removed files, because we never removes entries from DraftMgr, merely set contents to None and increments the version.

Now we definitely need a fix and a test for that :-)

clangd/DraftStore.cpp

29 ↗

(On Diff #131305)

Only add those entries where value is not None!

[simark]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

Ah you are right! "didClosing" a document before doing the config change triggers the assert.

Now we definitely need a fix and a test for that :-)

Are you saying that closing a document should remove the entry from the DraftMgr instead of just setting it to None? If so I can make a patch for that, it seems easy enough for a beginner. Or is the current behavior correct and we just need to make a test to verify it?

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

302 ↗

(On Diff #131131)

Unfortunately we can't move away from storing versions for removed files right now. We rely on them to report diagnostics in a consistent order (this is due to the weirdness of the threading model clangd currently uses, it's going away soon but we still need some time before that happens).
We could simply remove those files from the vector returned by getActiveFiles(). (And add a comment that the returned vector doesn't include those).

[simark]

Fix assertion about parsing a document that is not open

As found by Ilya, the getActiveFiles method would return the documents that were previously opened and then closed. This would cause the assertion in forceReparse:

assert(FileContents.Draft &&
  "forceReparse() was called for non-added document");

to trigger. I changed getActiveFiles to return only the files/drafts that are open at the time of the call.

[malaperle]

I haven't looked at the newest patch yet but I gave it a quick try and saw something odd. If I change the configuration to something invalid (say I specify the path to a CMakeLists.txt), then I get many errors/diagnostics, which is normal. But then when I change the config to something valid, the same diagnostics re-emitted, as if the configuration failed to change. I'll check the code tomorrow a bit but I thought I'd share this with you early.

[ilya-biryukov]

In D39571#1005926, @malaperle wrote:

I haven't looked at the newest patch yet but I gave it a quick try and saw something odd. If I change the configuration to something invalid (say I specify the path to a CMakeLists.txt), then I get many errors/diagnostics, which is normal. But then when I change the config to something valid, the same diagnostics re-emitted, as if the configuration failed to change. I'll check the code tomorrow a bit but I thought I'd share this with you early.

Maybe you were still seeing diagnostics from the older versions of the files? clangd should eventually produce the right diagnostics, but if processing of some of the requests is in-flight at the time there results may still be reported.
BTW, I don't know if you've seen it before, but there's -input-mirror-file option to clangd that records the lsp input which can be used to rerun clangd later.

# This should be run by VSCode (or Theia)
clangd -input-mirror-file=/tmp/clangd.input

# Later we can rerun from command line
$ clangd < /tmp/clangd.input
# Pass the flag to execute everyhing on one thread
$ clangd -run-synchronously < /tmp/clangd.input

This is useful to debug/rerun and see the logs, etc.

This patch is good to go after remove std::future<> from reparseOpenedFiles, unless @malaperle discovers there are other problems.
Ideally, it'd be also nice to have a test for it.

clangd/ClangdServer.cpp

541 ↗

(On Diff #133034)

We're not returning futures from forceReparse anymore, this function has to be updated accordingly.

[simark]

I think I managed to make some tests by using the MockCompilationDatabase. Basically with some code like:

#ifndef MACRO
static void func () {}  // 1
#else
static void func () {}  // 2
#endif

and these steps:

1. Server.addDocument(...)
2. Server.findDefinitions (assert that it returns definition 1)
3. CDB.ExtraClangFlags.push_back("-DMACRO=1")
4. Server.reparseOpenedFiles()
5. Server.findDefinitions (assert that it returns definition 2)

Right now that test fails, but it's not clear to me whether it's because the test is wrong or there's really a bug in there. I'll upload a work-in-progress version.

clangd/ClangdServer.cpp

541 ↗

(On Diff #133034)

Indeed, I found this by rebasing.

[simark]

Add tests, work in progress

Can you take a look at the "TEST(DidChangeConfiguration, DifferentDeclaration)"
test, and tell me if you see anything wrong with it? It seems to me like
changes in command line arguments (adding -DMACRO=1) are not taken into account
when changing configuration.

The other test (also very much work-in-progress) would be to verify that
changing configuration gets us the right diagnostics.

[ilya-biryukov]

In D39571#1006667, @simark wrote:

It seems to me like
changes in command line arguments (adding -DMACRO=1) are not taken into account
when changing configuration.

It looks like a bug in the preamble handling. (It does not check if macros were redefined).
You can workaround that by making sure the preamble ends before your code starts (preamble only captures preprocessor directives, so any C++ decl will end it):

  Annotations SourceAnnotations(R"cpp(
int avoid_preamble;

#ifndef MACRO
$before[[static void bob() {}]]
#else
$after[[static void bob() {}]]
#endif
/// ....
)cpp"

unittests/clangd/XRefsTests.cpp

260 ↗

(On Diff #134097)

I'd move it to ClangdTests.cpp, generic ClangdServer tests usually go there. It's fine to #include "Annotations.h" there, too, even though it hasn't been used before.

271 ↗

(On Diff #134097)

Specifying /*UseRelPath=*/true is not necessary for this test, default value should do.

[simark]

In D39571#1007291, @ilya-biryukov wrote:

It looks like a bug in the preamble handling. (It does not check if macros were redefined).
You can workaround that by making sure the preamble ends before your code starts (preamble only captures preprocessor directives, so any C++ decl will end it):

  Annotations SourceAnnotations(R"cpp(
int avoid_preamble;

#ifndef MACRO
$before[[static void bob() {}]]
#else
$after[[static void bob() {}]]
#endif
/// ....
)cpp"

Ah ok, indeed this test now works when I add this.

The other test I am working on (at the bottom of the file) acts weird, even if I add int avoid_preamble. The idea of the test is:

1. Set -DWITH_ERROR=1 in the compile commands database
2. Add the document, expect one error diagnostic
3. Unset WITH_ERROR in the compile commands database
4. Reparse the file, expect no error diagnostic

If I do it in this order, I get one diagnostic both times, when I don't expect one the second time the file is parsed. But if I do it the other way (first parse with no errors, second parse with an error), it works fine.

unittests/clangd/XRefsTests.cpp

260 ↗

(On Diff #134097)

Ok. I put it there for rapid prototyping, but I also though it didn't really belong there.

271 ↗

(On Diff #134097)

Ok.

[ilya-biryukov]

In D39571#1007393, @simark wrote:

If I do it in this order, I get one diagnostic both times, when I don't expect one the second time the file is parsed. But if I do it the other way (first parse with no errors, second parse with an error), it works fine.

That looks like another preamble-handling bug. It's much easier to fix and it's clangd-specific, so I'll make sure to fix that soon.
Thanks for bringing this up, we haven't been testing compile command changes thoroughly. I'll come up with a fix shortly.

Before this is fixed, you could try writing a similar test with other, non-preprocessor, flags. See ClangdVFSTest.ForceReparseCompileCommand that tests a similar thing by changing -xc to -xc++.
I'll make sure to test the -D case when fixing the bug.

[ilya-biryukov]

In D39571#1011842, @ilya-biryukov wrote:

In D39571#1007393, @simark wrote:

If I do it in this order, I get one diagnostic both times, when I don't expect one the second time the file is parsed. But if I do it the other way (first parse with no errors, second parse with an error), it works fine.

That looks like another preamble-handling bug. It's much easier to fix and it's clangd-specific, so I'll make sure to fix that soon.
Thanks for bringing this up, we haven't been testing compile command changes thoroughly. I'll come up with a fix shortly.

Before this is fixed, you could try writing a similar test with other, non-preprocessor, flags. See ClangdVFSTest.ForceReparseCompileCommand that tests a similar thing by changing -xc to -xc++.
I'll make sure to test the -D case when fixing the bug.

Should be fixed by D43454.

[simark]

In D39571#1011877, @ilya-biryukov wrote:

That looks like another preamble-handling bug. It's much easier to fix and it's clangd-specific, so I'll make sure to fix that soon.
Thanks for bringing this up, we haven't been testing compile command changes thoroughly. I'll come up with a fix shortly.

Before this is fixed, you could try writing a similar test with other, non-preprocessor, flags. See ClangdVFSTest.ForceReparseCompileCommand that tests a similar thing by changing -xc to -xc++.
I'll make sure to test the -D case when fixing the bug.

Should be fixed by D43454.

Thanks, I think I'll simply wait for that fix to get in.

[simark]

New version

Added a test for reparseOpenedFiles. It includes testing adding a file,
removing it, and reparsing the opened files, since this was broken at some
point.

[simark]

New version, take 2

The previous version contains the changes of already merged patches, I'm not
sure what I did wrong. This is another try.

[ilya-biryukov]

unittests/clangd/ClangdTests.cpp

83 ↗

(On Diff #135153)

NIT: misspelling: ErrorCHecking instead of ErrorChecking

94 ↗

(On Diff #135153)

This function should be const.
(Would require making Mutex mutable, but that's fine)

99 ↗

(On Diff #135153)

This function should be const and assert that P is in the map.

111 ↗

(On Diff #135153)

Maybe replace std::map<Path,bool> to llvm::StringMap<bool>?

492 ↗

(On Diff #135153)

Maybe expose a copy of the map from DiagConsumer and check for all files in a single line?

class MultipleErrorCHeckingDiagConsumer {
   /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
   /// For each file, a bool value indicates whether the last diagnostics contained an error.
   std::vector<std::pair<Path, bool>> filesWithDiags() const { /* ... */ }
};

/// ....
EXPECT_THAT(DiagConsumer.filesWithDiags(), UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true), Pair(BazCpp, false));

It would make the test more concise.

[simark]

unittests/clangd/ClangdTests.cpp

492 ↗

(On Diff #135153)

Nice :)

[simark]

unittests/clangd/ClangdTests.cpp

492 ↗

(On Diff #135153)

It's also better because we don't have to explicitly check that Baz is not there. So if some other file sneaks in the result for some reason and shouldn't be there, the test will now fail, where it wouldn't have previously.

[simark]

New version

Address comments in https://reviews.llvm.org/D39571#1014237

[ilya-biryukov]

Thanks for fixing all the comments! LGTM!

clang-tools-extra/trunk/clangd/ClangdLSPServer.h

private:
   void onSignatureHelp(TextDocumentPositionParams &Params) override;
   void onGoToDefinition(TextDocumentPositionParams &Params) override;
   void onSwitchSourceHeader(TextDocumentIdentifier &Params) override;
   void onDocumentHighlight(TextDocumentPositionParams &Params) override;
   void onFileEvent(DidChangeWatchedFilesParams &Params) override;
   void onCommand(ExecuteCommandParams &Params) override;
   void onRename(RenameParams &Parames) override;
   void onHover(TextDocumentPositionParams &Params) override;
+  void onChangeConfiguration(DidChangeConfigurationParams &Params) override;
 
   std::vector<TextEdit> getFixIts(StringRef File, const clangd::Diagnostic &D);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
   bool ShutdownRequestReceived = false;
 

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

Server.findHover(Params.textDocument.uri.file(), Params.position,
                                   llvm::toString(H.takeError()));
                        return;
                      }
 
                      reply(H->Value);
                    });
 }
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
+    DidChangeConfigurationParams &Params) {
+  ClangdConfigurationParamsChange &Settings = Params.settings;
+
+  // Compilation database change.
+  if (Settings.compilationDatabasePath.hasValue()) {
+    CDB.setCompileCommandsDir(Settings.compilationDatabasePath.getValue());
+    Server.reparseOpenedFiles();
+  }
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                                  bool StorePreamblesInMemory,
                                  const clangd::CodeCompleteOptions &CCOpts,
                                  llvm::Optional<StringRef> ResourceDir,
                                  llvm::Optional<Path> CompileCommandsDir,
                                  bool BuildDynamicSymbolIndex,
                                  SymbolIndex *StaticIdx)
     : Out(Out), CDB(std::move(CompileCommandsDir)), CCOpts(CCOpts),

clang-tools-extra/trunk/clangd/ClangdServer.h

public:
   void removeDocument(PathRef File);
 
   /// Force \p File to be reparsed using the latest contents.
   /// Will also check if CompileCommand, provided by GlobalCompilationDatabase
   /// for \p File has changed. If it has, will remove currently stored Preamble
   /// and AST and rebuild them from scratch.
   void forceReparse(PathRef File);
 
+  /// Calls forceReparse() on all currently opened files.
+  /// As a result, this method may be very expensive.
+  /// This method is normally called when the compilation database is changed.
+  void reparseOpenedFiles();
+
   /// Run code completion for \p File at \p Pos.
   /// Request is processed asynchronously.
   ///
   /// If \p OverridenContents is not None, they will used only for code
   /// completion, i.e. no diagnostics update will be scheduled and a draft for
   /// \p File will not be updated. If \p OverridenContents is None, contents of
   /// the current draft for \p File will be used. If \p UsedFS is non-null, it
   /// will be overwritten by vfs::FileSystem used for completion.

clang-tools-extra/trunk/clangd/ClangdServer.cpp

void ClangdServer::scheduleReparseAndDiags(
 
   WorkScheduler.update(File,
                        ParseInputs{std::move(Command),
                                    std::move(TaggedFS.Value),
                                    std::move(*Contents.Draft)},
                        WantDiags, std::move(Callback));
 }
 
+void ClangdServer::reparseOpenedFiles() {
+  for (const Path &FilePath : DraftMgr.getActiveFiles())
+    forceReparse(FilePath);
+}
+
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }
 
 std::vector<std::pair<Path, std::size_t>>
 ClangdServer::getUsedBytesPerFile() const {
   return WorkScheduler.getUsedBytesPerFile();
 }
 
 LLVM_NODISCARD bool
 ClangdServer::blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds) {
   return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds));
 }

clang-tools-extra/trunk/clangd/DraftStore.h

 /// filenames. The contents are owned by the DraftStore. Versions are mantained
 /// for the all added documents, including removed ones. The document version is
 /// incremented on each update and removal of the document.
 class DraftStore {
 public:
   /// \return version and contents of the stored document.
   /// For untracked files, a (0, None) pair is returned.
   VersionedDraft getDraft(PathRef File) const;
+
+  /// \return List of names of active drafts in this store.  Drafts that were
+  /// removed (which still have an entry in the Drafts map) are not returned by
+  /// this function.
+  std::vector<Path> getActiveFiles() const;
+
   /// \return version of the tracked document.
   /// For untracked files, 0 is returned.
   DocVersion getVersion(PathRef File) const;
 
   /// Replace contents of the draft for \p File with \p Contents.
   /// \return The new version of the draft for \p File.
   DocVersion updateDraft(PathRef File, StringRef Contents);
   /// Remove the contents of the draft

clang-tools-extra/trunk/clangd/DraftStore.cpp

VersionedDraft DraftStore::getDraft(PathRef File) const {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   auto It = Drafts.find(File);
   if (It == Drafts.end())
     return {0, llvm::None};
   return It->second;
 }
 
+std::vector<Path> DraftStore::getActiveFiles() const {
+  std::lock_guard<std::mutex> Lock(Mutex);
+  std::vector<Path> ResultVector;
+
+  for (auto DraftIt = Drafts.begin(); DraftIt != Drafts.end(); DraftIt++)
+    if (DraftIt->second.Draft)
+      ResultVector.push_back(DraftIt->getKey());
+
+  return ResultVector;
+}
+
 DocVersion DraftStore::getVersion(PathRef File) const {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   auto It = Drafts.find(File);
   if (It == Drafts.end())
     return 0;
   return It->second.Version;
 }

clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h

public:
   /// Scans File's parents looking for compilation databases.
   /// Any extra flags will be added.
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
 
   /// Uses the default fallback command, adding any extra flags.
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
+  /// Set the compile commands directory to \p P.
+  void setCompileCommandsDir(Path P);
+
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
 
 private:
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
   tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
   void addExtraFlags(PathRef File, tooling::CompileCommand &C) const;
 

clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp

 tooling::CompileCommand
 DirectoryBasedGlobalCompilationDatabase::getFallbackCommand(
     PathRef File) const {
   auto C = GlobalCompilationDatabase::getFallbackCommand(File);
   addExtraFlags(File, C);
   return C;
 }
 
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+  std::lock_guard<std::mutex> Lock(Mutex);
+  CompileCommandsDir = P;
+  CompilationDatabases.clear();
+}
+
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
     PathRef File, std::vector<std::string> ExtraFlags) {
   std::lock_guard<std::mutex> Lock(Mutex);
   ExtraFlagsForFile[File] = std::move(ExtraFlags);
 }
 
 void DirectoryBasedGlobalCompilationDatabase::addExtraFlags(
     PathRef File, tooling::CompileCommand &C) const {

clang-tools-extra/trunk/clangd/Protocol.h

 bool fromJSON(const json::Expr &, FileEvent &);
 
 struct DidChangeWatchedFilesParams {
   /// The actual file events.
   std::vector<FileEvent> changes;
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
+/// Clangd extension to manage a workspace/didChangeConfiguration notification
+/// since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional<std::string> compilationDatabasePath;
+};
+bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &);
+
+struct DidChangeConfigurationParams {
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  ClangdConfigurationParamsChange settings;
+};
+bool fromJSON(const json::Expr &, DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize = 0;
 
   /// Prefer spaces over tabs.
   bool insertSpaces = false;
 };
 bool fromJSON(const json::Expr &, FormattingOptions &);

clang-tools-extra/trunk/clangd/Protocol.cpp

 
 json::Expr toJSON(const DocumentHighlight &DH) {
   return json::obj{
       {"range", toJSON(DH.range)},
       {"kind", static_cast<int>(DH.kind)},
   };
 }
 
+bool fromJSON(const json::Expr &Params, DidChangeConfigurationParams &CCP) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+bool fromJSON(const json::Expr &Params, ClangdConfigurationParamsChange &CCPC) {
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/ProtocolHandlers.h

public:
   virtual void onSignatureHelp(TextDocumentPositionParams &Params) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams &Params) = 0;
   virtual void onSwitchSourceHeader(TextDocumentIdentifier &Params) = 0;
   virtual void onFileEvent(DidChangeWatchedFilesParams &Params) = 0;
   virtual void onCommand(ExecuteCommandParams &Params) = 0;
   virtual void onRename(RenameParams &Parames) = 0;
   virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0;
   virtual void onHover(TextDocumentPositionParams &Params) = 0;
+  virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
                               ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp

void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   Register("textDocument/switchSourceHeader",
            &ProtocolCallbacks::onSwitchSourceHeader);
   Register("textDocument/rename", &ProtocolCallbacks::onRename);
   Register("textDocument/hover", &ProtocolCallbacks::onHover);
   Register("workspace/didChangeWatchedFiles", &ProtocolCallbacks::onFileEvent);
   Register("workspace/executeCommand", &ProtocolCallbacks::onCommand);
   Register("textDocument/documentHighlight",
            &ProtocolCallbacks::onDocumentHighlight);
+  Register("workspace/didChangeConfiguration",
+           &ProtocolCallbacks::onChangeConfiguration);
 }

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

 //===-- ClangdTests.cpp - Clangd unit tests ---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "URI.h"
 #include "clang/Config/config.h"
 #include "llvm/ADT/SmallVector.h"
public:
   VFSTag lastVFSTag() { return LastVFSTag; }
 
 private:
   std::mutex Mutex;
   bool HadErrorInLastDiags = false;
   VFSTag LastVFSTag = VFSTag();
 };
 
+/// For each file, record whether the last published diagnostics contained at
+/// least one error.
+class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
+public:
+  void
+  onDiagnosticsReady(PathRef File,
+                     Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+    bool HadError = diagsContainErrors(Diagnostics.Value);
+
+    std::lock_guard<std::mutex> Lock(Mutex);
+    LastDiagsHadError[File] = HadError;
+  }
+
+  /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
+  /// For each file, a bool value indicates whether the last diagnostics
+  /// contained an error.
+  std::vector<std::pair<Path, bool>> filesWithDiags() const {
+    std::vector<std::pair<Path, bool>> Result;
+    std::lock_guard<std::mutex> Lock(Mutex);
+
+    for (const auto &it : LastDiagsHadError) {
+      Result.emplace_back(it.first(), it.second);
+    }
+
+    return Result;
+  }
+
+  void clear() {
+    std::lock_guard<std::mutex> Lock(Mutex);
+    LastDiagsHadError.clear();
+  }
+
+private:
+  mutable std::mutex Mutex;
+  llvm::StringMap<bool> LastDiagsHadError;
+};
+
 /// Replaces all patterns of the form 0x123abc with spaces
 std::string replacePtrsInDump(std::string const &Dump) {
   llvm::Regex RE("0x[0-9a-fA-F]+");
   llvm::SmallVector<StringRef, 1> Matches;
   llvm::StringRef Pending = Dump;
 
   std::string Result;
   while (RE.match(Pending, &Matches)) {
)cpp";
   // But forceReparse should reparse the file with proper flags.
   Server.forceReparse(FooCpp);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
   // Subsequent addDocument call should finish without errors too.
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+// Test ClangdServer.reparseOpenedFiles.
+TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
+  Annotations FooSource(R"cpp(
+#ifdef MACRO
+$one[[static void bob() {}]]
+#else
+$two[[static void bob() {}]]
+#endif
+
+int main () { bo^b (); return 0; }
+)cpp");
+
+  Annotations BarSource(R"cpp(
+#ifdef MACRO
+this is an error
+#endif
+)cpp");
+
+  Annotations BazSource(R"cpp(
+int hello;
+)cpp");
+
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  MultipleErrorCheckingDiagConsumer DiagConsumer;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+                      /*AsyncThreadsCount=*/0,
+                      /*StorePreamblesInMemory=*/true);
+
+  auto FooCpp = testPath("foo.cpp");
+  auto BarCpp = testPath("bar.cpp");
+  auto BazCpp = testPath("baz.cpp");
+
+  FS.Files[FooCpp] = "";
+  FS.Files[BarCpp] = "";
+  FS.Files[BazCpp] = "";
+
+  CDB.ExtraClangFlags = {"-DMACRO=1"};
+  Server.addDocument(FooCpp, FooSource.code());
+  Server.addDocument(BarCpp, BarSource.code());
+  Server.addDocument(BazCpp, BazSource.code());
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+              UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
+                                   Pair(BazCpp, false)));
+
+  auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+                                                     FooSource.range("one")}));
+
+  // Undefine MACRO, close baz.cpp.
+  CDB.ExtraClangFlags.clear();
+  DiagConsumer.clear();
+  Server.removeDocument(BazCpp);
+  Server.reparseOpenedFiles();
+
+  EXPECT_THAT(DiagConsumer.filesWithDiags(),
+              UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
+
+  Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
+  EXPECT_TRUE(bool(Locations));
+  EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp},
+                                                     FooSource.range("two")}));
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, DiagConsumer, FS,
                       /*AsyncThreadsCount=*/0,
                       /*StorePreamblesInMemory=*/true);