From 49ea3ae3468c5d67b2b6b03339d642e25d4fa6b0 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Tue, 14 Nov 2023 13:08:52 +0100 Subject: [PATCH 1/2] [include-cleaner] Make sure exports of stdlib also works for physical files This was creating discrepancy in cases where we might have a physical file entry (e.g. because we followed a source location from a stdlib file) and tried to find its exporters. --- .../include-cleaner/lib/Record.cpp | 26 +++++++++---------- .../include-cleaner/unittests/RecordTest.cpp | 2 ++ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp index 7a8e10a9c6754..a98129452e2bd 100644 --- a/clang-tools-extra/include-cleaner/lib/Record.cpp +++ b/clang-tools-extra/include-cleaner/lib/Record.cpp @@ -232,6 +232,17 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler { void checkForExport(FileID IncludingFile, int HashLine, std::optional
IncludedHeader, OptionalFileEntryRef IncludedFile) { + auto AddExport = [&] { + auto ExportingFileName = SM.getFileEntryForID(IncludingFile)->getName(); + if (IncludedFile) { + Out->IWYUExportBy[IncludedFile->getUniqueID()].push_back( + ExportingFileName); + } + if (IncludedHeader && IncludedHeader->kind() == Header::Standard) { + Out->StdIWYUExportBy[IncludedHeader->standard()].push_back( + ExportingFileName); + } + }; if (ExportStack.empty()) return; auto &Top = ExportStack.back(); @@ -240,20 +251,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler { // Make sure current include is covered by the export pragma. if ((Top.Block && HashLine > Top.SeenAtLine) || Top.SeenAtLine == HashLine) { - if (IncludedHeader) { - switch (IncludedHeader->kind()) { - case Header::Physical: - Out->IWYUExportBy[IncludedHeader->physical().getUniqueID()] - .push_back(Top.Path); - break; - case Header::Standard: - Out->StdIWYUExportBy[IncludedHeader->standard()].push_back(Top.Path); - break; - case Header::Verbatim: - assert(false && "unexpected Verbatim header"); - break; - } - } + AddExport(); // main-file #include with export pragma should never be removed. if (Top.SeenAtFile == SM.getMainFileID() && IncludedFile) Out->ShouldKeep.insert(IncludedFile->getUniqueID()); diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp index 36850731d5145..dfefa66887b0f 100644 --- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp @@ -452,6 +452,8 @@ TEST_F(PragmaIncludeTest, IWYUExportForStandardHeaders) { auto &FM = Processed.fileManager(); EXPECT_THAT(PI.getExporters(*tooling::stdlib::Header::named(""), FM), testing::UnorderedElementsAre(FileNamed("export.h"))); + EXPECT_THAT(PI.getExporters(llvm::cantFail(FM.getFileRef("string")), FM), + testing::UnorderedElementsAre(FileNamed("export.h"))); } TEST_F(PragmaIncludeTest, IWYUExportBlock) { From e033f5fa4a1edc4d266f125933dbd0e96c1da304 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Tue, 14 Nov 2023 10:42:55 +0100 Subject: [PATCH 2/2] [include-cleaner] Treat include_next as exporting Most of the time the included-header cannot be just `#include`d from user code, and providing findings asking for the inclusion of the inner header seems wrong. We have enough explicit intent from the developer around included header and including header being related via use of include_next directive, instead of a regular include. So this patch proposes to treat the inner header as being exported so that the user can only include the outer header. This also works nicely when the user is transitively relying on inner header, we'll still suggest including that one and if the spelling of inner and outer headers are different that inclusion will be correct. Otherwise if the spellings are the same, we'll treat the outer header as the provider. --- .../include-cleaner/lib/Record.cpp | 23 +++++++------ .../include-cleaner/unittests/RecordTest.cpp | 32 +++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp index a98129452e2bd..0aa0fbe0e1060 100644 --- a/clang-tools-extra/include-cleaner/lib/Record.cpp +++ b/clang-tools-extra/include-cleaner/lib/Record.cpp @@ -17,6 +17,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/Specifiers.h" +#include "clang/Basic/TokenKinds.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Lex/DirectoryLookup.h" #include "clang/Lex/MacroInfo.h" @@ -27,19 +28,15 @@ #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" -#include "llvm/ADT/iterator_range.h" #include "llvm/Support/Allocator.h" -#include "llvm/Support/Error.h" #include "llvm/Support/FileSystem/UniqueID.h" #include "llvm/Support/StringSaver.h" #include #include #include #include -#include #include #include @@ -225,17 +222,20 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler { } if (!IncludedHeader && File) IncludedHeader = *File; - checkForExport(HashFID, HashLine, std::move(IncludedHeader), File); + checkForExport(HashFID, HashLine, std::move(IncludedHeader), File, + IncludeTok); checkForKeep(HashLine, File); } void checkForExport(FileID IncludingFile, int HashLine, std::optional
IncludedHeader, - OptionalFileEntryRef IncludedFile) { - auto AddExport = [&] { + OptionalFileEntryRef IncludedFile, + const Token &IncludeTok) { + // Adds an export for the IncludedFile from IncludingFile. + auto AddExport = [&]() { auto ExportingFileName = SM.getFileEntryForID(IncludingFile)->getName(); if (IncludedFile) { - Out->IWYUExportBy[IncludedFile->getUniqueID()].push_back( + Out->IWYUExportBy[IncludedFile->getFileEntry().getUniqueID()].push_back( ExportingFileName); } if (IncludedHeader && IncludedHeader->kind() == Header::Standard) { @@ -243,8 +243,13 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler { ExportingFileName); } }; - if (ExportStack.empty()) + if (ExportStack.empty()) { + if (IncludeTok.getIdentifierInfo()->getPPKeywordID() == + tok::pp_include_next) { + AddExport(); + } return; + } auto &Top = ExportStack.back(); if (Top.SeenAtFile != IncludingFile) return; diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp index dfefa66887b0f..9ba377987e0b6 100644 --- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp @@ -558,5 +558,37 @@ TEST_F(PragmaIncludeTest, ExportInUnnamedBuffer) { PI.getExporters(llvm::cantFail(FM->getFileRef("foo.h")), *FM), testing::ElementsAre(llvm::cantFail(FM->getFileRef("exporter.h")))); } + +TEST_F(PragmaIncludeTest, ExportIncludeNext) { + llvm::StringLiteral Filename = "test.cpp"; + auto Code = R"cpp(#include )cpp"; + Inputs.ExtraFiles["foo/new"] = R"cpp( + #pragma once + #include_next + )cpp"; + Inputs.ExtraFiles["inner/new"] = "#pragma once"; + + auto Clang = std::make_unique( + std::make_shared()); + Clang->createDiagnostics(); + + Clang->setInvocation(std::make_unique()); + ASSERT_TRUE(CompilerInvocation::CreateFromArgs( + Clang->getInvocation(), {"-Ifoo/", "-Iinner/", Filename.data()}, + Clang->getDiagnostics(), "clang")); + + // Create unnamed memory buffers for all the files. + auto VFS = llvm::makeIntrusiveRefCnt(); + VFS->addFile(Filename, /*ModificationTime=*/0, + llvm::MemoryBuffer::getMemBufferCopy(Code, /*BufferName=*/"")); + for (const auto &Extra : Inputs.ExtraFiles) + VFS->addFile(Extra.getKey(), /*ModificationTime=*/0, + llvm::MemoryBuffer::getMemBufferCopy(Extra.getValue(), + /*BufferName=*/"")); + auto *FM = Clang->createFileManager(VFS); + ASSERT_TRUE(Clang->ExecuteAction(*Inputs.MakeAction())); + EXPECT_THAT(PI.getExporters(llvm::cantFail(FM->getFileRef("inner/new")), *FM), + testing::ElementsAre(llvm::cantFail(FM->getFileRef("foo/new")))); +} } // namespace } // namespace clang::include_cleaner