Add --project-root to clangd #155905
Add --project-root to clangd #155905
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: Dominicentek (Dominicentek) ChangesAdds an ability to change the current working directory for fallback commands. Full diff: https://github.com/llvm/llvm-project/pull/155905.diff 8 Files Affected:
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index ac1e9aa5f0ff1..51230b4506b1a 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -208,6 +208,7 @@ ClangdServer::Options::operator TUScheduler::Options() const {
Opts.UpdateDebounce = UpdateDebounce;
Opts.ContextProvider = ContextProvider;
Opts.PreambleThrottler = PreambleThrottler;
+ Opts.FallbackProjectRoot = FallbackProjectRoot;
return Opts;
}
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 4a1eae188f7eb..2c56d6f7e6d6c 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -152,6 +152,10 @@ class ClangdServer {
/// FIXME: If not set, should use the current working directory.
std::optional<std::string> WorkspaceRoot;
+ /// If set, fallback command uses this path as its current working directory
+ /// instead of the file's parent path.
+ std::optional<std::string> FallbackProjectRoot;
+
/// The resource directory is used to find internal headers, overriding
/// defaults and -resource-dir compiler flag).
/// If std::nullopt, ClangdServer calls
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
index c6afd0bc07cbd..b73697d4ee7e5 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -55,7 +55,7 @@ void actOnAllParentDirectories(PathRef FileName,
} // namespace
tooling::CompileCommand
-GlobalCompilationDatabase::getFallbackCommand(PathRef File) const {
+GlobalCompilationDatabase::getFallbackCommand(PathRef File, std::optional<std::string> ProjectRoot) const {
std::vector<std::string> Argv = {"clang"};
// Clang treats .h files as C by default and files without extension as linker
// input, resulting in unhelpful diagnostics.
@@ -64,7 +64,7 @@ GlobalCompilationDatabase::getFallbackCommand(PathRef File) const {
if (FileExtension.empty() || FileExtension == ".h")
Argv.push_back("-xobjective-c++-header");
Argv.push_back(std::string(File));
- tooling::CompileCommand Cmd(llvm::sys::path::parent_path(File),
+ tooling::CompileCommand Cmd(ProjectRoot ? *ProjectRoot : llvm::sys::path::parent_path(File),
llvm::sys::path::filename(File), std::move(Argv),
/*Output=*/"");
Cmd.Heuristic = "clangd fallback";
@@ -797,8 +797,8 @@ OverlayCDB::getCompileCommand(PathRef File) const {
return Cmd;
}
-tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File) const {
- auto Cmd = DelegatingCDB::getFallbackCommand(File);
+tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File, std::optional<std::string> ProjectRoot) const {
+ auto Cmd = DelegatingCDB::getFallbackCommand(File, ProjectRoot);
std::lock_guard<std::mutex> Lock(Mutex);
Cmd.CommandLine.insert(Cmd.CommandLine.end(), FallbackFlags.begin(),
FallbackFlags.end());
@@ -877,10 +877,10 @@ DelegatingCDB::getProjectModules(PathRef File) const {
return Base->getProjectModules(File);
}
-tooling::CompileCommand DelegatingCDB::getFallbackCommand(PathRef File) const {
+tooling::CompileCommand DelegatingCDB::getFallbackCommand(PathRef File, std::optional<std::string> ProjectRoot) const {
if (!Base)
- return GlobalCompilationDatabase::getFallbackCommand(File);
- return Base->getFallbackCommand(File);
+ return GlobalCompilationDatabase::getFallbackCommand(File, ProjectRoot);
+ return Base->getFallbackCommand(File, ProjectRoot);
}
bool DelegatingCDB::blockUntilIdle(Deadline D) const {
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
index 1d636d73664be..5d1b5cb632154 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -55,7 +55,7 @@ class GlobalCompilationDatabase {
/// Makes a guess at how to build a file.
/// The default implementation just runs clang on the file.
/// Clangd should treat the results as unreliable.
- virtual tooling::CompileCommand getFallbackCommand(PathRef File) const;
+ virtual tooling::CompileCommand getFallbackCommand(PathRef File, std::optional<std::string> ProjectRoot = std::nullopt) const;
/// If the CDB does any asynchronous work, wait for it to complete.
/// For use in tests.
@@ -86,7 +86,7 @@ class DelegatingCDB : public GlobalCompilationDatabase {
std::unique_ptr<ProjectModules>
getProjectModules(PathRef File) const override;
- tooling::CompileCommand getFallbackCommand(PathRef File) const override;
+ tooling::CompileCommand getFallbackCommand(PathRef File, std::optional<std::string> ProjectRoot = std::nullopt) const override;
bool blockUntilIdle(Deadline D) const override;
@@ -200,7 +200,7 @@ class OverlayCDB : public DelegatingCDB {
std::optional<tooling::CompileCommand>
getCompileCommand(PathRef File) const override;
- tooling::CompileCommand getFallbackCommand(PathRef File) const override;
+ tooling::CompileCommand getFallbackCommand(PathRef File, std::optional<std::string> ProjectRoot = std::nullopt) const override;
/// Sets or clears the compilation command for a particular file.
/// Returns true if the command was changed (including insertion and removal),
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 035e5e63d8fbb..3dc53767e0ea4 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -723,6 +723,7 @@ class ASTWorker {
const GlobalCompilationDatabase &CDB;
/// Callback invoked when preamble or main file AST is built.
ParsingCallbacks &Callbacks;
+ std::optional<std::string> FallbackProjectRoot;
Semaphore &Barrier;
/// Whether the 'onMainAST' callback ran for the current FileInputs.
@@ -840,13 +841,14 @@ ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB,
: IdleASTs(LRUCache), HeaderIncluders(HeaderIncluders), RunSync(RunSync),
UpdateDebounce(Opts.UpdateDebounce), FileName(FileName),
ContextProvider(Opts.ContextProvider), CDB(CDB), Callbacks(Callbacks),
+ FallbackProjectRoot(Opts.FallbackProjectRoot),
Barrier(Barrier), Done(false), Status(FileName, Callbacks),
PreamblePeer(FileName, Callbacks, Opts.StorePreamblesInMemory, RunSync,
Opts.PreambleThrottler, Status, HeaderIncluders, *this) {
// Set a fallback command because compile command can be accessed before
// `Inputs` is initialized. Other fields are only used after initialization
// from client inputs.
- FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+ FileInputs.CompileCommand = CDB.getFallbackCommand(FileName, FallbackProjectRoot);
}
ASTWorker::~ASTWorker() {
@@ -888,7 +890,7 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
if (Cmd)
Inputs.CompileCommand = std::move(*Cmd);
else
- Inputs.CompileCommand = CDB.getFallbackCommand(FileName);
+ Inputs.CompileCommand = CDB.getFallbackCommand(FileName, FallbackProjectRoot);
bool InputsAreTheSame =
std::tie(FileInputs.CompileCommand, FileInputs.Contents) ==
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index d0da20310a8b2..581a639646527 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -236,6 +236,10 @@ class TUScheduler {
/// Typically to inject per-file configuration.
/// If the path is empty, context sholud be "generic".
std::function<Context(PathRef)> ContextProvider;
+
+ /// If set, fallback command uses this path as its current working directory
+ /// instead of the file's parent path.
+ std::optional<std::string> FallbackProjectRoot;
};
TUScheduler(const GlobalCompilationDatabase &CDB, const Options &Opts,
diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp
index df8d075e80596..8d49b82d2ca53 100644
--- a/clang-tools-extra/clangd/tool/Check.cpp
+++ b/clang-tools-extra/clangd/tool/Check.cpp
@@ -187,7 +187,7 @@ class Checker {
Cmd.Heuristic.empty() ? "from CDB" : Cmd.Heuristic, Cmd.Directory,
printArgv(Cmd.CommandLine));
} else {
- Cmd = CDB->getFallbackCommand(File);
+ Cmd = CDB->getFallbackCommand(File, Opts.FallbackProjectRoot);
log("Generic fallback command is: [{0}] {1}", Cmd.Directory,
printArgv(Cmd.CommandLine));
}
@@ -502,7 +502,7 @@ bool check(llvm::StringRef File, const ThreadsafeFS &TFS,
config::DiagnosticCallback Diag) const override {
config::Fragment F;
// If we're timing clang-tidy checks, implicitly disabling the slow ones
- // is counterproductive!
+ // is counterproductive!
if (CheckTidyTime.getNumOccurrences())
F.Diagnostics.ClangTidy.FastCheckFilter.emplace("None");
return {std::move(F).compile(Diag)};
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index f287439f10cab..75d71c5a78f45 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -499,6 +499,14 @@ opt<bool> EnableConfig{
init(true),
};
+opt<Path> ProjectRoot{
+ "project-root",
+ cat(Misc),
+ desc("Path to use as the current working directory for fallback commands."),
+ init(""),
+ ValueOptional,
+};
+
opt<bool> UseDirtyHeaders{"use-dirty-headers", cat(Misc),
desc("Use files open in the editor when parsing "
"headers instead of reading from the disk"),
@@ -906,6 +914,8 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
}
if (!ResourceDir.empty())
Opts.ResourceDir = ResourceDir;
+ if (!ProjectRoot.empty())
+ Opts.FallbackProjectRoot = ProjectRoot;
Opts.BuildDynamicSymbolIndex = true;
#if CLANGD_ENABLE_REMOTE
if (RemoteIndexAddress.empty() != ProjectRoot.empty()) {
|
|
Hi @Dominicentek! Thanks for submitting this PR. The topic of this PR actually touches on a set of improvements I've been wanting to make to clangd for a long time, called "strong workspace mode". Some background on this proposed mode can be found in clangd/clangd#907 (with credit to @sam-mccall for the name and initial description). Basically, "strong workspace mode" would be a mode of operation of clangd where an instance of clangd is more strongly coupled to a particular workspace/project, and clangd's default behaviours are adjusted to reflect this. The specific behaviours that I have in mind are:
In addition, there is some discussion in clangd/clangd#1341 that suggests to me that it might make sense for "strong workspace mode" to be something the user opts into at the client level (for example, in vscode, using a setting provided by the I'd be curious to hear your opinion on the following:
|
|
Hi, thanks for reaching out. I made this pull request as my editor's (Zed) built-in clangd client doesn't set the project root directory as the working directory for fallback commands. VSCode does actually have this behavior. Since I was relying on fallback commands rather than compile_commands.json and couldn't really find a way to make clangd behave like this with Zed, I made it so you can change the working directory for fallback commands via the As for the "strong workspace mode", I am quite fond of this idea. Though I'd like to see this being done at the server level (with a CLI flag), since both VSCode and Zed let you customize CLI arguments for clangd, but Zed doesn't really have any clangd-specific settings, as opposed to VSCode. |
Do you know how VSCode does this? |
|
No, I'm not entirely sure. |
I tried to reproduce the described behaviour in VSCode, and I haven't been able to. Here's what I tried specifically:
The clangd logs are showing: indicating that it's using the source file's containing directory ( Could you elaborate on what VSCode behaviour you were observing? |
|
Ah I apologize, I was misremembering. I used |
|
Thanks for the clarification. I have a couple of follow-up questions about using clangd with Zed, with a view to understanding what sort of constraints we have on how to configure this behaviour:
|
|
I'm seeing the client include I take it this is the same directory you were envisioning passing to
Ok, thanks for checking. So given all that, what do you think about the following approach:
|
|
Yep, sounds great! |
Ok, cool. Would you like to update this PR to implement this revised approach? I'd be happy to review and (if no one else objects) approve it. |
|
Alright, I've made the changes. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
HighCommander4
left a comment
HighCommander4
left a comment
There was a problem hiding this comment.
I haven't had a chance to take a very detailed look, but some high-level feedback:
- We shouldn't assume the current working directory (
current_path) will be the workspace root, even if that happens to be the case today. Instead, let's propagate therootUri(orrootPath) that the client sent in theinitializerequest to the place where we need it. We already propagate it intoOpts.WorkspaceRoot, we can plumb it further from there intoGlobalCompilationDatabase. - Rather than changing the
GlobalCompilationDatabase::getFallbackCommandsignature, let's provide the flag to theGlobalCompilationDatabasevia its constructor (and derived classes' constructors).- In fact, since we need to tell it both the bool and the path, maybe we can group them into an
optional<string>, which would contain the path in strong workspace mode, and be empty in the default mode.
- In fact, since we need to tell it both the bool and the path, maybe we can group them into an
- We should add some sort of test coverage. We can add a unit test similar to
GlobalCompilationDatabaseTest.FallbackCommandwhich makes assertions aboutCmd.Directory.
HighCommander4
left a comment
HighCommander4
left a comment
There was a problem hiding this comment.
Thanks, I think the general shape of the patch looks pretty good now.
| /// If the path is empty, context sholud be "generic". | ||
| std::function<Context(PathRef)> ContextProvider; | ||
|
|
||
| bool test; |
There was a problem hiding this comment.
Leftover debug code?
| /// Provides compilation arguments used for parsing C and C++ files. | ||
| class GlobalCompilationDatabase { | ||
| public: | ||
| GlobalCompilationDatabase(std::optional<std::string> WorkingDirectory) |
There was a problem hiding this comment.
I would give this parameter a default to minimize the disruption caused by this change. (I'm not just talking about test code in this repo; I think this class has out-of-tree subclasses as well.)
| } | ||
|
|
||
| protected: | ||
| std::optional<std::string> WorkingDirectory; |
There was a problem hiding this comment.
Since this only applies to fallback commands, let's call it FallbackWorkingDirectory. Likewise for the field in DirectoryBasedGlobalCompilationDatabase::Options, the method that sets it, etc.
| public: | ||
| DelegatingCDB(const GlobalCompilationDatabase *Base); | ||
| DelegatingCDB(std::unique_ptr<GlobalCompilationDatabase> Base); | ||
| DelegatingCDB(const GlobalCompilationDatabase *Base, |
There was a problem hiding this comment.
Likewise here, we may want to give these added parameters a default
| std::optional<std::string> WorkingDirectory; | ||
|
|
||
| void | ||
| applyWorkingDirectory(const std::optional<std::string> &&WorkingDirectory); |
There was a problem hiding this comment.
What is the purpose of using const && here?
| "the workspace.\n" | ||
| "When enabled, fallback commands use the workspace directory as their " | ||
| "working directory instead of the parent folder."), | ||
| init(false), |
There was a problem hiding this comment.
Let's make this option hidden for now (example), until the feature is more fleshed out with other behaviours. (This just means it won't show up in clangd --help, only in clangd --help-hidden.)
| opt<bool> StrongWorkspaceMode{ | ||
| "strong-workspace-mode", | ||
| cat(Features), | ||
| desc("An alternate mode of operation for clangd, operating more closely to " |
There was a problem hiding this comment.
Suggested reword: "operating more closely to the workspace" --> "where the clangd instance is used to edit a single workspace".
| if (WorkingDirectory) | ||
| this->WorkingDirectory = *WorkingDirectory; | ||
| else { | ||
| SmallString<256> CWD; |
There was a problem hiding this comment.
It's worth a comment explaining why we need a fallback here. Something like "the user asked for strong workspace mode, but the client didn't provide a workspace path, so use the working directory as a fallback".
There was a problem hiding this comment.
I was wondering if an error instead of a silent fallback helps with immediate user feedback, since this is an opt-in feature. Strong workspace's precondition is that a the directory should be set anyway.
There was a problem hiding this comment.
I could go either way on this. The advantage of a fallback is that it can help work around a client that's unhelpfully failing to set rootUri, for which you as a user may not have much other recourse.
There was a problem hiding this comment.
I tweaked the comment wording a bit here. I think it's important to mention that it's the LSP client not providing a workspace path that necessitates a fallback (or error) here, i.e. something we as the server can't control.
|
For multi-workspace (not vscode workspace) projects, say a monorepo where each subproject is built individually (and therefore separate build dirs and compile_commands.json), is the idea that each subproject will (should?) now have its own clangd instance running? It mostly just worked in my experience so far, but wanted to understand this better. I am not completely sure how this might interact with vscode workspaces. |
I would say this patch doesn't make any changes to how clangd works on, or should be used with, such projects. This patch is adding (the beginnings of) a non-default mode of operation ("strong workspace mode") that users need to opt into. If you don't opt into it, everything will work just as it did before. For a project of the sort you describe, even if you opt into "strong workspace mode" (and after the remaining behaviours described in this comment) are implemented, not a lot is likely to change:
It has separately been proposed that clients like vscode-clangd gain the ability to manage multiple clangd instances and launch one per "workspace root" (in the sense of vscode's multi-root workspaces). For example, one such proposal is being pursued in clangd/vscode-clangd#810. If such a proposal is accepted, then using it for a project of the sort you describe (i.e. making each sub-project a "workspace root") would be a separate knob you can opt into. I think it would come with both advantages and disadvantages. (For example, clangd's index would no longer be able to cross-reference symbols between sub-projects. Depending on how your sub-projects are related, this may be a desirable change or an undesirable one.) If you do opt into such a hypothetical multi-root workspace feature, you could then also opt into "strong workspace mode" for each sub-project, and that may bring more benefits than using "strong workspace mode" with a single workspace root for your overall project would have. (For example, if you have an ARM sub-project and an x86 sub-project, and you open a system header from a source file in your ARM sub-project, clangd will automatically use a compile command from one of your ARM source files, more reliably than it does today.) |
|
That sounds reasonable, thank you! Two more questions,
For the example I was suggesting, only the subprojects have the compilation database, but
Does this impact indexing files located outside of the projects but are
So as long as an open file is in one of these |
I guess in retrospect this is not an obvious choice, e.g. it's not how So we could say But I think it would more user-friendly for strong workspace mode to use |
I think clangd today stores index data for such files in
Other than the index storage location being |
HighCommander4
left a comment
HighCommander4
left a comment
There was a problem hiding this comment.
The patch updates look good to me, just one final nit: can we use the terminology FallbackWorkingDirectory consistently, i.e. in the name of the CDBOpts field, the name of the applyWorkingDirectory method, the various constructor parameters etc.?
|
Hi @HighCommander4, I apologize if you're busy, but is there any progress on this patch? |
HighCommander4
left a comment
HighCommander4
left a comment
There was a problem hiding this comment.
Thanks! LGTM with some minor tweaks which I went ahead and made.
| if (WorkingDirectory) | ||
| this->WorkingDirectory = *WorkingDirectory; | ||
| else { | ||
| SmallString<256> CWD; |
There was a problem hiding this comment.
I tweaked the comment wording a bit here. I think it's important to mention that it's the LSP client not providing a workspace path that necessitates a fallback (or error) here, i.e. something we as the server can't control.
|
@Dominicentek Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
This might have caused buildbot errors. Running some builds to confirm, but in case it looks familiar posting it now: https://lab.llvm.org/buildbot/#/builders/164/builds/16465/steps/11/logs/stdio |
#155905)" This reverts commit 2fa4927. This caused sanitizer bots to fail and sanitizer errors to show up in our downstream testing: ``` [ RUN ] LSPTest.DiagnosticsHeaderSaved <<< initialize: {} <-- initialize(0) third_party/llvm/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp:557:14: runtime error: load of value 112, which is not a valid value for type 'bool' ``` With ASan at -O1.
|
Seeing the same thing with ASan at -O1: Only I've reverted this in 01c4eb5. |
Do you happen to have the full ASAN output for this? It may allow us to understand the issue without having to do a local ASAN build. |
is the stack trace I got. ASan didn't print out anything beyond that. |
|
Ah, I see, so it's actually a UBSan error, and it's complaining at the read of Should be a simple fix, just need to give this field a class member initializer. @Dominicentek heads up that this has been reverted and you'll need to resubmit with this fix to move forward with this change |
|
Yeah thanks, I'm aware. |
6 participants
Adds an ability to change the current working directory for fallback commands.