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 @@ -173,6 +173,11 @@ std::vector MainFileIncludes; + // The entries of the header search path. (HeaderSearch::search_dir_range()) + // Only includes the plain-directory entries (not header maps or frameworks). + // All paths are canonical (FileManager::getCanonicalPath()). + std::vector SearchPathCanonical; + // We reserve HeaderID(0) for the main file and will manually check for that // in getID and getOrCreateID because the UniqueID is not stable when the // content of the main file changes. 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 @@ -12,6 +12,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/DirectoryLookup.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/Preprocessor.h" @@ -178,6 +179,17 @@ MainFileEntry = SM.getFileEntryForID(SM.getMainFileID()); auto Collector = std::make_unique(CI, this); CI.getPreprocessor().addPPCallbacks(std::move(Collector)); + + // If we're reusing a preamble, don't repopulate SearchPathCanonical. + // The entries will be the same, but canonicalizing to find out is expensive! + if (SearchPathCanonical.empty()) { + for (const auto &Dir : + CI.getPreprocessor().getHeaderSearchInfo().search_dir_range()) { + if (Dir.getLookupType() == DirectoryLookup::LT_NormalDir) + SearchPathCanonical.emplace_back( + SM.getFileManager().getCanonicalName(*Dir.getDirRef())); + } + } } std::optional diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -1195,8 +1195,7 @@ return; std::string Result; - include_cleaner::Includes ConvertedIncludes = - convertIncludes(SM, AST.getIncludeStructure().MainFileIncludes); + include_cleaner::Includes ConvertedIncludes = convertIncludes(AST); for (const auto &P : RankedProviders) { if (P.kind() == include_cleaner::Header::Physical && P.physical() == SM.getFileEntryForID(SM.getMainFileID())) @@ -1248,9 +1247,10 @@ void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) { const SourceManager &SM = AST.getSourceManager(); - const auto &ConvertedMainFileIncludes = - convertIncludes(SM, AST.getIncludeStructure().MainFileIncludes); - const auto &HoveredInclude = convertIncludes(SM, llvm::ArrayRef{Inc}); + const auto &Converted = convertIncludes(AST); + const auto *Target = Converted.atLine(Inc.HashLine + 1); + if (Target == nullptr) // conversion can skip some broken inclueds + return; llvm::DenseSet UsedSymbols; include_cleaner::walkUsed( AST.getLocalTopLevelDecls(), collectMacroReferences(AST), @@ -1261,12 +1261,13 @@ UsedSymbols.contains(Ref.Target)) return; - auto Provider = - firstMatchedProvider(ConvertedMainFileIncludes, Providers); - if (!Provider || HoveredInclude.match(*Provider).empty()) - return; - - UsedSymbols.insert(Ref.Target); + for (const auto &H : Providers) { + auto Matches = Converted.match(H); + if (llvm::is_contained(Matches, Target)) + UsedSymbols.insert(Ref.Target); + if (!Matches.empty()) + break; + } }); for (const auto &UsedSymbolDecl : UsedSymbols) 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 @@ -72,17 +72,11 @@ /// Converts the clangd include representation to include-cleaner /// include representation. -include_cleaner::Includes -convertIncludes(const SourceManager &SM, - const llvm::ArrayRef Includes); +include_cleaner::Includes convertIncludes(const ParsedAST &); std::vector collectMacroReferences(ParsedAST &AST); -/// Find the first provider in the list that is matched by the includes. -std::optional -firstMatchedProvider(const include_cleaner::Includes &Includes, - llvm::ArrayRef Providers); } // namespace clangd } // namespace clang 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 @@ -26,6 +26,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Format/Format.h" +#include "clang/Lex/DirectoryLookup.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/Preprocessor.h" #include "clang/Tooling/Core/Replacement.h" @@ -353,11 +354,16 @@ return Macros; } -include_cleaner::Includes -convertIncludes(const SourceManager &SM, - const llvm::ArrayRef Includes) { +include_cleaner::Includes convertIncludes(const ParsedAST &AST) { + auto &SM = AST.getSourceManager(); + include_cleaner::Includes ConvertedIncludes; - for (const Inclusion &Inc : Includes) { + // We satisfy Includes's contract that search dirs and included files have + // matching path styles: both ultimately use FileManager::getCanonicalName(). + for (const auto &Dir : AST.getIncludeStructure().SearchPathCanonical) + ConvertedIncludes.addSearchDirectory(Dir); + + for (const Inclusion &Inc : AST.getIncludeStructure().MainFileIncludes) { include_cleaner::Include TransformedInc; llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written); TransformedInc.Spelled = WrittenRef.trim("\"<>"); @@ -365,6 +371,10 @@ SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset); TransformedInc.Line = Inc.HashLine + 1; TransformedInc.Angled = WrittenRef.starts_with("<"); + // Inc.Resolved is canonicalized with clangd::getCanonicalPath(), + // which is based on FileManager::getCanonicalName(ParentDir). + // Calling getFile() updates FileEntry::getName() to match this path. + // FIXME: make this explicit by using FileEntryRef instead. auto FE = SM.getFileManager().getFile(Inc.Resolved); if (!FE) { elog("IncludeCleaner: Failed to get an entry for resolved path {0}: {1}", @@ -382,9 +392,7 @@ if (AST.getLangOpts().ObjC) return {}; const auto &SM = AST.getSourceManager(); - const auto &Includes = AST.getIncludeStructure(); - include_cleaner::Includes ConvertedIncludes = - convertIncludes(SM, Includes.MainFileIncludes); + include_cleaner::Includes ConvertedIncludes = convertIncludes(AST); const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); auto *PreamblePatch = PreamblePatch::getPatchEntry(AST.tuPath(), SM); @@ -407,7 +415,7 @@ } for (auto *Inc : ConvertedIncludes.match(H)) { Satisfied = true; - auto HeaderID = Includes.getID(Inc->Resolved); + auto HeaderID = AST.getIncludeStructure().getID(Inc->Resolved); assert(HeaderID.has_value() && "ConvertedIncludes only contains resolved includes."); Used.insert(*HeaderID); @@ -500,14 +508,4 @@ return Result; } -std::optional -firstMatchedProvider(const include_cleaner::Includes &Includes, - llvm::ArrayRef Providers) { - for (const auto &H : Providers) { - if (!Includes.match(H).empty()) - return H; - } - // No match for this provider in the includes list. - return std::nullopt; -} } // namespace clang::clangd diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -1327,24 +1327,15 @@ if (IncludeOnLine == Includes.end()) return std::nullopt; - const auto &Inc = *IncludeOnLine; const SourceManager &SM = AST.getSourceManager(); ReferencesResult Results; - auto ConvertedMainFileIncludes = convertIncludes(SM, Includes); - auto ReferencedInclude = convertIncludes(SM, Inc); - include_cleaner::walkUsed( - AST.getLocalTopLevelDecls(), collectMacroReferences(AST), - AST.getPragmaIncludes(), SM, - [&](const include_cleaner::SymbolReference &Ref, - llvm::ArrayRef Providers) { - if (Ref.RT != include_cleaner::RefType::Explicit) - return; - - auto Provider = - firstMatchedProvider(ConvertedMainFileIncludes, Providers); - if (!Provider || ReferencedInclude.match(*Provider).empty()) - return; + auto Converted = convertIncludes(AST); + auto *Target = Converted.atLine(IncludeOnLine->HashLine + 1); + if (!Target) + return std::nullopt; + auto AddResult = + [&](const include_cleaner::SymbolReference &Ref) { auto Loc = SM.getFileLoc(Ref.RefLocation); // File locations can be outside of the main file if macro is // expanded through an #include. @@ -1357,14 +1348,29 @@ sourceLocToPosition(SM, Token->endLocation())}; Result.Loc.uri = URIMainFile; Results.References.push_back(std::move(Result)); + }; + include_cleaner::walkUsed( + AST.getLocalTopLevelDecls(), collectMacroReferences(AST), + AST.getPragmaIncludes(), SM, + [&](const include_cleaner::SymbolReference &Ref, + llvm::ArrayRef Providers) { + if (Ref.RT != include_cleaner::RefType::Explicit) + return; + for (const auto &H : Providers) { + auto Matches = Converted.match(H); + if (llvm::is_contained(Matches, Target)) + AddResult(Ref); + if (!Matches.empty()) + break; + } }); if (Results.References.empty()) return std::nullopt; // Add the #include line to the references list. ReferencesResult::Reference Result; - Result.Loc.range = - rangeTillEOL(SM.getBufferData(SM.getMainFileID()), Inc.HashOffset); + Result.Loc.range = rangeTillEOL(SM.getBufferData(SM.getMainFileID()), + IncludeOnLine->HashOffset); Result.Loc.uri = URIMainFile; Results.References.push_back(std::move(Result)); 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 @@ -292,6 +292,15 @@ directive(tok::pp_include_next))); } +TEST_F(HeadersTest, SearchPath) { + FS.Files["foo/bar.h"] = "x"; + FS.Files["foo/bar/baz.h"] = "y"; + CDB.ExtraClangFlags.push_back("-Ifoo/bar"); + CDB.ExtraClangFlags.push_back("-Ifoo/bar/.."); + EXPECT_THAT(collectIncludes().SearchPathCanonical, + ElementsAre(Subdir, testPath("foo/bar"), testPath("foo"))); +} + TEST_F(HeadersTest, InsertInclude) { std::string Path = testPath("sub/bar.h"); FS.Files[Path] = ""; 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 @@ -459,48 +459,6 @@ MainCode.range()); } -TEST(IncludeCleaner, FirstMatchedProvider) { - struct { - const char *Code; - const std::vector Providers; - const std::optional ExpectedProvider; - } Cases[] = { - {R"cpp( - #include "bar.h" - #include "foo.h" - )cpp", - {include_cleaner::Header{"bar.h"}, include_cleaner::Header{"foo.h"}}, - include_cleaner::Header{"bar.h"}}, - {R"cpp( - #include "bar.h" - #include "foo.h" - )cpp", - {include_cleaner::Header{"foo.h"}, include_cleaner::Header{"bar.h"}}, - include_cleaner::Header{"foo.h"}}, - {"#include \"bar.h\"", - {include_cleaner::Header{"bar.h"}}, - include_cleaner::Header{"bar.h"}}, - {"#include \"bar.h\"", {include_cleaner::Header{"foo.h"}}, std::nullopt}, - {"#include \"bar.h\"", {}, std::nullopt}}; - for (const auto &Case : Cases) { - Annotations Code{Case.Code}; - SCOPED_TRACE(Code.code()); - - TestTU TU; - TU.Code = Code.code(); - TU.AdditionalFiles["bar.h"] = ""; - TU.AdditionalFiles["foo.h"] = ""; - - auto AST = TU.build(); - std::optional MatchedProvider = - firstMatchedProvider( - convertIncludes(AST.getSourceManager(), - AST.getIncludeStructure().MainFileIncludes), - Case.Providers); - EXPECT_EQ(MatchedProvider, Case.ExpectedProvider); - } -} - TEST(IncludeCleaner, BatchFix) { TestTU TU; TU.Filename = "main.cpp"; @@ -564,6 +522,32 @@ FixMessage("fix all includes")}))); } +// In the presence of IWYU pragma private, we should accept spellings other +// than the recommended one if they appear to name the same public header. +TEST(IncludeCleaner, VerbatimEquivalence) { + auto TU = TestTU::withCode(R"cpp( + #include "lib/rel/public.h" + int x = Public; + )cpp"); + TU.AdditionalFiles["repo/lib/rel/private.h"] = R"cpp( + #pragma once + // IWYU pragma: private, include "rel/public.h" + int Public; + )cpp"; + TU.AdditionalFiles["repo/lib/rel/public.h"] = R"cpp( + #pragma once + #include "rel/private.h" + )cpp"; + + TU.ExtraArgs.push_back("-Irepo"); + TU.ExtraArgs.push_back("-Irepo/lib"); + + auto AST = TU.build(); + auto Findings = computeIncludeCleanerFindings(AST); + EXPECT_THAT(Findings.MissingIncludes, IsEmpty()); + EXPECT_THAT(Findings.UnusedIncludes, IsEmpty()); +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h +++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h @@ -172,8 +172,20 @@ /// Registers a directory on the include path (-I etc) from HeaderSearch. /// This allows reasoning about equivalence of e.g. "path/a/b.h" and "a/b.h". /// This must be called before calling add() in order to take effect. + /// + /// The paths may be relative or absolute, but the paths passed to + /// addSearchDirectory() and add() (that is: Include.Resolved->getName()) + /// should be consistent, as they are compared lexically. + /// Generally, this is satisfied if you obtain paths through HeaderSearch + /// and FileEntries through PPCallbacks::IncludeDirective(). + /// + /// FIXME: change Include to use FileEntryRef so callers don't have to worry + /// about the behavior of FileEntry::getName() to follow this contract. void addSearchDirectory(llvm::StringRef); + /// Registers an include directive seen in the main file. + /// + /// This should only be called after all search directories are added. void add(const Include &); /// All #includes seen, in the order they appear.