[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

43

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

246

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?

248

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

250

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.

255

Pass Params.settings? see previous comment.

clangd/ClangdServer.cpp

516

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

57

remove?

58

call getCompilationDatabase from here?

60

move back to private?

clangd/Protocol.cpp

183

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

210

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

227

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

231

else {

  logIgnoredField(KeyValue, Logger);
}

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

"customField": ""

}

clangd/Protocol.h

260

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

269

I don't think you need this constructor?

271

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.

274

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

246

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

295

NIT: Add full stop at the end of comments.

296

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

43

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

clangd/ClangdServer.h

296

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

183

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

296

Exactly!

[Nebiroth]

clangd/Protocol.h

269

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

584

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.

585

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

clangd/ClangdServer.h

318

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.

320

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.

320

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

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

clangd/GlobalCompilationDatabase.cpp

108–109

Accidental change?

clangd/GlobalCompilationDatabase.h

65

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

65

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

74

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

clangd/Protocol.h

263

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

268

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

test/clangd/initialize-params-invalid.test

2

Accidental newline?

21

NIT: the indent seems off by one character

test/clangd/initialize-params.test

2

Accidental newline?

21

NIT: the indent seems off by one character

[malaperle]

clangd/Protocol.h

263

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

[ilya-biryukov]

clangd/Protocol.h

263

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

263

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–109

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

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

251

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

clangd/ClangdServer.cpp

584

NIT: remote empty line

586

DonePromise and DoneFuture are never used. Remove them?

592

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

clangd/ClangdServer.h

241

NIT: remove this empty line

clangd/GlobalCompilationDatabase.cpp

108–109

Right. Sorry, my mistake.

clangd/Protocol.h

262

NIT: remote empty line

264

NIT: remote empty line

271

NIT: remove empty line

272

NIT: Use = default instead

273

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

16

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

34

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

1

I am still seeing this newline

[Nebiroth]

test/clangd/did-change-configuration.test

34

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

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

55

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

clangd/GlobalCompilationDatabase.h

65

We need to lock the Mutex here!

clangd/Protocol.cpp

368

NIT: maybe remove this empty line?

test/clangd/did-change-configuration.test

34

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

65

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

65

Agreed, perhaps configurationChangeManager would be more appropriate then?

[simark]

clangd/ClangdLSPServer.cpp

65

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

244

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

clangd/GlobalCompilationDatabase.h

64

Maybe move definition to .cpp file?

[simark]

clangd/ClangdLSPServer.cpp

244

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

Will do.

[simark]

Move implementation of setCompileCommandsDir to .cpp

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

244

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

244

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

65

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

244

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

244

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

65

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

244

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

65

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

244

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

[simark]

clangd/ClangdLSPServer.cpp

244

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

Exactly.

[simark]

clangd/ClangdLSPServer.cpp

244

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

244

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

Only add those entries where value is not None!

[simark]

clangd/ClangdLSPServer.cpp

244

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

244

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

583

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

583

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!

clangd/ClangdLSPServer.h

private:
   void onCodeAction(Ctx C, CodeActionParams &Params) override;
   void onCompletion(Ctx C, TextDocumentPositionParams &Params) override;
   void onSignatureHelp(Ctx C, TextDocumentPositionParams &Params) override;
   void onGoToDefinition(Ctx C, TextDocumentPositionParams &Params) override;
   void onSwitchSourceHeader(Ctx C, TextDocumentIdentifier &Params) override;
   void onFileEvent(Ctx C, DidChangeWatchedFilesParams &Params) override;
   void onCommand(Ctx C, ExecuteCommandParams &Params) override;
   void onRename(Ctx C, RenameParams &Parames) override;
+  void onChangeConfiguration(Ctx C,
+                             DidChangeConfigurationParams &Params) override;
 
   std::vector<clang::tooling::Replacement>
   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;

clangd/ClangdLSPServer.cpp

 //===--- ClangdLSPServer.cpp - LSP server ------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "JSONRPCDispatcher.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Program.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include "llvm/Support/FormatVariadic.h"
 
 using namespace clang::clangd;
 using namespace clang;
 
 namespace {
 
for (auto &R : Replacements) {
     Edits.push_back({ReplacementRange, R.getReplacementText()});
   }
   return Edits;
 }
 
 } // namespace
 
 void ClangdLSPServer::onInitialize(Ctx C, InitializeParams &Params) {
   C.reply(json::obj{
       {{"capabilities",
         json::obj{
             {"textDocumentSync", 1},
             {"documentFormattingProvider", true},
             {"documentRangeFormattingProvider", true},
             {"documentOnTypeFormattingProvider",
              json::obj{
                  {"firstTriggerCharacter", "}"},
                  {"moreTriggerCharacter", {}},
              }},
             {"codeActionProvider", true},
             {"completionProvider",
              json::obj{
                  {"resolveProvider", false},
                  {"triggerCharacters", {".", ">", ":"}},
              }},
             {"signatureHelpProvider",
              json::obj{
                  {"triggerCharacters", {"(", ","}},
              }},
             {"definitionProvider", true},
+            {"configurationChangeProvider", true},
             {"renameProvider", true},
             {"executeCommandProvider",
              json::obj{
                  {"commands", {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND}},
              }},
         }}}});
   if (Params.rootUri && !Params.rootUri->file.empty())
     Server.setRootPath(Params.rootUri->file);

 
 void ClangdLSPServer::onSwitchSourceHeader(Ctx C,
                                            TextDocumentIdentifier &Params) {
   llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file);
   std::string ResultUri;
   C.reply(Result ? URI::fromFile(*Result).uri : "");
 }
 
