This is an archive of the discontinued LLVM Phabricator instance.

[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

Nebiroth created this revision.Nov 2 2017, 2:44 PM
malaperle requested changes to this revision.Nov 3 2017, 8:46 AM
malaperle added inline comments.
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.

This revision now requires changes to proceed.Nov 3 2017, 8:46 AM
ilya-biryukov requested changes to this revision.Nov 8 2017, 2:30 AM

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 added inline comments.Nov 8 2017, 2:15 PM
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 added inline comments.Nov 8 2017, 11:51 PM
clangd/ClangdServer.h
289 ↗(On Diff #121388)

Exactly!

Nebiroth marked 18 inline comments as done.Nov 14 2017, 2:07 PM
Nebiroth added inline comments.
clangd/Protocol.h
285 ↗(On Diff #121388)

I do inside parse() for DidChangeConfigurationParams.

Nebiroth updated this revision to Diff 123069.Nov 15 2017, 12:18 PM
Nebiroth edited edge metadata.
Nebiroth marked 3 inline comments as done.

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 added inline comments.Nov 16 2017, 5:09 AM
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

ilya-biryukov requested changes to this revision.Nov 16 2017, 5:09 AM
This revision now requires changes to proceed.Nov 16 2017, 5:09 AM
malaperle added inline comments.Nov 16 2017, 6:05 AM
clangd/Protocol.h
295 ↗(On Diff #123069)

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

ilya-biryukov added inline comments.Nov 16 2017, 7:54 AM
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 added inline comments.Nov 16 2017, 8:01 AM
clangd/Protocol.h
295 ↗(On Diff #123069)

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

Nebiroth updated this revision to Diff 124024.Nov 22 2017, 4:33 PM
Nebiroth edited edge metadata.
Nebiroth marked 19 inline comments as done.

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 added inline comments.Nov 22 2017, 4:33 PM
clangd/GlobalCompilationDatabase.cpp
108 ↗(On Diff #123069)

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

ilya-biryukov added inline comments.Nov 23 2017, 3:32 AM
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

ilya-biryukov requested changes to this revision.Nov 23 2017, 3:32 AM
This revision now requires changes to proceed.Nov 23 2017, 3:32 AM
Nebiroth marked 12 inline comments as done.Nov 27 2017, 10:20 AM
Nebiroth added inline comments.
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 updated this revision to Diff 126447.Dec 11 2017, 2:25 PM
Nebiroth edited edge metadata.

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
This revision was automatically updated to reflect the committed changes.