[clangd] Allow specifying what headers are always included via "" or <> #67749
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd ChangesProjects can now add config fragments like this to their .clangd: Style:
QuotedHeaders: "src/.*"
AngledHeaders: ["path/sdk/.*", "third-party/.*"]to force headers inserted via the --header-insertion=iwyu mode matching at least one of the regexes to have <> (AngledHeaders) or "" (QuotedHeaders) around them, respectively. For other headers (and in conflicting cases where both styles have a matching regex), the current system header detection remains. Based on https://reviews.llvm.org/D145843 This solution does not affect other clang tools like clang-format. CC @HighCommander4 @adkaster. I have no merge rights so someone else needs to commit this. Patch is 20.08 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67749.diff 12 Files Affected:
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 70c4d7e65b76db3..388e032e160474a 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -21,6 +21,7 @@
#include "AST.h"
#include "CodeCompletionStrings.h"
#include "Compiler.h"
+#include "Config.h"
#include "ExpectedTypes.h"
#include "Feature.h"
#include "FileDistance.h"
@@ -786,8 +787,8 @@ SpecifiedScope getQueryScopes(CodeCompletionContext &CCContext,
llvm::StringRef SpelledSpecifier = Lexer::getSourceText(
CharSourceRange::getCharRange(SemaSpecifier->getRange()),
CCSema.SourceMgr, clang::LangOptions());
- if (SpelledSpecifier.consume_front("::"))
- Scopes.QueryScopes = {""};
+ if (SpelledSpecifier.consume_front("::"))
+ Scopes.QueryScopes = {""};
Scopes.UnresolvedQualifier = std::string(SpelledSpecifier);
// Sema excludes the trailing "::".
if (!Scopes.UnresolvedQualifier->empty())
@@ -1540,7 +1541,7 @@ class CodeCompleteFlow {
CompletionPrefix HeuristicPrefix;
std::optional<FuzzyMatcher> Filter; // Initialized once Sema runs.
Range ReplacedRange;
- std::vector<std::string> QueryScopes; // Initialized once Sema runs.
+ std::vector<std::string> QueryScopes; // Initialized once Sema runs.
std::vector<std::string> AccessibleScopes; // Initialized once Sema runs.
// Initialized once QueryScopes is initialized, if there are scopes.
std::optional<ScopeDistance> ScopeProximity;
@@ -1599,7 +1600,9 @@ class CodeCompleteFlow {
Inserter.emplace(
SemaCCInput.FileName, SemaCCInput.ParseInput.Contents, Style,
SemaCCInput.ParseInput.CompileCommand.Directory,
- &Recorder->CCSema->getPreprocessor().getHeaderSearchInfo());
+ &Recorder->CCSema->getPreprocessor().getHeaderSearchInfo(),
+ Config::current().Style.QuotedHeaders,
+ Config::current().Style.AngledHeaders);
for (const auto &Inc : Includes.MainFileIncludes)
Inserter->addExisting(Inc);
@@ -1682,7 +1685,9 @@ class CodeCompleteFlow {
auto Style = getFormatStyleForFile(FileName, Content, TFS);
// This will only insert verbatim headers.
Inserter.emplace(FileName, Content, Style,
- /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+ /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr,
+ Config::current().Style.QuotedHeaders,
+ Config::current().Style.AngledHeaders);
auto Identifiers = collectIdentifiers(Content, Style);
std::vector<RawIdentifier> IdentifierResults;
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index daae8d1c0c833eb..5d70ed3376715e5 100644
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -121,6 +121,10 @@ struct Config {
// declarations, always spell out the whole name (with or without leading
// ::). All nested namespaces are affected as well.
std::vector<std::string> FullyQualifiedNamespaces;
+
+ // List of regexes for inserting certain headers with <> or "".
+ std::vector<std::function<bool(llvm::StringRef)>> QuotedHeaders;
+ std::vector<std::function<bool(llvm::StringRef)>> AngledHeaders;
} Style;
/// Configures code completion feature.
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index d4ff7ae3181bc3d..b41c8cde15dbf9a 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -483,6 +483,72 @@ struct FragmentCompiler {
FullyQualifiedNamespaces.begin(), FullyQualifiedNamespaces.end());
});
}
+
+// TODO: Share this code with Diagnostics.Includes.IgnoreHeader
+#ifdef CLANGD_PATH_CASE_INSENSITIVE
+ static llvm::Regex::RegexFlags Flags = llvm::Regex::IgnoreCase;
+#else
+ static llvm::Regex::RegexFlags Flags = llvm::Regex::NoFlags;
+#endif
+ {
+ auto Filters = std::make_shared<std::vector<llvm::Regex>>();
+ for (auto &HeaderPattern : F.AngledHeaders) {
+ // Anchor on the right.
+ std::string AnchoredPattern = "(" + *HeaderPattern + ")$";
+ llvm::Regex CompiledRegex(AnchoredPattern, Flags);
+ std::string RegexError;
+ if (!CompiledRegex.isValid(RegexError)) {
+ diag(Warning,
+ llvm::formatv("Invalid regular expression '{0}': {1}",
+ *HeaderPattern, RegexError)
+ .str(),
+ HeaderPattern.Range);
+ continue;
+ }
+ Filters->push_back(std::move(CompiledRegex));
+ }
+ if (Filters->empty())
+ return;
+ auto Filter = [Filters](llvm::StringRef Path) {
+ for (auto &Regex : *Filters)
+ if (Regex.match(Path))
+ return true;
+ return false;
+ };
+ Out.Apply.push_back([Filter](const Params &, Config &C) {
+ C.Style.AngledHeaders.emplace_back(Filter);
+ });
+ }
+
+ {
+ auto Filters = std::make_shared<std::vector<llvm::Regex>>();
+ for (auto &HeaderPattern : F.QuotedHeaders) {
+ // Anchor on the right.
+ std::string AnchoredPattern = "(" + *HeaderPattern + ")$";
+ llvm::Regex CompiledRegex(AnchoredPattern, Flags);
+ std::string RegexError;
+ if (!CompiledRegex.isValid(RegexError)) {
+ diag(Warning,
+ llvm::formatv("Invalid regular expression '{0}': {1}",
+ *HeaderPattern, RegexError)
+ .str(),
+ HeaderPattern.Range);
+ continue;
+ }
+ Filters->push_back(std::move(CompiledRegex));
+ }
+ if (Filters->empty())
+ return;
+ auto Filter = [Filters](llvm::StringRef Path) {
+ for (auto &Regex : *Filters)
+ if (Regex.match(Path))
+ return true;
+ return false;
+ };
+ Out.Apply.push_back([Filter](const Params &, Config &C) {
+ C.Style.QuotedHeaders.emplace_back(Filter);
+ });
+ }
}
void appendTidyCheckSpec(std::string &CurSpec,
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index a59d4124b0828ac..02ef30949629f7a 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -289,6 +289,19 @@ struct Fragment {
// ::). All nested namespaces are affected as well.
// Affects availability of the AddUsing tweak.
std::vector<Located<std::string>> FullyQualifiedNamespaces;
+
+ /// List of regexes for headers that should always be included with a
+ /// ""-style include. By default, and in case of a conflict with
+ /// AngledHeaders (i.e. a header matches a regex in both QuotedHeaders and
+ /// AngledHeaders), system headers use <> and non-system headers use "".
+ /// These can match any suffix of the header file in question.
+ std::vector<Located<std::string>> QuotedHeaders;
+ /// List of regexes for headers that should always be included with a
+ /// <>-style include. By default, and in case of a conflict with
+ /// AngledHeaders (i.e. a header matches a regex in both QuotedHeaders and
+ /// AngledHeaders), system headers use <> and non-system headers use "".
+ /// These can match any suffix of the header file in question.
+ std::vector<Located<std::string>> AngledHeaders;
};
StyleBlock Style;
diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index cf3cec472bf8a31..141a897b8ec82ee 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -117,6 +117,14 @@ class Parser {
if (auto Values = scalarValues(N))
F.FullyQualifiedNamespaces = std::move(*Values);
});
+ Dict.handle("QuotedHeaders", [&](Node &N) {
+ if (auto Values = scalarValues(N))
+ F.QuotedHeaders = std::move(*Values);
+ });
+ Dict.handle("AngledHeaders", [&](Node &N) {
+ if (auto Values = scalarValues(N))
+ F.AngledHeaders = std::move(*Values);
+ });
Dict.parse(N);
}
diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp
index 6005069be01160d..bec6aed5bfd6fd2 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -9,6 +9,7 @@
#include "Headers.h"
#include "Preamble.h"
#include "SourceCode.h"
+#include "support/Logger.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Frontend/CompilerInstance.h"
@@ -30,8 +31,7 @@ namespace clangd {
class IncludeStructure::RecordHeaders : public PPCallbacks {
public:
RecordHeaders(const CompilerInstance &CI, IncludeStructure *Out)
- : SM(CI.getSourceManager()),
- Out(Out) {}
+ : SM(CI.getSourceManager()), Out(Out) {}
// Record existing #includes - both written and resolved paths. Only #includes
// in the main file are collected.
@@ -286,11 +286,11 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
assert(InsertedHeader.valid());
if (InsertedHeader.Verbatim)
return InsertedHeader.File;
- bool IsAngled = false;
+ bool IsAngledByDefault = false;
std::string Suggested;
if (HeaderSearchInfo) {
Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
- InsertedHeader.File, BuildDir, IncludingFile, &IsAngled);
+ InsertedHeader.File, BuildDir, IncludingFile, &IsAngledByDefault);
} else {
// Calculate include relative to including file only.
StringRef IncludingDir = llvm::sys::path::parent_path(IncludingFile);
@@ -303,9 +303,33 @@ IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
// FIXME: should we allow (some limited number of) "../header.h"?
if (llvm::sys::path::is_absolute(Suggested))
return std::nullopt;
+ bool IsAngled = false;
+ for (auto Filter : AngledHeaders) {
+ if (Filter(Suggested)) {
+ IsAngled = true;
+ break;
+ }
+ }
+ bool IsQuoted = false;
+ for (auto Filter : QuotedHeaders) {
+ if (Filter(Suggested)) {
+ IsQuoted = true;
+ break;
+ }
+ }
+ // No filters apply, or both filters apply (a bug), use system default.
+ if (IsAngled == IsQuoted) {
+ // Probably a bug in the config regex.
+ if (IsAngled && IsQuoted) {
+ elog("Header '{0}' matches both quoted and angled regexes, default will "
+ "be used.",
+ Suggested);
+ }
+ IsAngled = IsAngledByDefault;
+ }
if (IsAngled)
Suggested = "<" + Suggested + ">";
- else
+ else // if (IsQuoted)
Suggested = "\"" + Suggested + "\"";
return Suggested;
}
diff --git a/clang-tools-extra/clangd/Headers.h b/clang-tools-extra/clangd/Headers.h
index 41cf3de6bba350b..f1ec44422775f9c 100644
--- a/clang-tools-extra/clangd/Headers.h
+++ b/clang-tools-extra/clangd/Headers.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H
+#include "Config.h"
#include "Protocol.h"
#include "SourceCode.h"
#include "index/Symbol.h"
@@ -32,6 +33,8 @@
namespace clang {
namespace clangd {
+
+using HeaderFilter = llvm::ArrayRef<std::function<bool(llvm::StringRef)>>;
/// Returns true if \p Include is literal include like "path" or <path>.
bool isLiteralInclude(llvm::StringRef Include);
@@ -211,10 +214,12 @@ class IncludeInserter {
// include path of non-verbatim header will not be shortened.
IncludeInserter(StringRef FileName, StringRef Code,
const format::FormatStyle &Style, StringRef BuildDir,
- HeaderSearch *HeaderSearchInfo)
+ HeaderSearch *HeaderSearchInfo, HeaderFilter QuotedHeaders,
+ HeaderFilter AngledHeaders)
: FileName(FileName), Code(Code), BuildDir(BuildDir),
HeaderSearchInfo(HeaderSearchInfo),
- Inserter(FileName, Code, Style.IncludeStyle) {}
+ Inserter(FileName, Code, Style.IncludeStyle),
+ QuotedHeaders(QuotedHeaders), AngledHeaders(AngledHeaders) {}
void addExisting(const Inclusion &Inc);
@@ -258,6 +263,8 @@ class IncludeInserter {
HeaderSearch *HeaderSearchInfo = nullptr;
llvm::StringSet<> IncludedHeaders; // Both written and resolved.
tooling::HeaderIncludes Inserter; // Computers insertion replacement.
+ HeaderFilter QuotedHeaders;
+ HeaderFilter AngledHeaders;
};
} // namespace clangd
diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h
index b3ba3a716083e88..7e289ef4c13532e 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.h
+++ b/clang-tools-extra/clangd/IncludeCleaner.h
@@ -55,7 +55,6 @@ struct IncludeCleanerFindings {
IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST);
-using HeaderFilter = llvm::ArrayRef<std::function<bool(llvm::StringRef)>>;
std::vector<Diag>
issueIncludeCleanerDiagnostics(ParsedAST &AST, llvm::StringRef Code,
const IncludeCleanerFindings &Findings,
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 3f6c0eaebac6c41..90684d36f152c7e 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -612,7 +612,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
getFormatStyleForFile(Filename, Inputs.Contents, *Inputs.TFS);
auto Inserter = std::make_shared<IncludeInserter>(
Filename, Inputs.Contents, Style, BuildDir.get(),
- &Clang->getPreprocessor().getHeaderSearchInfo());
+ &Clang->getPreprocessor().getHeaderSearchInfo(),
+ Cfg.Style.QuotedHeaders, Cfg.Style.AngledHeaders);
ArrayRef<Inclusion> MainFileIncludes;
if (Preamble) {
MainFileIncludes = Preamble->Includes.MainFileIncludes;
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index f0ffc429c0ca909..780a23fc1348841 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -539,6 +539,40 @@ TEST_F(ConfigCompileTests, Style) {
Frag.Style.FullyQualifiedNamespaces.push_back(std::string("bar"));
EXPECT_TRUE(compileAndApply());
EXPECT_THAT(Conf.Style.FullyQualifiedNamespaces, ElementsAre("foo", "bar"));
+
+ {
+ EXPECT_TRUE(Conf.Style.QuotedHeaders.empty())
+ << Conf.Style.QuotedHeaders.size();
+ Frag.Style.QuotedHeaders.push_back(Located<std::string>("foo.h"));
+ Frag.Style.QuotedHeaders.push_back(Located<std::string>(".*inc"));
+ EXPECT_TRUE(compileAndApply());
+ auto HeaderFilter = [this](llvm::StringRef Path) {
+ for (auto &Filter : Conf.Style.QuotedHeaders) {
+ if (Filter(Path))
+ return true;
+ }
+ return false;
+ };
+ EXPECT_TRUE(HeaderFilter("foo.h"));
+ EXPECT_FALSE(HeaderFilter("bar.h"));
+ }
+
+ {
+ EXPECT_TRUE(Conf.Style.AngledHeaders.empty())
+ << Conf.Style.AngledHeaders.size();
+ Frag.Style.AngledHeaders.push_back(Located<std::string>("foo.h"));
+ Frag.Style.AngledHeaders.push_back(Located<std::string>(".*inc"));
+ EXPECT_TRUE(compileAndApply());
+ auto HeaderFilter = [this](llvm::StringRef Path) {
+ for (auto &Filter : Conf.Style.AngledHeaders) {
+ if (Filter(Path))
+ return true;
+ }
+ return false;
+ };
+ EXPECT_TRUE(HeaderFilter("foo.h"));
+ EXPECT_FALSE(HeaderFilter("bar.h"));
+ }
}
} // namespace
} // namespace config
diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
index 44a6647d4c0a818..8f8ecc52b4034a8 100644
--- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -282,13 +282,19 @@ TEST(ParseYAML, Style) {
CapturedDiags Diags;
Annotations YAML(R"yaml(
Style:
- FullyQualifiedNamespaces: [foo, bar])yaml");
+ FullyQualifiedNamespaces: [foo, bar]
+ AngledHeaders: ["foo", "bar"]
+ QuotedHeaders: ["baz", "baar"])yaml");
auto Results =
Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
ASSERT_THAT(Diags.Diagnostics, IsEmpty());
ASSERT_EQ(Results.size(), 1u);
EXPECT_THAT(Results[0].Style.FullyQualifiedNamespaces,
ElementsAre(val("foo"), val("bar")));
+ EXPECT_THAT(Results[0].Style.AngledHeaders,
+ ElementsAre(val("foo"), val("bar")));
+ EXPECT_THAT(Results[0].Style.QuotedHeaders,
+ ElementsAre(val("baz"), val("baar")));
}
} // namespace
} // namespace config
diff --git a/clang-tools-extra/clangd/unittests/HeadersTests.cpp b/clang-tools-extra/clangd/unittests/HeadersTests.cpp
index dc6adaee1125718..751383e3b4650a4 100644
--- a/clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -107,7 +107,8 @@ class HeadersTest : public ::testing::Test {
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
CDB.getCompileCommand(MainFile)->Directory,
- &Clang->getPreprocessor().getHeaderSearchInfo());
+ &Clang->getPreprocessor().getHeaderSearchInfo(),
+ QuotedHeaders, AngledHeaders);
for (const auto &Inc : Inclusions)
Inserter.addExisting(Inc);
auto Inserted = ToHeaderFile(Preferred);
@@ -127,7 +128,8 @@ class HeadersTest : public ::testing::Test {
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
CDB.getCompileCommand(MainFile)->Directory,
- &Clang->getPreprocessor().getHeaderSearchInfo());
+ &Clang->getPreprocessor().getHeaderSearchInfo(),
+ QuotedHeaders, AngledHeaders);
auto Edit = Inserter.insert(VerbatimHeader, Directive);
Action.EndSourceFile();
return Edit;
@@ -139,6 +141,8 @@ class HeadersTest : public ::testing::Test {
std::string Subdir = testPath("sub");
std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
IgnoringDiagConsumer IgnoreDiags;
+ std::vector<std::function<bool(llvm::StringRef)>> QuotedHeaders;
+ std::vector<std::function<bool(llvm::StringRef)>> AngledHeaders;
std::unique_ptr<CompilerInstance> Clang;
};
@@ -304,6 +308,9 @@ TEST_F(HeadersTest, InsertInclude) {
std::string Path = testPath("sub/bar.h");
FS.Files[Path] = "";
EXPECT_EQ(calculate(Path), "\"bar.h\"");
+
+ AngledHeaders.push_back([](auto Path) { return true; });
+ EXPECT_EQ(calculate(Path), "<bar.h>");
}
TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
@@ -326,6 +333,17 @@ TEST_F(HeadersTest, ShortenIncludesInSearchPath) {
EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
}
+TEST_F(HeadersTest, ShortenIncludesInSearchPathBracketed) {
+ AngledHeaders.push_back([](auto Path) { return true; });
+ std::string BarHeader = testPath("sub/bar.h");
+ EXPECT_EQ(calculate(BarHeader), "<bar.h>");
+
+ SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
+ CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+ BarHeader = testPath("sub/bar.h");
+ EXPECT_EQ(calculate(BarHeader), "<sub/bar.h>");
+}
+
TEST_F(HeadersTest, ShortenedIncludeNotInSearchPath) {
std::string BarHeader =
llvm::sys::path::convert_to_slash(testPath("sub-2/bar.h"));
@@ -338,6 +356,10 @@ TEST_F(HeadersTest, PreferredHeader) {
std::string BazHeader = testPath("sub/baz.h");
EXPECT_EQ(calculate(BarHeader, BazHeader), "\"baz.h\"");
+
+ AngledHeaders.push_back([](auto Path) { return true; });
+ std::string BiffHeader = testPath("sub/biff.h");
+ EXPECT_EQ(calculate(BarHeader, BiffHeader), "<biff.h>");
}
TEST_F(HeadersTest, DontInsertDuplicatePreferred) {
@@ -370,7 +392,8 @@ TEST_F(HeadersTest, PreferInserted) {
TEST(Headers, NoHeaderSearchInfo) {
std::string MainFile = testPath("main.cpp");
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
- /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+ /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr,
+ /*QuotedHeaders=*/{}, /*AngledHeaders=*/{});
auto HeaderPath = testPat...
[truncated]
|
|
I investigated the test failures some more in the context of where the bug affects real uses and I'm unsure as to what's going on. Style:
QuotedHeaders: ["blahblahblah"]
AngledHeaders: ["AK/.*", "Userland/.*", "Kernel/.*", "Applications/.*", "Lib.*/.*"]will match all regexes correctly. Style:
AngledHeaders: ["AK/.*", "Userland/.*", "Kernel/.*", "Applications/.*", "Lib.*/.*"]will make AngledHeaders completely disappear from the parsed and compiled config; apparently the dictionary key doesn't even exist anymore. Exactly this behavior makes a test fail on CI. Style:
QuotedHeaders: ["blahblahblah"]will keep QuotedHeaders in the parsed and compiled config and works as expected. |
kleinesfilmroellchen
force-pushed
the
clangd-header-quote-style
branch
from
d2bb824 to
167048c
Compare
|
Thanks for the finding @zyn0217, refactored the config compiler code a little. Tests should pass now and I have re-tested this on SerenityOS and another small project where it works as expected in practice. |
kleinesfilmroellchen
force-pushed
the
clangd-header-quote-style
branch
from
167048c to
2e3f397
Compare
|
What is the relationship between this patch, and clangd 17's "missing include" warning? Does the quick-fix for the "missing include" warning also respect these config options? |
Same question from me -- this is the main reason I personally care about this PR :) We have directories in our project tree that we'd like to be included as |
As far as I can tell, clangd uses the correct header style for inserted includes regardles of how those includes are created (via auto-include on code completion or fix suggestions). |
kleinesfilmroellchen
force-pushed
the
clangd-header-quote-style
branch
from
2e3f397 to
8a2b22e
Compare
|
@HighCommander4 @sam-mccall do you have any capacity to review this change? It would be nice to get it into the 18 release if the window hasn't closed. |
I played around with this a bit, and discovered that there are actually three situations in which clangd inserts a header:
The patch seems to handle the first two, but not the third. The third scenario can be tested by creating a workspace with the following files: .clangd: CompileFlags:
Add: -I.
Diagnostics:
MissingIncludes: Strict
Style:
AngledHeaders: "angled/.*"angled/angled.hpp: #ifndef ANGLED_HPP_
#define ANGLED_HPP_
void angled();
#endifindirect.hpp: #include <angled/angled.hpp>main.cpp: #include "indirect.hpp"
int main() {
angled();
}Now, in main.cpp, if you hover over the |
I'm happy to try and help, but I haven't really been involved in the development of the include-cleaner component, and based on the findings in my previous comment, the patch would benefit from feedback from someone more familiar with that, to provide guidance about where in the code the config option should be checked to handle the include-cleaner scenario as well. |
|
After some discussion, I would like to land the feature "partially" as-is without support for "include cleaner" header insertion. (Case 3 in @HighCommander4's elaboration above.) This is motivated by two factors:
If maintainers agree, I'd like to have this merged before 18. |
There was a problem hiding this comment.
I took a pass through the implementation. Generally looks good.
Would be nice to have a test that exercises the include insertion process in a more end-to-end way. I think something in CodeCompleteTests.cpp would work, using WithContextValue to set config values (example), and using the insertIncludeText matcher to make assertions about the spelling of inserted includes.
| @@ -211,10 +214,12 @@ class IncludeInserter { | |||
| // include path of non-verbatim header will not be shortened. | |||
| IncludeInserter(StringRef FileName, StringRef Code, | |||
| const format::FormatStyle &Style, StringRef BuildDir, | |||
| HeaderSearch *HeaderSearchInfo) | |||
| HeaderSearch *HeaderSearchInfo, HeaderFilter QuotedHeaders, | |||
There was a problem hiding this comment.
We're taking ArrayRefs here, and passing it vectors that live in Config.
How confident are we that this is ok from a lifetime point of view, i.e. the Config object will outlive the IncludeInserter object? How confident are we that this will remain the case across future refactorings?
Would it be better to copy the vector instead? Each element is a function which stores a shared_ptr to a vector of regexes, so we wouldn't be copying the regexes themselves.
There was a problem hiding this comment.
I'm not sure how much difference that makes. The ArrayRef usage is based on other code I've seen around, and while the config lives for a while, the IncludeInserter is only briefly used to add an include.
There was a problem hiding this comment.
Sorry about inactivity on my part (here and in general). Thanks for working on this, and thanks @HighCommander4 as always - will leave LGTM to you.
Landing this without include-cleaner support is OK with me, though a bit sad.
I'm not sure I see the difficulty though:
IncludeCleaner.cpp has generateMissingIncludeDiagnostics() which already takes the IgnoreHeaders filter, and decides whether to spell with angles or quotes based on the include-cleaner library's suggested spelling (Angled). Can we just override that based on the QuotedHeaders etc filters?
That wouldn't validate whether the include can actually be included with the flipped quoting style, but it almost always is, and ISTM the other paths also don't validate this.
It's sad that there's not a single codepath that would handle all these, but maybe the perfect is the enemy of the good here.
The code duplication was my reason for looking at the common path. If you're fine with duplicating the code I will go ahead and do that. |
Projects can now add config fragments like this to their .clangd: ```yaml Style: QuotedHeaders: "src/.*" AngledHeaders: ["path/sdk/.*", "third-party/.*"] ``` to force headers inserted via the --header-insertion=iwyu mode matching at least one of the regexes to have <> (AngledHeaders) or "" (QuotedHeaders) around them, respectively. For other headers (and in conflicting cases where both styles have a matching regex), the current system header detection remains. Ref clangd/clangd#1247 Based on https://reviews.llvm.org/D145843 This solution does not affect other clang tools like clang-format.
kleinesfilmroellchen
force-pushed
the
clangd-header-quote-style
branch
from
8a2b22e to
67971ca
Compare
The code duplication was my reason for looking at the common path. If you're fine with duplicating the code I will go ahead and do that. I have updated the commit with the review comments and added end-to-end tests. |
|
Sorry, I'm still struggling with Github reviews compared to Phabricator... how do I get to an interdiff showing the latest version of the patch compared to the version I reviewed? |
I'm fairly sure that GitHub has no built-in feature for this; I've never used it for my own reviews but it's unfortunate that that is missing of course. |
|
Yeah seems llvm prefers not to force push PRs, but to keep stacking commits. The squashed commit will have the PR description as the final commit description. The compare button is a bit useless when it has a bunch of changes from main on it (https://github.com/llvm/llvm-project/compare/8a2b22e69f82e3f4caa271b57b998d1c03b21d39..67971ca27ef5e2767aba5cfe2cec1021c4de5ec1) |
Thanks. Yeah, it looks like the "Compare" link next to the force-push notification in the Conversation view is the sort of thing I'm looking for, except that in this case, the patch was rebased since the previous version, and the rebase and the changes to address the review comments were pushed together, and, unlike Phabricator, Github doesn't seem to know to filter out the former. |
|
Anyways, I rebased the old version of the patch locally so that I could generate an interdiff, which I'll attach for reference: diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index 12ed1420c93e..3559591fce00 100644
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -124,7 +124,7 @@ struct Config {
// ::). All nested namespaces are affected as well.
std::vector<std::string> FullyQualifiedNamespaces;
- // List of regexes for inserting certain headers with <> or "".
+ // List of matcher functions for inserting certain headers with <> or "".
std::vector<std::function<bool(llvm::StringRef)>> QuotedHeaders;
std::vector<std::function<bool(llvm::StringRef)>> AngledHeaders;
} Style;
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index 81474bbd86a5..f0d1080f6c83 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -302,12 +302,16 @@ struct Fragment {
/// AngledHeaders (i.e. a header matches a regex in both QuotedHeaders and
/// AngledHeaders), system headers use <> and non-system headers use "".
/// These can match any suffix of the header file in question.
+ /// Matching is performed against the header text, not its absolute path
+ /// within the project.
std::vector<Located<std::string>> QuotedHeaders;
/// List of regexes for headers that should always be included with a
/// <>-style include. By default, and in case of a conflict with
/// AngledHeaders (i.e. a header matches a regex in both QuotedHeaders and
/// AngledHeaders), system headers use <> and non-system headers use "".
/// These can match any suffix of the header file in question.
+ /// Matching is performed against the header text, not its absolute path
+ /// within the project.
std::vector<Located<std::string>> AngledHeaders;
};
StyleBlock Style;
diff --git a/clang-tools-extra/clangd/Headers.h b/clang-tools-extra/clangd/Headers.h
index e1f3eaa3397b..b91179da253e 100644
--- a/clang-tools-extra/clangd/Headers.h
+++ b/clang-tools-extra/clangd/Headers.h
@@ -9,7 +9,6 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H
-#include "Config.h"
#include "Protocol.h"
#include "SourceCode.h"
#include "index/Symbol.h"
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index 6d387fec9b38..bf264a5a44ee 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -11,6 +11,7 @@
#include "ClangdServer.h"
#include "CodeComplete.h"
#include "Compiler.h"
+#include "Config.h"
#include "Feature.h"
#include "Matchers.h"
#include "Protocol.h"
@@ -916,6 +917,41 @@ TEST(CompletionTest, NoIncludeInsertionWhenDeclFoundInFile) {
AllOf(named("Y"), Not(insertInclude()))));
}
+TEST(CompletionTest, IncludeInsertionRespectsQuotedAngledConfig) {
+ TestTU TU;
+ TU.ExtraArgs.push_back("-I" + testPath("sub"));
+ TU.AdditionalFiles["sub/bar.h"] = "";
+ auto BarURI = URI::create(testPath("sub/bar.h")).toString();
+
+ Symbol Sym = cls("ns::X");
+ Sym.CanonicalDeclaration.FileURI = BarURI.c_str();
+ Sym.IncludeHeaders.emplace_back(BarURI, 1, Symbol::Include);
+ Annotations Test("int main() { ns::^ }");
+ TU.Code = Test.code().str();
+ auto Results = completions(TU, Test.point(), {Sym});
+ // Default for a local path is quoted include
+ EXPECT_THAT(Results.Completions,
+ ElementsAre(AllOf(named("X"), insertInclude("\"bar.h\""))));
+ {
+ Config C;
+ C.Style.AngledHeaders.push_back(
+ [](auto header) { return header == "bar.h"; });
+ WithContextValue WithCfg(Config::Key, std::move(C));
+ Results = completions(TU, Test.point(), {Sym});
+ EXPECT_THAT(Results.Completions,
+ ElementsAre(AllOf(named("X"), insertInclude("<bar.h>"))));
+ }
+ {
+ Config C;
+ C.Style.QuotedHeaders.push_back(
+ [](auto header) { return header == "bar.h"; });
+ WithContextValue WithCfg(Config::Key, std::move(C));
+ Results = completions(TU, Test.point(), {Sym});
+ EXPECT_THAT(Results.Completions,
+ ElementsAre(AllOf(named("X"), insertInclude("\"bar.h\""))));
+ }
+}
+
TEST(CompletionTest, IndexSuppressesPreambleCompletions) {
Annotations Test(R"cpp(
#include "bar.h"
@@ -1134,8 +1170,8 @@ TEST(CodeCompleteTest, NoColonColonAtTheEnd) {
}
TEST(CompletionTests, EmptySnippetDoesNotCrash) {
- // See https://github.com/clangd/clangd/issues/1216
- auto Results = completions(R"cpp(
+ // See https://github.com/clangd/clangd/issues/1216
+ auto Results = completions(R"cpp(
int main() {
auto w = [&](auto &&f) { return f(f); };
auto f = w([&](auto &&f) {
@@ -1151,18 +1187,18 @@ TEST(CompletionTests, EmptySnippetDoesNotCrash) {
}
TEST(CompletionTest, Issue1427Crash) {
- // Need to provide main file signals to ensure that the branch in
- // SymbolRelevanceSignals::computeASTSignals() that tries to
- // compute a symbol ID is taken.
- ASTSignals MainFileSignals;
- CodeCompleteOptions Opts;
- Opts.MainFileSignals = &MainFileSignals;
- completions(R"cpp(
+ // Need to provide main file signals to ensure that the branch in
+ // SymbolRelevanceSignals::computeASTSignals() that tries to
+ // compute a symbol ID is taken.
+ ASTSignals MainFileSignals;
+ CodeCompleteOptions Opts;
+ Opts.MainFileSignals = &MainFileSignals;
+ completions(R"cpp(
auto f = []() {
1.0_^
};
)cpp",
- {}, Opts);
+ {}, Opts);
}
TEST(CompletionTest, BacktrackCrashes) {
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index 0eea7ff01b48..efee23003067 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -555,7 +555,9 @@ TEST_F(ConfigCompileTests, Style) {
return false;
};
EXPECT_TRUE(HeaderFilter("foo.h"));
+ EXPECT_TRUE(HeaderFilter("prefix/foo.h"));
EXPECT_FALSE(HeaderFilter("bar.h"));
+ EXPECT_FALSE(HeaderFilter("foo.h/bar.h"));
}
{ |
|
(In the future, it would greatly help me as a reviewer if rebases are pushed separately from updates to address review comments, so that the latter get their own Compare link.) |
+1, please go ahead and wire up the include cleaner support based on Sam's outline; it seems fairly straightforward, the amount of duplication is not much, and we might as well have the config work consistently across all include-insertion features. The new test + other changes look good as well! |
|
@kleinesfilmroellchen or @ADKaster it's been quiet for a while, do you think there's any hope for this one still, or is this PR abandoned? Great work to date, BTW, this would be a big win for a project I work on as well (which unconditionally uses <> for internal headers). |
|
I don't have any energy and the massive reachitecting requested would take me tons of time. Please adopt this. Serenity folks are also waiting fwiw |
Thanks, I appreciate the update. Unfortunately I cannot adopt this work at this time. |
I certainly didn't intend to request a massive rearchitecting. Sam's comment gave me the impression that hooking up the preferences to include-cleaner would be fairly straightforward. Anyways, Sam also said:
so I'm fine with leaving that to a future enhancement. @kleinesfilmroellchen could you rebase the patch to a recent |
(I just realized there is an "Update branch" button which allows me to do this from the Github UI. Apologies, one year after the project's switch away from Phabricator and I still struggle at times to get the hang of Github...) |
|
The rebased patch has green buildkite runs, so I will go ahead and merge it. |
|
Thank you @kleinesfilmroellchen for your contribution! |
|
Did this already land in 19.1.7? I am having trouble getting it to work. My CompileFlags:
Add: -I.
Diagnostics:
MissingIncludes: Strict
Style:
AngledHeaders: ["base/.*", "engine/.*", "game/.*"]:LspInfo in neovim confirms I am running clang 19.1.7 When I start to call functions from system.h it automatically included it in quotes ._. #include "base/system.h" |
No, it will appear in clangd 20. |
Projects can now add config fragments like this to their .clangd:
to force headers inserted via the --header-insertion=iwyu mode matching at least one of the regexes to have <> (AngledHeaders) or "" (QuotedHeaders) around them, respectively. For other headers (and in conflicting cases where both styles have a matching regex), the current system header detection remains.
Ref clangd/clangd#1247
Based on https://reviews.llvm.org/D145843
This solution does not affect other clang tools like clang-format.
CC @HighCommander4 @ADKaster. I have no merge rights so someone else needs to commit this.