+void ClangdLSPServer::onChangeConfiguration(
+    Ctx C, DidChangeConfigurationParams &Params) {
+
+  // Verify if path has value and is a valid path
+  if (Params.settings.compilationDatabasePath.hasValue()) {
+    CDB.setCompileCommandsDir(
+        Params.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)
     : Out(Out), CDB(/*Logger=*/Out, std::move(CompileCommandsDir)),
       CCOpts(CCOpts), Server(CDB, /*DiagConsumer=*/*this, FSProvider,
                              AsyncThreadsCount, StorePreamblesInMemory,
                              /*Logger=*/Out, ResourceDir) {}

clangd/ClangdServer.h

public:
   /// associated resources are freed.
   std::future<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.
   std::future<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.
+  std::vector<std::future<void>> reparseOpenedFiles();
+
   /// DEPRECATED. Please use a callback-based version, this API is deprecated
   /// and will soon be removed.
   ///
   /// Run code completion for \p File at \p Pos.
   ///
   /// Request is processed asynchronously. You can use the returned future to
   /// wait for the results of the async request.
   ///
public:
   llvm::Expected<Tagged<std::vector<Location>>> findDefinitions(PathRef File,
                                                                 Position Pos);
 
   /// Helper function that returns a path to the corresponding source file when
   /// given a header file and vice versa.
   llvm::Optional<Path> switchSourceHeader(PathRef Path);
 
   /// Run formatting for \p Rng inside \p File.
   std::vector<tooling::Replacement> formatRange(PathRef File, Range Rng);
   /// Run formatting for the whole \p File.
   std::vector<tooling::Replacement> formatFile(PathRef File);
   /// Run formatting after a character was typed at \p Pos in \p File.
   std::vector<tooling::Replacement> formatOnType(PathRef File, Position Pos);
   /// Rename all occurrences of the symbol at the \p Pos in \p File to
   /// \p NewName.
   Expected<std::vector<tooling::Replacement>> rename(PathRef File, Position Pos,
                                                      llvm::StringRef NewName);
 
   /// Gets current document contents for \p File. \p File must point to a
   /// currently tracked file.
   /// FIXME(ibiryukov): This function is here to allow offset-to-Position
   /// conversions in outside code, maybe there's a way to get rid of it.
   std::string getDocument(PathRef File);
 
   /// Only for testing purposes.
   /// Waits until all requests to worker thread are finished and dumps AST for
   /// \p File. \p File must be in the list of added documents.
   std::string dumpAST(PathRef File);
   /// Called when an event occurs for a watched file in the workspace.
   void onFileEvent(const DidChangeWatchedFilesParams &Params);
 
 private:
   std::future<void>
   scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
                           std::shared_ptr<CppFile> Resources,
                           Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
 
   std::future<void> scheduleCancelRebuild(std::shared_ptr<CppFile> Resources);
 
   clangd::Logger &Logger;
   GlobalCompilationDatabase &CDB;
   DiagnosticsConsumer &DiagConsumer;

clangd/ClangdServer.cpp

llvm::Optional<Path> ClangdServer::switchSourceHeader(PathRef Path) {
 
   return llvm::None;
 }
 
 std::future<void> ClangdServer::scheduleReparseAndDiags(
     PathRef File, VersionedDraft Contents, std::shared_ptr<CppFile> Resources,
     Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS) {
 
   assert(Contents.Draft && "Draft must have contents");
   UniqueFunction<llvm::Optional<std::vector<DiagWithFixIts>>()>
       DeferredRebuild =
           Resources->deferRebuild(*Contents.Draft, TaggedFS.Value);
   std::promise<void> DonePromise;
   std::future<void> DoneFuture = DonePromise.get_future();
 
   DocVersion Version = Contents.Version;
   Path FileStr = File;
auto CancelReparses = [Resources](std::promise<void> DonePromise,
     FulfillPromiseGuard Guard(DonePromise);
     DeferredCancel();
   };
   WorkScheduler.addToFront(std::move(CancelReparses), std::move(DonePromise),
                            std::move(DeferredCancel));
   return DoneFuture;
 }
 
+std::vector<std::future<void>> ClangdServer::reparseOpenedFiles() {
+  std::vector<std::future<void>> FutureVector;
+  std::vector<Path> ActiveFilePaths;
+
+  ActiveFilePaths = DraftMgr.getActiveFiles();
+
+  for (const auto& FilePath : ActiveFilePaths)
+    FutureVector.push_back(forceReparse(FilePath));
+
+  return FutureVector;
+}
+
+
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }

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 all active files
+  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

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 Draft : Drafts.keys())
+    ResultVector.push_back(Draft);
+
+  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;
 }
 
 DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents) {
   std::lock_guard<std::mutex> Lock(Mutex);
+  assert(!File.empty());
 
   auto &Entry = Drafts[File];
   DocVersion NewVersion = ++Entry.Version;
   Entry.Draft = Contents;
   return NewVersion;
 }
 
 DocVersion DraftStore::removeDraft(PathRef File) {
   std::lock_guard<std::mutex> Lock(Mutex);
+  assert(!File.empty());
 
   auto &Entry = Drafts[File];
   DocVersion NewVersion = ++Entry.Version;
   Entry.Draft = llvm::None;
   return NewVersion;
 }

