Page MenuHomePhabricator

[clangd] DidChangeConfiguration Notification
ClosedPublic

Authored by simark on Nov 2 2017, 2:44 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Merged with latest llvm + clang
Minor code cleanup

Nebiroth updated this revision to Diff 126449.Dec 11 2017, 2:31 PM

Removed some more empty lines

ilya-biryukov requested changes to this revision.Dec 13 2017, 5:37 AM

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.

This revision now requires changes to proceed.Dec 13 2017, 5:37 AM
Nebiroth updated this revision to Diff 126980.Dec 14 2017, 9:06 AM
Nebiroth marked 5 inline comments as done.

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

simark updated this revision to Diff 131051.Jan 23 2018, 5:42 AM
simark added a subscriber: 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 added inline comments.Jan 23 2018, 7:52 AM
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 added inline comments.Jan 23 2018, 12:34 PM
clangd/ClangdLSPServer.cpp
78 ↗(On Diff #131051)

Agreed, perhaps configurationChangeManager would be more appropriate then?

simark added inline comments.Jan 23 2018, 12:43 PM
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 updated this revision to Diff 131131.Jan 23 2018, 1:00 PM

Fix formatting, remove capability

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

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 added inline comments.Jan 24 2018, 8:19 AM
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 updated this revision to Diff 131305.Jan 24 2018, 10:11 AM

Move implementation of setCompileCommandsDir to .cpp

ilya-biryukov added inline comments.Jan 25 2018, 1:30 AM
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 added inline comments.Jan 25 2018, 6:41 AM
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 added inline comments.Jan 25 2018, 6:57 AM
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 added inline comments.Jan 25 2018, 12:33 PM
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 added inline comments.Jan 31 2018, 10:41 AM
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 added inline comments.Feb 1 2018, 1:38 AM
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 added inline comments.Feb 1 2018, 6:25 AM
clangd/ClangdLSPServer.cpp
302 ↗(On Diff #131131)

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

Exactly.

simark added inline comments.Feb 1 2018, 2:57 PM
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 added inline comments.Feb 2 2018, 12:49 AM
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 added inline comments.Feb 2 2018, 8:31 AM
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 added inline comments.Feb 5 2018, 8:09 AM
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 updated this revision to Diff 133034.EditedFeb 6 2018, 10:06 AM

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.

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.

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.

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 updated this revision to Diff 134097.Feb 13 2018, 12:30 PM

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.

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.

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.

simark commandeered this revision.Feb 16 2018, 2:50 PM
simark added a reviewer: Nebiroth.

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.

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.

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 updated this revision to Diff 135152.Feb 20 2018, 3:32 PM

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 updated this revision to Diff 135153.Feb 20 2018, 3:36 PM

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 added inline comments.Feb 21 2018, 2:43 AM
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 marked 5 inline comments as done.Feb 21 2018, 4:34 AM
simark added inline comments.
unittests/clangd/ClangdTests.cpp
492 ↗(On Diff #135153)

Nice :)

simark marked an inline comment as done.Feb 21 2018, 4:40 AM
simark added inline comments.
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.

ilya-biryukov accepted this revision.Feb 22 2018, 2:16 AM

Thanks for fixing all the comments! LGTM!

This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2018, 6:05 AM
Closed by commit rL325784: [clangd] DidChangeConfiguration Notification (authored by simark, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.