diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/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 diff --git a/clang-tools-extra/clangd/Headers.h b/clang-tools-extra/clangd/Headers.h --- a/clang-tools-extra/clangd/Headers.h +++ b/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 diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp --- 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 "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(CI, this); CI.getPreprocessor().addCommentHandler(Collector.get()); CI.getPreprocessor().addPPCallbacks(std::move(Collector)); + RecordedNoIncludes.record(CI); } std::optional @@ -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 diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -105,6 +105,9 @@ std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, llvm::StringRef Code); +std::vector 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 diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -576,5 +576,46 @@ return Result; } +std::vector issueNoIncludesDiagnostics(ParsedAST &AST, + llvm::StringRef Code) { + // C/C++ only + if (AST.getLangOpts().ObjC) + return {}; + std::vector 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 diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/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); } diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/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("", 1, Symbol::Include); + TestTU TU; + TU.Code = R"cpp( + // IWYU pragma: no_include + 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" diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/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( diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h +++ b/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 +#include #include namespace clang { @@ -39,6 +41,21 @@ namespace include_cleaner { +struct IwyuNoInclude { + std::string + Written; // For `IWYU pragma no_include `, it is ``. + 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 IwyuNoIncludes; +}; + } // namespace include_cleaner } // namespace clang diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp --- a/clang-tools-extra/include-cleaner/lib/Record.cpp +++ b/clang-tools-extra/include-cleaner/lib/Record.cpp @@ -423,4 +423,91 @@ return std::make_unique(*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 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(Clang, *this); + Clang.getPreprocessor().addCommentHandler(Handler.get()); + Clang.getPreprocessor().addPPCallbacks(std::move(Handler)); +} + } // namespace clang::include_cleaner diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp --- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp +++ b/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(&PI); + return std::make_unique(&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 + // IWYU pragma: no_include "headers.hpp" are not + // IWYU pragma: no_include "headers.hpp" + // IWYU pragma: no_include "multiple" "invalid" + // IWYU pragma: no_include no_quotes are considered invalid + // IWYU pragma: no_include texts after are ignored + // IWYU pragma: no_include not starting with quotes "ignored" + )cpp"; + Inputs.ExtraFiles["external.h"] = R"cpp( + #pragma once + // IWYU pragma: no_include + )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(""), resolved("bar.h"), includeLine(4)), + AllOf(written(""), resolved("valid"), includeLine(9)))); +} + TEST_F(PragmaIncludeTest, SelfContained) { Inputs.Code = R"cpp( #include "guarded.h"