clangd/GlobalCompilationDatabase.h

 /// directories.
 class DirectoryBasedGlobalCompilationDatabase
     : public GlobalCompilationDatabase {
 public:
   DirectoryBasedGlobalCompilationDatabase(
       clangd::Logger &Logger, llvm::Optional<Path> CompileCommandsDir);
 
   /// 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;
 
+  void setCompileCommandsDir(Path P) {
+    CompileCommandsDir = P;
+    CompilationDatabases.clear();
+  }
+
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
 
 private:
   tooling::CompilationDatabase *getCompilationDatabase(PathRef File) const;
   tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File) const;
   void addExtraFlags(PathRef File, tooling::CompileCommand &C) const;
 
   mutable std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
   /// directories).
   mutable llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
       CompilationDatabases;
 

clangd/GlobalCompilationDatabase.cpp

 //===--- GlobalCompilationDatabase.cpp --------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
 #include "Logger.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 
 tooling::CompileCommand
 GlobalCompilationDatabase::getFallbackCommand(PathRef File) const {
DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(
     PathRef File) const {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   namespace path = llvm::sys::path;
   if (CompileCommandsDir.hasValue()) {
     tooling::CompilationDatabase *ReturnValue =
         tryLoadDatabaseFromPath(CompileCommandsDir.getValue());
     if (ReturnValue == nullptr)
       Logger.log("Failed to find compilation database for " + Twine(File) +
-                 "in overriden directory " + CompileCommandsDir.getValue());
+                 " in overriden directory " + CompileCommandsDir.getValue());
     return ReturnValue;
   }
 
   for (auto Path = path::parent_path(File); !Path.empty();
        Path = path::parent_path(Path)) {
     auto CDB = tryLoadDatabaseFromPath(Path);
     if (!CDB)
       continue;

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 {
+  DidChangeConfigurationParams() = default;
+  DidChangeConfigurationParams(ClangdConfigurationParamsChange settings): settings(settings) {}
+
+  // 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 &);
+json::Expr toJSON(const DidChangeConfigurationParams &);
+
 struct FormattingOptions {
   /// Size of a tab in spaces.
   int tabSize;
 
   /// Prefer spaces over tabs.
   bool insertSpaces;
 };
 bool fromJSON(const json::Expr &, FormattingOptions &);

clangd/Protocol.cpp

if (*T < static_cast<int>(FileChangeType::Created) ||
         *T > static_cast<int>(FileChangeType::Deleted))
       return false;
     Out = static_cast<FileChangeType>(*T);
     return true;
   }
   return false;
 }
 
 bool fromJSON(const json::Expr &Params, FileEvent &R) {
   json::ObjectMapper O(Params);
   return O && O.map("uri", R.uri) && O.map("type", R.type);
 }
 
 bool fromJSON(const json::Expr &Params, DidChangeWatchedFilesParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("changes", R.changes);
 }

 }
 
 json::Expr toJSON(const FormattingOptions &P) {
   return json::obj{
       {"tabSize", P.tabSize},
       {"insertSpaces", P.insertSpaces},
   };
 }
 
 bool fromJSON(const json::Expr &Params, DocumentRangeFormattingParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&
          O.map("range", R.range) && O.map("options", R.options);
 }
 
 bool fromJSON(const json::Expr &Params, DocumentOnTypeFormattingParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&
          O.map("position", R.position) && O.map("ch", R.ch) &&
          O.map("options", R.options);
 }
 
 bool fromJSON(const json::Expr &Params, DocumentFormattingParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&
          O.map("options", R.options);
 }
 
 bool fromJSON(const json::Expr &Params, Diagnostic &R) {
   json::ObjectMapper O(Params);
   if (!O || !O.map("range", R.range) || !O.map("message", R.message))
     return false;
   O.map("severity", R.severity);
   return true;
 }
 
 bool fromJSON(const json::Expr &Params, CodeActionContext &R) {
   json::ObjectMapper O(Params);

 }
 
 bool fromJSON(const json::Expr &Params, RenameParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&
          O.map("position", R.position) && O.map("newName", R.newName);
 }
 
