This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add support for per-file override compilation command
AbandonedPublic

Authored by arphaman on Jul 18 2018, 4:53 PM.

Details

Summary

This patch builds on top of the "extra flags" extension added in r307241.
It adds the ability to specify user-defined custom compilation command for an opened file through the LSP layer. This is a non-standard extension to the protocol.

The particular use-case is follows: our clients do not have a compilation database and will specify all compilation commands dynamically when a file is open.

Diff Detail

Event Timeline

arphaman created this revision.Jul 18 2018, 4:53 PM

Btw, the "extraFlags" extension is still usable with "compilationCommand".

malaperle added a subscriber: malaperle.

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

Thanks for your response,
As it stands right now we will not run the indexer in Clangd for our use case, and it's unclear if we can even assemble a set of compile commands, so we would like to provide the commands when a file is open. We might be interested in a "didChangeConfiguration" message extension in the future (ideally it would be possible to change the subset of the constructed compilation database).

jkorous added a comment.EditedJul 20 2018, 5:48 AM

Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message? All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen.

EDIT: Well, provided we would find a way how not to parse the file twice.

Alex, I am just wondering if we shouldn't rather create another implementation of GlobalCompilationDatabase interface (something like InMemoryGlobalCompilationDatabase), add it to ClangdServer and use it as the first place to be searched in ClangdServer::getCompileCommand(PathRef File). What do you think?

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

Thanks for your response,
As it stands right now we will not run the indexer in Clangd for our use case, and it's unclear if we can even assemble a set of compile commands, so we would like to provide the commands when a file is open. We might be interested in a "didChangeConfiguration" message extension in the future (ideally it would be possible to change the subset of the constructed compilation database).

Sounds good, I think sending it in didOpen makes sense then. And yes, I agree that we would need to support changing a subset of commands through didChangeConfiguration, eventually!

Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message?

We don't yet :) But we will need to send the information per-file through it instead of using the compile_commands.json. Right now, what is supported in the didChangeConfiguration is pointing to a different compile_commands.json, in order to support multiple configuration (actually pointing to the folder containing the json file).

All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen.

EDIT: Well, provided we would find a way how not to parse the file twice.

The idea would be to send the per-file commands to clangd *before* anything is opened. So the didOpen would just read the latest command in memory. And subsequent changes to commands would be communicated with didChangeConfiguration and then files would get reparsed. I'm actually thinking we might want to send the commands in the "initialize" request for all the initial commands and then update them with didChangeConfiguration whenever they change. That way, there is no risk for reparsing as we should not do anything (indexing!) before the initialize.
But it doesn't sounds like you need this right now :)

BTW it looks like we already had kind of support for compilation command before (extra flags).

commit 5ec1f7ca32eb85077a22ce81d41aa02a017d4852
Author: Krasimir Georgiev <krasimir@google.com>
Date: Thu Jul 6 08:44:54 2017 +0000

[clangd] Add support for per-file extra flags

There is even an LSP extension proposal:

https://github.com/Microsoft/language-server-protocol/issues/255

But it seems as if we lost access to it here:

commit aa49372548ff984ae9c48879a0d05833538a76b3
Author: Sam McCall <sam.mccall@gmail.com>
Date: Mon Dec 4 10:08:45 2017 +0000

[clangd] GlobalCompilationDatabase interface changes

BTW it looks like we already had kind of support for compilation command before (extra flags).

commit 5ec1f7ca32eb85077a22ce81d41aa02a017d4852
Author: Krasimir Georgiev <krasimir@google.com>
Date: Thu Jul 6 08:44:54 2017 +0000

[clangd] Add support for per-file extra flags

There is even an LSP extension proposal:

https://github.com/Microsoft/language-server-protocol/issues/255

But it seems as if we lost access to it here:

commit aa49372548ff984ae9c48879a0d05833538a76b3
Author: Sam McCall <sam.mccall@gmail.com>
Date: Mon Dec 4 10:08:45 2017 +0000

[clangd] GlobalCompilationDatabase interface changes

The extra-flags are still supported, see the test for example. They're not a full compilation command, just extra flags.

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

Thanks for your response,
As it stands right now we will not run the indexer in Clangd for our use case, and it's unclear if we can even assemble a set of compile commands, so we would like to provide the commands when a file is open. We might be interested in a "didChangeConfiguration" message extension in the future (ideally it would be possible to change the subset of the constructed compilation database).

Sounds good, I think sending it in didOpen makes sense then. And yes, I agree that we would need to support changing a subset of commands through didChangeConfiguration, eventually!

Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message?

We don't yet :) But we will need to send the information per-file through it instead of using the compile_commands.json. Right now, what is supported in the didChangeConfiguration is pointing to a different compile_commands.json, in order to support multiple configuration (actually pointing to the folder containing the json file).

All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen.

EDIT: Well, provided we would find a way how not to parse the file twice.

The idea would be to send the per-file commands to clangd *before* anything is opened. So the didOpen would just read the latest command in memory. And subsequent changes to commands would be communicated with didChangeConfiguration and then files would get reparsed. I'm actually thinking we might want to send the commands in the "initialize" request for all the initial commands and then update them with didChangeConfiguration whenever they change. That way, there is no risk for reparsing as we should not do anything (indexing!) before the initialize.
But it doesn't sounds like you need this right now :)

I reconsidered our needs. We would actually want to use the 'didChangeConfiguration' extension right now as well. I will work on a patch for it that is based on top of this one :)

Alex, I am just wondering if we shouldn't rather create another implementation of GlobalCompilationDatabase interface (something like InMemoryGlobalCompilationDatabase), add it to ClangdServer and use it as the first place to be searched in ClangdServer::getCompileCommand(PathRef File). What do you think?

We can certainly create another class, but I'd be against adding it into ClangdServer. It makes sense to have just one CDB in there. If we had our own server instead of ClangdLSP then this class would be really useful, but for now this solution is probably preferable.

The extensions itself seems like a reasonable way to provide compile commands for the individual files. In case didChangeConfiguration does not work for you for some reason, happy to take a look at this change too.
One drawback of using didChangeConfiguration for compile commands is that it forces global invalidation, which may be a bad thing if your client tracks compile commands on a smaller scale and can send requests to invalidate individual files. However, if you're happy with it, we might get away with not adding yet another mechanism to pass compile commands to clangd, which is definitely a good thing!

The extensions itself seems like a reasonable way to provide compile commands for the individual files. In case didChangeConfiguration does not work for you for some reason, happy to take a look at this change too.
One drawback of using didChangeConfiguration for compile commands is that it forces global invalidation, which may be a bad thing if your client tracks compile commands on a smaller scale and can send requests to invalidate individual files. However, if you're happy with it, we might get away with not adding yet another mechanism to pass compile commands to clangd, which is definitely a good thing!

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

Why do we need both? Can we have only the one from D49758 and send two requests (compile command changed for file 'X' + didOpen('X')) whenever a new file gets added?

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

Why do we need both? Can we have only the one from D49758 and send two requests (compile command changed for file 'X' + didOpen('X')) whenever a new file gets added?

The didChangeConfiguration command would re-parse all the open documents, which is something we want to avoid. I guess one solution is to avoid reparsing anything when the updated file is not in the database in the first place. I'll work on that.

arphaman abandoned this revision.Jul 26 2018, 8:52 AM

Abandoned in favor of https://reviews.llvm.org/D49758 (will update it today).