aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSam McCall <sam.mccall@gmail.com>2024-06-19 15:29:05 +0200
committerGitHub <noreply@github.com>2024-06-19 15:29:05 +0200
commit5574a5894fdb7f9a46a4fbe6c8970fd39890dc9b (patch)
tree5a0e00f583272225f442802389bc83adea21d9d3
parentca423a26e7bfc31a36c9ad790b0ae1bb9be18836 (diff)
downloadllvm-5574a5894fdb7f9a46a4fbe6c8970fd39890dc9b.zip
llvm-5574a5894fdb7f9a46a4fbe6c8970fd39890dc9b.tar.gz
llvm-5574a5894fdb7f9a46a4fbe6c8970fd39890dc9b.tar.bz2
[include-cleaner] don't consider the associated header unused (#67228)
Loosely, the "associated header" of `foo.cpp` is `foo.h`. It should be included, many styles include it first. So far we haven't special cased it in any way, and require this include to be justified. e.g. if foo.cpp defines a function declared in foo.h, then the #include is allowed to check these declarations match. However this doesn't really align with what users want: - people reasonably want to include the associated header for the side-effect of validating that it compiles. In the degenerate case, `lib.cpp`is just `#include "lib.h"` (see bug) - That `void foo(){}` IWYU-uses `void foo();` is a bit artificial, and most users won't internalize this. Instead they'll stick with the simpler model "include the header that defines your API". In the rare cases where these give different answers[1], our current behavior is a puzzling special case from the user POV. It is more user-friendly to accept both models. - even where this diagnostic is a true positive (defs don't match header decls) the diagnostic does not communicate this usefully. Fixes https://github.com/llvm/llvm-project/issues/67140 [1] Example of an associated header that's not IWYU-used: ``` // http.h inline URL buildHttpURL(string host, int port, string path) { return "http://" + host + ":" + port + "/" + path; } // http.cpp class HTTPURLHandler : URLHandler { ... }; REGISTER_URL_HANDLER("http", HTTPURLHandler); ```
-rw-r--r--clang-tools-extra/include-cleaner/lib/Record.cpp36
-rw-r--r--clang-tools-extra/include-cleaner/unittests/RecordTest.cpp59
2 files changed, 89 insertions, 6 deletions
diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp
index 78a4df6..6b5be95 100644
--- a/clang-tools-extra/include-cleaner/lib/Record.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -34,6 +34,7 @@
#include "llvm/Support/Allocator.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem/UniqueID.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/StringSaver.h"
#include <algorithm>
#include <assert.h>
@@ -180,7 +181,9 @@ public:
RecordPragma(const Preprocessor &P, PragmaIncludes *Out)
: SM(P.getSourceManager()), HeaderInfo(P.getHeaderSearchInfo()), Out(Out),
Arena(std::make_shared<llvm::BumpPtrAllocator>()),
- UniqueStrings(*Arena) {}
+ UniqueStrings(*Arena),
+ MainFileStem(llvm::sys::path::stem(
+ SM.getNonBuiltinFilenameForID(SM.getMainFileID()).value_or(""))) {}
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -228,8 +231,9 @@ public:
}
if (!IncludedHeader && File)
IncludedHeader = *File;
- checkForExport(HashFID, HashLine, std::move(IncludedHeader), File);
+ checkForExport(HashFID, HashLine, IncludedHeader, File);
checkForKeep(HashLine, File);
+ checkForDeducedAssociated(IncludedHeader);
}
void checkForExport(FileID IncludingFile, int HashLine,
@@ -269,6 +273,27 @@ public:
KeepStack.pop_back(); // Pop immediately for single-line keep pragma.
}
+ // Consider marking H as the "associated header" of the main file.
+ //
+ // Our heuristic:
+ // - it must be the first #include in the main file
+ // - it must have the same name stem as the main file (foo.h and foo.cpp)
+ // (IWYU pragma: associated is also supported, just not by this function).
+ //
+ // We consider the associated header as if it had a keep pragma.
+ // (Unlike IWYU, we don't treat #includes inside the associated header as if
+ // they were written in the main file.)
+ void checkForDeducedAssociated(std::optional<Header> H) {
+ namespace path = llvm::sys::path;
+ if (!InMainFile || SeenAssociatedCandidate)
+ return;
+ SeenAssociatedCandidate = true; // Only the first #include is our candidate.
+ if (!H || H->kind() != Header::Physical)
+ return;
+ if (path::stem(H->physical().getName(), path::Style::posix) == MainFileStem)
+ Out->ShouldKeep.insert(H->physical().getUniqueID());
+ }
+
bool HandleComment(Preprocessor &PP, SourceRange Range) override {
auto &SM = PP.getSourceManager();
auto Pragma =
@@ -280,7 +305,9 @@ public:
int CommentLine = SM.getLineNumber(CommentFID, CommentOffset);
if (InMainFile) {
- if (Pragma->starts_with("keep")) {
+ if (Pragma->starts_with("keep") ||
+ // Limited support for associated headers: never consider unused.
+ Pragma->starts_with("associated")) {
KeepStack.push_back({CommentLine, false});
} else if (Pragma->starts_with("begin_keep")) {
KeepStack.push_back({CommentLine, true});
@@ -342,6 +369,9 @@ private:
std::shared_ptr<llvm::BumpPtrAllocator> Arena;
/// Intern table for strings. Contents are on the arena.
llvm::StringSaver UniqueStrings;
+ // Used when deducing associated header.
+ llvm::StringRef MainFileStem;
+ bool SeenAssociatedCandidate = false;
struct ExportPragma {
// The line number where we saw the begin_exports or export pragma.
diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
index d1f7590..1a5996e 100644
--- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
@@ -316,7 +316,10 @@ protected:
};
}
- TestAST build() { return TestAST(Inputs); }
+ TestAST build(bool ResetPragmaIncludes = true) {
+ if (ResetPragmaIncludes) PI = PragmaIncludes();
+ return TestAST(Inputs);
+ }
void createEmptyFiles(llvm::ArrayRef<StringRef> FileNames) {
for (llvm::StringRef File : FileNames)
@@ -379,6 +382,56 @@ TEST_F(PragmaIncludeTest, IWYUKeep) {
EXPECT_TRUE(PI.shouldKeep(FM.getFile("std/set").get()));
}
+TEST_F(PragmaIncludeTest, AssociatedHeader) {
+ createEmptyFiles({"foo/main.h", "bar/main.h", "bar/other.h", "std/vector"});
+ auto IsKeep = [&](llvm::StringRef Name, TestAST &AST) {
+ return PI.shouldKeep(AST.fileManager().getFile(Name).get());
+ };
+
+ Inputs.FileName = "main.cc";
+ Inputs.ExtraArgs.push_back("-isystemstd");
+ {
+ Inputs.Code = R"cpp(
+ #include "foo/main.h"
+ #include "bar/main.h"
+ )cpp";
+ auto AST = build();
+ EXPECT_TRUE(IsKeep("foo/main.h", AST));
+ EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
+ }
+
+ {
+ Inputs.Code = R"cpp(
+ #include "bar/other.h"
+ #include "bar/main.h"
+ )cpp";
+ auto AST = build();
+ EXPECT_FALSE(IsKeep("bar/other.h", AST));
+ EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
+ }
+
+ {
+ Inputs.Code = R"cpp(
+ #include "foo/main.h"
+ #include "bar/other.h" // IWYU pragma: associated
+ #include <vector> // IWYU pragma: associated
+ )cpp";
+ auto AST = build();
+ EXPECT_TRUE(IsKeep("foo/main.h", AST));
+ EXPECT_TRUE(IsKeep("bar/other.h", AST));
+ EXPECT_TRUE(IsKeep("std/vector", AST));
+ }
+
+ Inputs.FileName = "vector.cc";
+ {
+ Inputs.Code = R"cpp(
+ #include <vector>
+ )cpp";
+ auto AST = build();
+ EXPECT_FALSE(IsKeep("std/vector", AST)) << "stdlib is not associated";
+ }
+}
+
TEST_F(PragmaIncludeTest, IWYUPrivate) {
Inputs.Code = R"cpp(
#include "public.h"
@@ -577,7 +630,7 @@ TEST_F(PragmaIncludeTest, OutlivesFMAndSM) {
Inputs.MakeAction = nullptr; // Don't populate PI anymore.
// Now this build gives us a new File&Source Manager.
- TestAST Processed = build();
+ TestAST Processed = build(/*ResetPragmaIncludes=*/false);
auto &FM = Processed.fileManager();
auto PrivateFE = FM.getFile("private.h");
assert(PrivateFE);
@@ -610,7 +663,7 @@ TEST_F(PragmaIncludeTest, CanRecordManyTimes) {
// any IWYU pragmas. Make sure strings from previous recordings are still
// alive.
Inputs.Code = "";
- build();
+ build(/*ResetPragmaIncludes=*/false);
EXPECT_EQ(Public, "\"public.h\"");
}
} // namespace