+bool fromJSON(const json::Expr &Params, DidChangeConfigurationParams &CCP) {
+  json::ObjectMapper O(Params);
+  return O && O.map("settings", CCP.settings);
+}
+
+json::Expr toJSON(const DidChangeConfigurationParams &CCP) {
+
+  return json::obj{
+    {"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

clangd/ProtocolHandlers.h

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

clangd/ProtocolHandlers.cpp

void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   Register("textDocument/completion", &ProtocolCallbacks::onCompletion);
   Register("textDocument/signatureHelp", &ProtocolCallbacks::onSignatureHelp);
   Register("textDocument/definition", &ProtocolCallbacks::onGoToDefinition);
   Register("textDocument/switchSourceHeader",
            &ProtocolCallbacks::onSwitchSourceHeader);
   Register("textDocument/rename", &ProtocolCallbacks::onRename);
   Register("workspace/didChangeWatchedFiles", &ProtocolCallbacks::onFileEvent);
   Register("workspace/executeCommand", &ProtocolCallbacks::onCommand);
+  Register("workspace/didChangeConfiguration",
+           &ProtocolCallbacks::onChangeConfiguration);
 }

test/clangd/did-change-configuration.test

+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 150
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"voidmain(){}"}}}
+
+Content-Length: 86
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":""}}
+#Failed to decode workspace/didChangeConfiguration request.
+#Incorrect mapping node
+
+Content-Length: 114
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":{}}}}
+#Failed to decode workspace/didChangeConfiguration request.
+#compilationDatabasePath is not a scalar node
+
+Content-Length: 140
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/","testBadValue":"foo1234"}}}
+#Ignored unknown field "testBadValue"
+#Failed to find compilation database for / in overriden directory /
+#Bad field, bad compilation database
+
+Content-Length: 722
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+
+Content-Length: 115
+
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"/"}}}
+#CHECK:{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"diagnostics":[{"message":"type specifier missing, defaults to 'int'","range":{"end":{"character":1,"line":0},"start":{"character":1,"line":0}},"severity":2},{"message":"control reaches end of non-void function","range":{"end":{"character":12,"line":0},"start":{"character":12,"line":0}},"severity":2}],"uri":"file:///foo.c"}}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":10000,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}

test/clangd/initialize-params-invalid.test

 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 # It is absolutely vital that this file has CRLF line endings.
 #
 # Test with invalid initialize request parameters
 Content-Length: 142
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":"","rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
 #      CHECK:  "id": 0,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "capabilities": {
 # CHECK-NEXT:      "codeActionProvider": true,
 # CHECK-NEXT:      "completionProvider": {
 # CHECK-NEXT:        "resolveProvider": false,
 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          ".",
 # CHECK-NEXT:          ">",
 # CHECK-NEXT:          ":"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
+# CHECK-NEXT:      "configurationChangeProvider": true,
 # CHECK-NEXT:      "definitionProvider": true,
 # CHECK-NEXT:      "documentFormattingProvider": true,
 # CHECK-NEXT:      "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:        "firstTriggerCharacter": "}",
 # CHECK-NEXT:        "moreTriggerCharacter": []
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "documentRangeFormattingProvider": true,
 # CHECK-NEXT:      "executeCommandProvider": {

test/clangd/initialize-params.test

 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 # It is absolutely vital that this file has CRLF line endings.
 #
 # Test initialize request parameters with rootUri
 Content-Length: 143
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
 #      CHECK:  "id": 0,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "capabilities": {
 # CHECK-NEXT:      "codeActionProvider": true,
 # CHECK-NEXT:      "completionProvider": {
 # CHECK-NEXT:        "resolveProvider": false,
 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          ".",
 # CHECK-NEXT:          ">",
 # CHECK-NEXT:          ":"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
+# CHECK-NEXT:      "configurationChangeProvider": true,
 # CHECK-NEXT:      "definitionProvider": true,
 # CHECK-NEXT:      "documentFormattingProvider": true,
 # CHECK-NEXT:      "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:        "firstTriggerCharacter": "}",
 # CHECK-NEXT:        "moreTriggerCharacter": []
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "documentRangeFormattingProvider": true,
 # CHECK-NEXT:      "executeCommandProvider": {