zyounan updated this revision to Diff 499417.
zyounan added a comment.
Move parsing logic to Record.cpp
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143319/new/
https://reviews.llvm.org/D143319
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
clang-tools-extra/include-cleaner/lib/Record.cpp
clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
Index: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
===================================================================
--- clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
@@ -20,8 +20,10 @@
namespace clang::include_cleaner {
namespace {
+using testing::AllOf;
using testing::ElementsAreArray;
using testing::IsEmpty;
+using testing::UnorderedElementsAre;
// Matches a Decl* if it is a NamedDecl with the given name.
MATCHER_P(named, N, "") {
@@ -284,19 +286,23 @@
// We don't build an AST, we just run a preprocessor action!
TestInputs Inputs;
PragmaIncludes PI;
+ RecordedNoIncludes NI;
PragmaIncludeTest() {
Inputs.MakeAction = [this] {
struct Hook : public PreprocessOnlyAction {
public:
- Hook(PragmaIncludes *Out) : Out(Out) {}
+ Hook(PragmaIncludes *Out, RecordedNoIncludes *NoInc)
+ : Out(Out), NoInc(NoInc) {}
bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
Out->record(CI);
+ NoInc->record(CI);
return true;
}
PragmaIncludes *Out;
+ RecordedNoIncludes *NoInc;
};
- return std::make_unique<Hook>(&PI);
+ return std::make_unique<Hook>(&PI, &NI);
};
}
@@ -486,6 +492,40 @@
EXPECT_TRUE(PI.getExporters(FM.getFile("bar.h").get(), FM).empty());
}
+MATCHER_P(written, Name, "") { return arg.Written == Name; }
+MATCHER_P(includeLine, N, "") { return arg.HashLine == N; }
+MATCHER_P(resolved, Name, "") { return arg.Resolved == Name; }
+
+TEST_F(PragmaIncludeTest, IWYUNoInclude) {
+ Inputs.Code = R"cpp(
+ // IWYU pragma: no_include "baz.h"
+ #include "external.h"
+ #include "foo.h"
+ // IWYU pragma: no_include <bar.h>
+ // IWYU pragma: no_include <multiple> "headers.hpp" are not <valid>
+ // IWYU pragma: no_include <multiple> "headers.hpp" <invalid>
+ // IWYU pragma: no_include "multiple" <headers.hpp> "invalid"
+ // IWYU pragma: no_include no_quotes are considered invalid
+ // IWYU pragma: no_include <valid> texts after are ignored
+ // IWYU pragma: no_include not starting with quotes <are> "ignored"
+ )cpp";
+ Inputs.ExtraFiles["external.h"] = R"cpp(
+ #pragma once
+ // IWYU pragma: no_include <headers.hpp>
+ )cpp";
+ Inputs.ExtraArgs.emplace_back("-I.");
+ createEmptyFiles({"foo.h", "bar.h", "baz.h", "valid", "invalid", "multiple",
+ "are", "ignored", "headers.hpp"});
+ TestAST Processed = build();
+ auto &NoIncludes = NI.IwyuNoIncludes;
+ EXPECT_THAT(
+ NoIncludes,
+ UnorderedElementsAre(
+ AllOf(written("\"baz.h\""), resolved("baz.h"), includeLine(1)),
+ AllOf(written("<bar.h>"), resolved("bar.h"), includeLine(4)),
+ AllOf(written("<valid>"), resolved("valid"), includeLine(9))));
+}
+
TEST_F(PragmaIncludeTest, SelfContained) {
Inputs.Code = R"cpp(
#include "guarded.h"
Index: clang-tools-extra/include-cleaner/lib/Record.cpp
===================================================================
--- clang-tools-extra/include-cleaner/lib/Record.cpp
+++ clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -423,4 +423,91 @@
return std::make_unique<PPRecorder>(*this, PP);
}
+void RecordedNoIncludes::record(const CompilerInstance &Clang) {
+ // Derived from PPCallbacks as we want to make Clang take the ownership of
+ // collector.
+ class NoIncludePragmaCollector : public CommentHandler, public PPCallbacks {
+ public:
+ NoIncludePragmaCollector(const CompilerInstance &Clang,
+ RecordedNoIncludes &Out)
+ : SM(Clang.getSourceManager()), Out(Out) {}
+ bool HandleComment(Preprocessor &PP, SourceRange Range) override {
+ bool InMainFile = SM.isInMainFile(Range.getBegin());
+ auto Pragma =
+ tooling::parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
+ if (!Pragma)
+ return false;
+
+ // Record no_include pragma. We only care about main files.
+ if (InMainFile && Pragma->consume_front("no_include")) {
+ auto Spelling =
+ Pragma->take_until([](char C) { return C == '\n'; }).trim();
+ if (Spelling.empty())
+ return false;
+ const auto SplitQuotedIncludeHeaders = [](llvm::StringRef Buffer) {
+ llvm::SmallVector<llvm::StringRef> Ret;
+ for (size_t Pos = 0, Last = 0; Pos != Buffer.size();) {
+ char Closing = 0;
+ switch (Buffer[Pos]) {
+ case '<':
+ Last = Pos;
+ Closing = '>';
+ break;
+ case '"':
+ Last = Pos++;
+ Closing = '"';
+ break;
+ default:
+ // Ignore non-quoted string
+ ++Pos;
+ continue;
+ }
+ while (Pos != Buffer.size() && Buffer[Pos] != Closing)
+ ++Pos;
+ if (Pos == Buffer.size())
+ break;
+ Ret.emplace_back(Buffer.substr(Last, Pos - Last + 1));
+ ++Pos;
+ }
+ return Ret;
+ };
+ const auto Headers = SplitQuotedIncludeHeaders(Spelling);
+ // Should we support multiple headers on one pragma? Or emit a warning?
+ if (Headers.size() != 1)
+ return false;
+ Spelling = Headers[0];
+ unsigned Offset = SM.getFileOffset(Range.getBegin());
+ unsigned CurrentLine =
+ SM.getLineNumber(SM.getMainFileID(), Offset) - 1; // 0-based.
+ if (const auto Header = Spelling.trim("\"<>"); !Header.empty()) {
+ const auto FE = PP.LookupFile(
+ /*FilenameLoc=*/{}, /*Filename=*/Header,
+ /*isAngled=*/Spelling.starts_with("<"),
+ /*FromDir=*/nullptr, /*FromFile=*/nullptr,
+ /*CurDir=*/nullptr, /*SearchPath=*/nullptr,
+ /*RelativePath=*/nullptr,
+ /*SuggestedModule=*/nullptr, /*IsMapped=*/nullptr,
+ /*IsFrameworkFound=*/nullptr, /*SkipCache=*/false,
+ /*OpenFile=*/false, /*CacheFailures=*/false);
+ if (!FE)
+ return false;
+ Out.IwyuNoIncludes.insert(IwyuNoInclude(
+ /*Written=*/Spelling.str(),
+ /*Resolved=*/
+ FE->getFileEntry().tryGetRealPathName().str(), CurrentLine));
+ }
+ return false;
+ }
+ return false;
+ }
+
+ private:
+ SourceManager &SM;
+ RecordedNoIncludes &Out;
+ };
+ auto Handler = std::make_unique<NoIncludePragmaCollector>(Clang, *this);
+ Clang.getPreprocessor().addCommentHandler(Handler.get());
+ Clang.getPreprocessor().addPPCallbacks(std::move(Handler));
+}
+
} // namespace clang::include_cleaner
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
===================================================================
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -18,6 +18,7 @@
#define CLANG_INCLUDE_CLEANER_RECORD_H
#include "clang-include-cleaner/Types.h"
+#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/SmallVector.h"
@@ -25,6 +26,7 @@
#include "llvm/Support/Allocator.h"
#include "llvm/Support/FileSystem/UniqueID.h"
#include <memory>
+#include <set>
#include <vector>
namespace clang {
@@ -39,6 +41,21 @@
namespace include_cleaner {
+struct IwyuNoInclude {
+ std::string
+ Written; // For `IWYU pragma no_include <vector>`, it is `<vector>`.
+ std::string Resolved; // Resolved path to written file.
+ int HashLine; // 0-based line number containing IWYU pragma
+ IwyuNoInclude() = default;
+ IwyuNoInclude(std::string Written, std::string Resolved, int HashLine)
+ : Written(std::move(Written)), Resolved(std::move(Resolved)),
+ HashLine(HashLine) {}
+ friend bool operator<(const IwyuNoInclude &LHS, const IwyuNoInclude &RHS) {
+ // We want only 1 pragma for the same written header.
+ return LHS.Written < RHS.Written;
+ }
+};
+
/// Captures #include mapping information. It analyses IWYU Pragma comments and
/// other use-instead-like mechanisms (#pragma include_instead) on included
/// files.
@@ -143,6 +160,18 @@
include_cleaner::Includes Includes;
};
+/// Collects IWYU no_include directives. We don't put the parse logic in
+/// \class PragmaIncludes because the directive could be scattered throughout
+/// the source rather than being limited in preamble bounds.
+struct RecordedNoIncludes {
+ /// The callback (when installed into clang) tracks no_includes pragma in
+ /// this.
+ void record(const CompilerInstance &Clang);
+
+ /// no_include directives.
+ std::set<IwyuNoInclude> IwyuNoIncludes;
+};
+
} // namespace include_cleaner
} // namespace clang
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -9,6 +9,7 @@
#include "Annotations.h"
#include "Config.h"
#include "IncludeCleaner.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -22,8 +23,10 @@
namespace clangd {
namespace {
+using ::testing::AllOf;
using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+using ::testing::Field;
using ::testing::IsEmpty;
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
@@ -611,6 +614,39 @@
EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
}
+MATCHER_P(message, Message, "") { return arg.Message == Message; }
+MATCHER_P2(range, Start, End, "") {
+ return arg.start == Start && arg.end == End;
+}
+
+TEST(IncludeCleaner, IWYUPragmaNoInclude) {
+ Config Cfg;
+ Cfg.Diagnostics.UnusedIncludes = Config::Experiment;
+ WithContextValue Ctx(Config::Key, std::move(Cfg));
+ TestTU TU;
+ TU.Code = R"cpp(
+#include "foo.h"
+#include "bar.h"
+// IWYU pragma: no_include "bar.h"
+ )cpp";
+ TU.AdditionalFiles["foo.h"] = guard("");
+ TU.AdditionalFiles["bar.h"] = guard("");
+ ParsedAST AST = TU.build();
+
+ auto Diags = issueNoIncludesDiagnostics(AST, TU.Code);
+ EXPECT_THAT(
+ Diags,
+ UnorderedElementsAre(AllOf(
+ message(
+ "included header 'bar.h' is marked as `no_include` at line 4"),
+ Field(&DiagBase::Range, range(Position{2, 0}, Position{2, 16})))));
+ ASSERT_TRUE(Diags.size() == 1);
+ ASSERT_TRUE(Diags.back().Fixes.size() == 1);
+ ASSERT_TRUE(Diags.back().Fixes.back().Edits.size() == 1);
+ EXPECT_THAT(Diags.back().Fixes.back().Edits.back().range,
+ range(Position{2, 0}, Position{3, 0}));
+}
+
TEST(IncludeCleaner, NoDiagsForObjC) {
TestTU TU;
TU.Code = R"cpp(
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -878,6 +878,26 @@
AllOf(named("Y"), Not(insertInclude()))));
}
+TEST(CompletionTest, NoIncludeInsertionIfSpecifiedInNoIncludePragma) {
+ Symbol SymX = cls("ns::X");
+ std::string BarHeader = testPath("sub/bar");
+ auto BarURI = URI::create(BarHeader).toString();
+ SymX.CanonicalDeclaration.FileURI = BarURI.c_str();
+ SymX.IncludeHeaders.emplace_back("<bar>", 1, Symbol::Include);
+ TestTU TU;
+ TU.Code = R"cpp(
+ // IWYU pragma: no_include <bar>
+ namespace ns {}
+ int main() { ns::^ }
+ )cpp";
+ TU.AdditionalFiles["sub/bar"] = "";
+ Annotations Test(TU.Code);
+ TU.ExtraArgs.emplace_back("-I" + testPath("sub"));
+ auto Results = completions(TU, Test.point(), {SymX});
+ EXPECT_THAT(Results.Completions,
+ ElementsAre(AllOf(named("X"), Not(insertInclude()))));
+}
+
TEST(CompletionTest, IndexSuppressesPreambleCompletions) {
Annotations Test(R"cpp(
#include "bar.h"
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -571,6 +571,9 @@
MainFileIncludes = Preamble->Includes.MainFileIncludes;
for (const auto &Inc : Preamble->Includes.MainFileIncludes)
Inserter->addExisting(Inc);
+ for (const auto &NoInc :
+ Preamble->Includes.RecordedNoIncludes.IwyuNoIncludes)
+ Inserter->addNoInclude(NoInc);
}
// FIXME: Consider piping through ASTSignals to fetch this to handle the
// case where a header file contains ObjC decls but no #imports.
@@ -691,9 +694,14 @@
if (Result.Diags) {
auto UnusedHeadersDiags =
issueUnusedIncludesDiagnostics(Result, Inputs.Contents);
+ auto NoIncludeHeadersDiags =
+ issueNoIncludesDiagnostics(Result, Inputs.Contents);
Result.Diags->insert(Result.Diags->end(),
make_move_iterator(UnusedHeadersDiags.begin()),
make_move_iterator(UnusedHeadersDiags.end()));
+ Result.Diags->insert(Result.Diags->end(),
+ make_move_iterator(NoIncludeHeadersDiags.begin()),
+ make_move_iterator(NoIncludeHeadersDiags.end()));
}
return std::move(Result);
}
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -105,6 +105,9 @@
std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
llvm::StringRef Code);
+std::vector<Diag> issueNoIncludesDiagnostics(ParsedAST &AST,
+ llvm::StringRef Code);
+
/// Affects whether standard library includes should be considered for
/// removal. This is off by default for now due to implementation limitations:
/// - macros are not tracked
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -576,5 +576,46 @@
return Result;
}
+std::vector<Diag> issueNoIncludesDiagnostics(ParsedAST &AST,
+ llvm::StringRef Code) {
+ // C/C++ only
+ if (AST.getLangOpts().ObjC)
+ return {};
+ std::vector<Diag> Result;
+ std::string FileName =
+ AST.getSourceManager()
+ .getFileEntryRefForID(AST.getSourceManager().getMainFileID())
+ ->getName()
+ .str();
+ auto &NoIncludes =
+ AST.getIncludeStructure().RecordedNoIncludes.IwyuNoIncludes;
+ if (NoIncludes.empty())
+ return {};
+ for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
+ const auto It = NoIncludes.find(include_cleaner::IwyuNoInclude(
+ Inc.Written, Inc.Resolved, Inc.HashLine));
+ if (It == NoIncludes.end())
+ continue;
+ Diag D;
+ D.Message = llvm::formatv(
+ "included header '{0}' is marked as `no_include` at line {1}",
+ StringRef(Inc.Written).trim("<>\""), It->HashLine + 1);
+ D.Name = "no-include-headers";
+ D.Source = Diag::DiagSource::Clangd;
+ D.File = FileName;
+ D.Severity = DiagnosticsEngine::Warning;
+ D.Tags.push_back(Unnecessary);
+ D.Range = getDiagnosticRange(Code, Inc.HashOffset);
+ D.Fixes.emplace_back();
+ D.Fixes.back().Message = "remove #include directive";
+ D.Fixes.back().Edits.emplace_back();
+ D.Fixes.back().Edits.back().range.start.line = Inc.HashLine;
+ D.Fixes.back().Edits.back().range.end.line = Inc.HashLine + 1;
+ D.InsideMainFile = true;
+ Result.push_back(std::move(D));
+ }
+ return Result;
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Headers.h
===================================================================
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -11,6 +11,7 @@
#include "Protocol.h"
#include "SourceCode.h"
+#include "clang-include-cleaner/Record.h"
#include "index/Symbol.h"
#include "support/Path.h"
#include "clang/Basic/FileEntry.h"
@@ -184,6 +185,8 @@
class RecordHeaders;
+ include_cleaner::RecordedNoIncludes RecordedNoIncludes;
+
private:
// MainFileEntry will be used to check if the queried file is the main file
// or not.
@@ -219,6 +222,8 @@
void addExisting(const Inclusion &Inc);
+ void addNoInclude(const include_cleaner::IwyuNoInclude &Inc);
+
/// Checks whether to add an #include of the header into \p File.
/// An #include will not be added if:
/// - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
@@ -258,7 +263,9 @@
StringRef BuildDir;
HeaderSearch *HeaderSearchInfo = nullptr;
llvm::StringSet<> IncludedHeaders; // Both written and resolved.
- tooling::HeaderIncludes Inserter; // Computers insertion replacement.
+ llvm::StringSet<> IwyuNoIncludeHeaders; // Iwyu: no_include headers, with
+ // written and resolved.
+ tooling::HeaderIncludes Inserter; // Computers insertion replacement.
};
} // namespace clangd
Index: clang-tools-extra/clangd/Headers.cpp
===================================================================
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -9,6 +9,7 @@
#include "Headers.h"
#include "Preamble.h"
#include "SourceCode.h"
+#include "clang-include-cleaner/Record.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Frontend/CompilerInstance.h"
@@ -233,6 +234,7 @@
auto Collector = std::make_unique<RecordHeaders>(CI, this);
CI.getPreprocessor().addCommentHandler(Collector.get());
CI.getPreprocessor().addPPCallbacks(std::move(Collector));
+ RecordedNoIncludes.record(CI);
}
std::optional<IncludeStructure::HeaderID>
@@ -300,6 +302,12 @@
IncludedHeaders.insert(Inc.Resolved);
}
+void IncludeInserter::addNoInclude(const include_cleaner::IwyuNoInclude &Inc) {
+ IwyuNoIncludeHeaders.insert(Inc.Written);
+ if (!Inc.Resolved.empty())
+ IwyuNoIncludeHeaders.insert(Inc.Resolved);
+}
+
/// FIXME(ioeric): we might not want to insert an absolute include path if the
/// path is not shortened.
bool IncludeInserter::shouldInsertInclude(
@@ -312,7 +320,13 @@
auto Included = [&](llvm::StringRef Header) {
return IncludedHeaders.find(Header) != IncludedHeaders.end();
};
- return !Included(DeclaringHeader) && !Included(InsertedHeader.File);
+ auto IsIwyuNoInclude = [&](llvm::StringRef Header) {
+ return IwyuNoIncludeHeaders.find(Header) != IwyuNoIncludeHeaders.end();
+ };
+ auto ShouldInsert = [&](auto &Predicate) {
+ return !Predicate(DeclaringHeader) && !Predicate(InsertedHeader.File);
+ };
+ return ShouldInsert(Included) && ShouldInsert(IsIwyuNoInclude);
}
std::optional<std::string>
Index: clang-tools-extra/clangd/CodeComplete.cpp
===================================================================
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1556,6 +1556,8 @@
&Recorder->CCSema->getPreprocessor().getHeaderSearchInfo());
for (const auto &Inc : Includes.MainFileIncludes)
Inserter->addExisting(Inc);
+ for (const auto &Inc : Includes.RecordedNoIncludes.IwyuNoIncludes)
+ Inserter->addNoInclude(Inc);
// Most of the cost of file proximity is in initializing the FileDistance
// structures based on the observed includes, once per query. Conceptually
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
