Skip to content

[clangd] Allow specifying what headers are always included via "" or <> #67749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions clang-tools-extra/clangd/CodeComplete.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -807,8 +807,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())
Expand Down Expand Up @@ -1604,7 +1604,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;
Expand Down Expand Up @@ -1663,7 +1663,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);

Expand Down Expand Up @@ -1746,7 +1748,9 @@ class CodeCompleteFlow {
auto Style = getFormatStyleForFile(FileName, Content, TFS, false);
// 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;
Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/clangd/Config.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,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 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;

/// controls the completion options for argument lists.
Expand Down
49 changes: 49 additions & 0 deletions clang-tools-extra/clangd/ConfigCompile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,55 @@ struct FragmentCompiler {
FullyQualifiedNamespaces.begin(), FullyQualifiedNamespaces.end());
});
}
auto QuotedFilter = compileHeaderRegexes(F.QuotedHeaders);
if (QuotedFilter.has_value()) {
Out.Apply.push_back(
[QuotedFilter = *QuotedFilter](const Params &, Config &C) {
C.Style.QuotedHeaders.emplace_back(QuotedFilter);
});
}
auto AngledFilter = compileHeaderRegexes(F.AngledHeaders);
if (AngledFilter.has_value()) {
Out.Apply.push_back(
[AngledFilter = *AngledFilter](const Params &, Config &C) {
C.Style.AngledHeaders.emplace_back(AngledFilter);
});
}
}

auto compileHeaderRegexes(llvm::ArrayRef<Located<std::string>> HeaderPatterns)
-> std::optional<std::function<bool(llvm::StringRef)>> {
// 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 : HeaderPatterns) {
// 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 std::nullopt;
auto Filter = [Filters](llvm::StringRef Path) {
for (auto &Regex : *Filters)
if (Regex.match(Path))
return true;
return false;
};
return Filter;
}

void appendTidyCheckSpec(std::string &CurSpec,
Expand Down
17 changes: 17 additions & 0 deletions clang-tools-extra/clangd/ConfigFragment.h
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,23 @@ 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.
/// 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;

Expand Down
8 changes: 8 additions & 0 deletions clang-tools-extra/clangd/ConfigYAML.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,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);
}

Expand Down
34 changes: 29 additions & 5 deletions clang-tools-extra/clangd/Headers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -287,11 +287,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);
Expand All @@ -304,9 +304,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;
}
Expand Down
10 changes: 8 additions & 2 deletions clang-tools-extra/clangd/Headers.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,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);

Expand Down Expand Up @@ -211,10 +213,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,
Copy link
Collaborator

@HighCommander4 HighCommander4 Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);

Expand Down Expand Up @@ -258,6 +262,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
Expand Down
1 change: 0 additions & 1 deletion clang-tools-extra/clangd/IncludeCleaner.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ IncludeCleanerFindings
computeIncludeCleanerFindings(ParsedAST &AST,
bool AnalyzeAngledIncludes = false);

using HeaderFilter = llvm::ArrayRef<std::function<bool(llvm::StringRef)>>;
std::vector<Diag>
issueIncludeCleanerDiagnostics(ParsedAST &AST, llvm::StringRef Code,
const IncludeCleanerFindings &Findings,
Expand Down
3 changes: 2 additions & 1 deletion clang-tools-extra/clangd/ParsedAST.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
getFormatStyleForFile(Filename, Inputs.Contents, *Inputs.TFS, false);
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;
Expand Down
55 changes: 45 additions & 10 deletions clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,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"
Expand Down Expand Up @@ -1138,8 +1173,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) {
Expand All @@ -1155,18 +1190,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) {
Expand Down
38 changes: 38 additions & 0 deletions clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,44 @@ TEST_F(ConfigCompileTests, Style) {
Frag.Style.FullyQualifiedNamespaces.push_back(std::string("bar"));
EXPECT_TRUE(compileAndApply());
EXPECT_THAT(Conf.Style.FullyQualifiedNamespaces, ElementsAre("foo", "bar"));

{
Frag = {};
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_TRUE(HeaderFilter("prefix/foo.h"));
EXPECT_FALSE(HeaderFilter("bar.h"));
EXPECT_FALSE(HeaderFilter("foo.h/bar.h"));
}

{
Frag = {};
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
Expand Down
Loading
Loading