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 @@ -23,11 +23,13 @@ #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringSet.h" #include "llvm/Support/Error.h" #include "llvm/Support/FileSystem/UniqueID.h" #include +#include namespace clang { namespace clangd { @@ -145,13 +147,11 @@ return !NonSelfContained.contains(ID); } - bool hasIWYUExport(HeaderID ID) const { - return HasIWYUExport.contains(ID); - } - // Return all transitively reachable files. llvm::ArrayRef allHeaders() const { return RealPathNames; } + llvm::Optional> exporters(HeaderID ID) const; + // Return all transitively reachable files, and their minimum include depth. // All transitive includes (absolute paths), with their minimum include depth. // Root --> 0, #included file --> 1, etc. @@ -189,9 +189,7 @@ // Contains HeaderIDs of all non self-contained entries in the // IncludeStructure. llvm::DenseSet NonSelfContained; - // Contains a set of headers that have either "IWYU pragma: export" or "IWYU - // pragma: begin_exports". - llvm::DenseSet HasIWYUExport; + llvm::DenseMap> Exporters; }; // Calculates insertion edit for including a new header in a file. 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,12 +9,15 @@ #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" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/Preprocessor.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Path.h" #include @@ -89,6 +92,18 @@ auto IncludingID = Out->getOrCreateID(*IncludingFileEntry), IncludedID = Out->getOrCreateID(*File); Out->IncludeChildren[IncludingID].push_back(IncludedID); + // FIXME: This has to be off-by-two instead of off-by-one for some reason. + int HashLine = + SM.getLineNumber(SM.getFileID(HashLoc), SM.getFileOffset(HashLoc)) - + 2; + HashLoc.dump(SM); + if (IncludingID == LastFileID && HashLine == LastPragmaExportLine) { + dlog("{0} exports {1}", IncludingFileEntry->getName(), File->getName()); + auto It = Out->Exporters.find(IncludedID); + if (It == Out->Exporters.end()) + It = Out->Exporters.insert({IncludedID, {}}).first; + It->second.push_back(IncludingID); + } } } @@ -133,6 +148,8 @@ llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err); if (Err) return false; + unsigned Offset = SM.getFileOffset(Range.getBegin()); + int Line = SM.getLineNumber(SM.getMainFileID(), Offset) - 1; if (inMainFile()) { // Given: // @@ -153,19 +170,13 @@ if (!Text.startswith(IWYUPragmaExport) && !Text.startswith(IWYUPragmaKeep)) return false; - unsigned Offset = SM.getFileOffset(Range.getBegin()); - LastPragmaKeepInMainFileLine = - SM.getLineNumber(SM.getMainFileID(), Offset) - 1; + LastPragmaKeepInMainFileLine = Line; } else { - // Memorize headers that that have export pragmas in them. Include Cleaner - // does not support them properly yet, so they will be not marked as - // unused. - // FIXME: Once IncludeCleaner supports export pragmas, remove this. - if (!Text.startswith(IWYUPragmaExport) && - !Text.startswith(IWYUPragmaBeginExports)) - return false; - Out->HasIWYUExport.insert( - *Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin())))); + if (Text.startswith(IWYUPragmaExport)) { + LastFileID = Out->getOrCreateID( + *SM.getFileEntryRefForID(SM.getFileID(Range.getBegin()))); + LastPragmaExportLine = Line; + } } return false; } @@ -186,6 +197,9 @@ // The last line "IWYU pragma: keep" was seen in the main file, 0-indexed. int LastPragmaKeepInMainFileLine = -1; + + HeaderID LastFileID = static_cast(0); + int LastPragmaExportLine = -1; }; bool isLiteralInclude(llvm::StringRef Include) { @@ -271,6 +285,14 @@ return Result; } +llvm::Optional> +IncludeStructure::exporters(IncludeStructure::HeaderID ID) const { + auto It = Exporters.find(ID); + if (It == Exporters.end()) + return llvm::None; + return llvm::Optional>(It->second); +} + llvm::DenseMap IncludeStructure::includeDepth(HeaderID Root) const { // Include depth 0 is the main file only. 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 @@ -67,15 +67,15 @@ /// Retrieves IDs of all files containing SourceLocations from \p Locs. /// The output only includes things SourceManager sees as files (not macro IDs). /// This can include , etc that are not true files. -/// \p HeaderResponsible returns the public header that should be included given -/// symbols from a file with the given FileID (example: public headers should be -/// preferred to non self-contained and private headers). -/// \p UmbrellaHeader returns the public public header is responsible for -/// providing symbols from a file with the given FileID (example: MyType.h -/// should be included instead of MyType_impl.h). +/// \p HeadersResponsible returns the public header that should be included +/// given symbols from a file with the given FileID (example: public headers +/// should be preferred to non self-contained and private headers). \p +/// UmbrellaHeader returns the public public header is responsible for providing +/// symbols from a file with the given FileID (example: MyType.h should be +/// included instead of MyType_impl.h). ReferencedFiles findReferencedFiles( const ReferencedLocations &Locs, const SourceManager &SM, - llvm::function_ref HeaderResponsible, + llvm::function_ref(FileID)> HeadersResponsible, llvm::function_ref(FileID)> UmbrellaHeader); ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs, const IncludeStructure &Includes, 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 @@ -24,6 +24,7 @@ #include "clang/Lex/Preprocessor.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringSet.h" @@ -31,6 +32,7 @@ #include "llvm/Support/Path.h" #include "llvm/Support/Regex.h" #include +#include namespace clang { namespace clangd { @@ -270,10 +272,6 @@ } assert(Inc.HeaderID); auto HID = static_cast(*Inc.HeaderID); - // FIXME: Ignore the headers with IWYU export pragmas for now, remove this - // check when we have more precise tracking of exported headers. - if (AST.getIncludeStructure().hasIWYUExport(HID)) - return false; auto FE = AST.getSourceManager().getFileManager().getFileRef( AST.getIncludeStructure().getRealPath(HID)); assert(FE); @@ -301,8 +299,8 @@ // its first includer that is self-contained. This is the header users can // include, so it will be responsible for bringing the symbols from given // header into the scope. -FileID headerResponsible(FileID ID, const SourceManager &SM, - const IncludeStructure &Includes) { +std::vector headersResponsible(FileID ID, const SourceManager &SM, + const IncludeStructure &Includes) { // Unroll the chain of non self-contained headers until we find the one that // can be included. for (const FileEntry *FE = SM.getFileEntryForID(ID); ID != SM.getMainFileID(); @@ -319,7 +317,24 @@ // on its includer. ID = SM.getFileID(SM.getIncludeLoc(ID)); } - return ID; + std::vector Result = {ID}; + std::queue Queue; + Queue.push(*Includes.getID(SM.getFileEntryForID(ID))); + llvm::DenseSet Visited; + while (!Queue.empty()) { + IncludeStructure::HeaderID HID = Queue.front(); + Visited.insert(HID); + Result.push_back(HID); + auto Exporters = Includes.exporters(HID); + if (!Exporters) + continue; + for (const auto &Exporter : *Exporters) { + if (Visited.find(Exporter) != Visited.end()) + continue; + Queue.push(Exporter); + } + } + return Result; } } // namespace @@ -343,7 +358,7 @@ ReferencedFiles findReferencedFiles( const ReferencedLocations &Locs, const SourceManager &SM, - llvm::function_ref HeaderResponsible, + llvm::function_ref(FileID)> HeadersResponsible, llvm::function_ref(FileID)> UmbrellaHeader) { std::vector Sorted{Locs.User.begin(), Locs.User.end()}; llvm::sort(Sorted); // Group by FileID. @@ -365,7 +380,9 @@ llvm::DenseSet UserFiles; llvm::StringSet<> PublicHeaders; for (FileID ID : Builder.Files) { - UserFiles.insert(HeaderResponsible(ID)); + for (auto HID : HeadersResponsible(ID)) { + UserFiles.insert(HID); + } if (auto PublicHeader = UmbrellaHeader(ID)) { PublicHeaders.insert(*PublicHeader); } @@ -387,7 +404,7 @@ return findReferencedFiles( Locs, SM, [&SM, &Includes](FileID ID) { - return headerResponsible(ID, SM, Includes); + return headersResponsible(ID, SM, Includes); }, [&SM, &CanonIncludes](FileID ID) -> Optional { auto Entry = SM.getFileEntryRefForID(ID); diff --git a/clang-tools-extra/clangd/unittests/HeadersTests.cpp b/clang-tools-extra/clangd/unittests/HeadersTests.cpp --- a/clang-tools-extra/clangd/unittests/HeadersTests.cpp +++ b/clang-tools-extra/clangd/unittests/HeadersTests.cpp @@ -258,18 +258,6 @@ directive(tok::pp_include_next))); } -TEST_F(HeadersTest, IWYUPragmaKeep) { - FS.Files[MainFile] = R"cpp( -#include "bar.h" // IWYU pragma: keep -#include "foo.h" -)cpp"; - - EXPECT_THAT( - collectIncludes().MainFileIncludes, - UnorderedElementsAre(AllOf(written("\"foo.h\""), hasPragmaKeep(false)), - AllOf(written("\"bar.h\""), hasPragmaKeep(true)))); -} - TEST_F(HeadersTest, InsertInclude) { std::string Path = testPath("sub/bar.h"); FS.Files[Path] = ""; @@ -418,31 +406,43 @@ EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes))); } -TEST_F(HeadersTest, HasIWYUPragmas) { +TEST_F(HeadersTest, IWYUPragmaKeep) { FS.Files[MainFile] = R"cpp( -#include "export.h" -#include "begin_exports.h" -#include "none.h" +#include "bar.h" // IWYU pragma: keep +#include "foo.h" )cpp"; - FS.Files["export.h"] = R"cpp( -#pragma once -#include "none.h" // IWYU pragma: export + + EXPECT_THAT( + collectIncludes().MainFileIncludes, + UnorderedElementsAre(AllOf(written("\"foo.h\""), hasPragmaKeep(false)), + AllOf(written("\"bar.h\""), hasPragmaKeep(true)))); +} + +TEST_F(HeadersTest, IWYUPragmaExport) { + FS.Files[MainFile] = R"cpp( +// WHATEVER COMMENT +#include "umbrella.h" + +void foo() { + bar(); +} )cpp"; - FS.Files["begin_exports.h"] = R"cpp( + FS.Files["umbrella.h"] = R"cpp( #pragma once -// IWYU pragma: begin_exports -#include "none.h" -// IWYU pragma: end_exports + +#include "lib.h" // IWYU pragma: export )cpp"; - FS.Files["none.h"] = R"cpp( + FS.Files["lib.h"] = R"cpp( #pragma once -// Not a pragma. + +void bar() {} )cpp"; auto Includes = collectIncludes(); - EXPECT_TRUE(Includes.hasIWYUExport(getID("export.h", Includes))); - EXPECT_TRUE(Includes.hasIWYUExport(getID("begin_exports.h", Includes))); - EXPECT_FALSE(Includes.hasIWYUExport(getID("none.h", Includes))); + auto LibExporters = Includes.exporters(getID("lib.h", Includes)); + ASSERT_TRUE(LibExporters); + EXPECT_THAT(*LibExporters, + UnorderedElementsAre(getID("umbrella.h", Includes))); } } // namespace 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 @@ -582,6 +582,10 @@ TestTU TU; TU.Code = R"cpp( #include "foo.h" + + void foo() { + bar(); + } )cpp"; TU.AdditionalFiles["foo.h"] = R"cpp( #ifndef FOO_H @@ -600,8 +604,6 @@ findReferencedLocations(AST), AST.getIncludeStructure(), AST.getCanonicalIncludes(), AST.getSourceManager()); EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); - // FIXME: This is not correct: foo.h is unused but is not diagnosed as such - // because we ignore headers with IWYU export pragmas for now. EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty()); }