[clangd] Add HeaderInsertion yaml config option
#128503
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: Mythreya (MythreyaK) ChangesThis is the yaml config equivalent of Fix for issue Full diff: https://github.com/llvm/llvm-project/pull/128503.diff 9 Files Affected:
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 52be15d3da936..9027ae855e409 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -455,6 +455,7 @@ void ClangdServer::codeComplete(PathRef File, Position Pos,
CodeCompleteOpts.MainFileSignals = IP->Signals;
CodeCompleteOpts.AllScopes = Config::current().Completion.AllScopes;
CodeCompleteOpts.ArgumentLists = Config::current().Completion.ArgumentLists;
+ CodeCompleteOpts.InsertIncludes = Config::current().HeaderInsertion.Policy;
// FIXME(ibiryukov): even if Preamble is non-null, we may want to check
// both the old and the new version in case only one of them matches.
CodeCompleteResult Result = clangd::codeComplete(
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index a8182ce98ebe0..5db8eeaee1027 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -294,7 +294,8 @@ struct CompletionCandidate {
std::optional<llvm::StringRef>
headerToInsertIfAllowed(const CodeCompleteOptions &Opts,
CodeCompletionContext::Kind ContextKind) const {
- if (Opts.InsertIncludes == CodeCompleteOptions::NeverInsert ||
+ if (Opts.InsertIncludes ==
+ CodeCompleteOptions::IncludeInsertion::NeverInsert ||
RankedIncludeHeaders.empty() ||
!contextAllowsHeaderInsertion(ContextKind))
return std::nullopt;
diff --git a/clang-tools-extra/clangd/CodeComplete.h b/clang-tools-extra/clangd/CodeComplete.h
index bb2ebd9478645..8df0637d8aae6 100644
--- a/clang-tools-extra/clangd/CodeComplete.h
+++ b/clang-tools-extra/clangd/CodeComplete.h
@@ -71,10 +71,8 @@ struct CodeCompleteOptions {
/// Whether to present doc comments as plain-text or markdown.
MarkupKind DocumentationFormat = MarkupKind::PlainText;
- enum IncludeInsertion {
- IWYU,
- NeverInsert,
- } InsertIncludes = IncludeInsertion::IWYU;
+ using IncludeInsertion = Config::HeaderInsertionPolicy;
+ Config::HeaderInsertionPolicy InsertIncludes = IncludeInsertion::IWYU;
/// Whether include insertions for Objective-C code should use #import instead
/// of #include.
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index 586d031d58481..eb33f1b8c79ae 100644
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -151,6 +151,15 @@ struct Config {
ArgumentListsPolicy ArgumentLists = ArgumentListsPolicy::FullPlaceholders;
} Completion;
+ enum class HeaderInsertionPolicy {
+ IWYU, // Include what you use
+ NeverInsert // Never insert headers as part of code completion
+ };
+
+ struct {
+ HeaderInsertionPolicy Policy = HeaderInsertionPolicy::IWYU;
+ } HeaderInsertion;
+
/// Configures hover feature.
struct {
/// Whether hover show a.k.a type.
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index aa2561e081047..fb02b1baaebc4 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -685,6 +685,17 @@ struct FragmentCompiler {
C.Completion.ArgumentLists = *Val;
});
}
+ if (F.HeaderInsertion) {
+ if (auto Val =
+ compileEnum<Config::HeaderInsertionPolicy>("HeaderInsertion",
+ *F.HeaderInsertion)
+ .map("IWYU", Config::HeaderInsertionPolicy::IWYU)
+ .map("Never", Config::HeaderInsertionPolicy::NeverInsert)
+ .value())
+ Out.Apply.push_back([Val](const Params &, Config &C) {
+ C.HeaderInsertion.Policy = *Val;
+ });
+ }
}
void compile(Fragment::HoverBlock &&F) {
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index 9535b20253b13..c4c7515c5c25e 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -333,6 +333,14 @@ struct Fragment {
/// Delimiters: empty pair of delimiters "()" or "<>"
/// FullPlaceholders: full name of both type and parameter
std::optional<Located<std::string>> ArgumentLists;
+ /// Add #include directives when accepting code completions. Config
+ /// equivalent of the CLI option '--header-insertion'
+ /// Valid values are enum Config::HeaderInsertionPolicy values:
+ /// "IWYU": Include what you use. Insert the owning header for top-level
+ /// symbols, unless the header is already directly included or the
+ /// symbol is forward-declared
+ /// "NeverInsert": Never insert headers
+ std::optional<Located<std::string>> HeaderInsertion;
};
CompletionBlock Completion;
diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index 95cc5c1f9f1cf..34fe61afddadf 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -241,6 +241,10 @@ class Parser {
if (auto ArgumentLists = scalarValue(N, "ArgumentLists"))
F.ArgumentLists = *ArgumentLists;
});
+ Dict.handle("HeaderInsertion", [&](Node &N) {
+ if (auto HeaderInsertion = scalarValue(N, "HeaderInsertion"))
+ F.HeaderInsertion = *HeaderInsertion;
+ });
Dict.parse(N);
}
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index 714891703b6f3..44fd1d450ec64 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -257,13 +257,13 @@ opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{
desc("Add #include directives when accepting code completions"),
init(CodeCompleteOptions().InsertIncludes),
values(
- clEnumValN(CodeCompleteOptions::IWYU, "iwyu",
+ clEnumValN(CodeCompleteOptions::IncludeInsertion::IWYU, "iwyu",
"Include what you use. "
"Insert the owning header for top-level symbols, unless the "
"header is already directly included or the symbol is "
"forward-declared"),
clEnumValN(
- CodeCompleteOptions::NeverInsert, "never",
+ CodeCompleteOptions::IncludeInsertion::NeverInsert, "never",
"Never insert #include directives as part of code completion")),
};
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index b12f8275b8a26..042ad73b8a280 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -882,7 +882,8 @@ TEST(CompletionTest, IncludeInsertionPreprocessorIntegrationTests) {
ElementsAre(AllOf(named("X"), insertInclude("\"bar.h\""))));
// Can be disabled via option.
CodeCompleteOptions NoInsertion;
- NoInsertion.InsertIncludes = CodeCompleteOptions::NeverInsert;
+ NoInsertion.InsertIncludes =
+ CodeCompleteOptions::IncludeInsertion::NeverInsert;
Results = completions(TU, Test.point(), {Sym}, NoInsertion);
EXPECT_THAT(Results.Completions,
ElementsAre(AllOf(named("X"), Not(insertInclude()))));
|
MythreyaK
force-pushed
the
mythreyak/cland-header-insertion-yaml-config
branch
from
193219f to
8a0714c
Compare
|
Example config Completion:
HeaderInsertion: Never |
Adding @HighCommander4 as reviewer. No new tests yet since I wasn't sure about it. |
|
Ping |
|
(Recording myself as reviewer. I have a backlog of several patches in my review queue, will get to this as time permits.) |
There was a problem hiding this comment.
Thanks for the patch! The general approach here looks good.
One issue that I see with the current implementation is that it makes the command-line flag --header-insertion=never have no effect. That's because, while the value of the command-line flag will be copied into the code completion options here, that will then be overwritten in codeComplete() where we now do:
CodeCompleteOpts.InsertIncludes = Config::current().HeaderInsertion.Policy;
If the user didn't specify a HeaderInsertion config option, the value of Config::current().HeaderInsertion.Policy will be IWYU (the default specified in Config.h). and we overwrite the NeverInsert value with IWYU.
This would break users who have --header-insertion=never specified today and expect it to continue to work.
To get the command line flag to interact with the config option better, we need to handle it in FlagsConfigProvider, which gives the command-line flag the ability to influence the value of Config::current().HeaderInsertion.Policy.
We have a few options regarding the precedence, but I think the simplest to implement is to handle it similar to --background-index here: if the flag's value is NeverInsert, then write that into the config object, otherwise leave it alone.
The resulting behaviour will be:
- If the user specifies
--header-insertion=neveron the command line, it takes precedence and disables header insertion globally, regardless of any config options. - If the user doesn't specify
--header-insertion=neveron the command line, the config file option is used if specified, falling backing toIWYUas the default.
I think that should work well enough in practice -- let me know what you think.
Oh right! I completely forgot about that.
Clarification, so if the user has
Sounds good, I'll update my patch shortly. |
Yes. I realize that's asymmetric; unfortunately, LLVM's command-line argument framework doesn't allow us to distinguish between "the option's value is In practice, I think this is fine because explicitly passing |
MythreyaK
force-pushed
the
mythreyak/cland-header-insertion-yaml-config
branch
from
8a0714c to
b9710d1
Compare
This is the yaml config equivalent of `--header-insertion` CLI option
MythreyaK
force-pushed
the
mythreyak/cland-header-insertion-yaml-config
branch
from
b9710d1 to
f2e4b19
Compare
|
@MythreyaK 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 is the yaml config equivalent of
--header-insertionCLIFix for issue