Skip to content

Commit 5574a58

Browse files
authored
[include-cleaner] don't consider the associated header unused (llvm#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 llvm#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); ```
1 parent ca423a2 commit 5574a58

File tree

2 files changed

+89
-6
lines changed

2 files changed

+89
-6
lines changed

clang-tools-extra/include-cleaner/lib/Record.cpp

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#include "llvm/Support/Allocator.h"
3535
#include "llvm/Support/Error.h"
3636
#include "llvm/Support/FileSystem/UniqueID.h"
37+
#include "llvm/Support/Path.h"
3738
#include "llvm/Support/StringSaver.h"
3839
#include <algorithm>
3940
#include <assert.h>
@@ -180,7 +181,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
180181
RecordPragma(const Preprocessor &P, PragmaIncludes *Out)
181182
: SM(P.getSourceManager()), HeaderInfo(P.getHeaderSearchInfo()), Out(Out),
182183
Arena(std::make_shared<llvm::BumpPtrAllocator>()),
183-
UniqueStrings(*Arena) {}
184+
UniqueStrings(*Arena),
185+
MainFileStem(llvm::sys::path::stem(
186+
SM.getNonBuiltinFilenameForID(SM.getMainFileID()).value_or(""))) {}
184187

185188
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
186189
SrcMgr::CharacteristicKind FileType,
@@ -228,8 +231,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
228231
}
229232
if (!IncludedHeader && File)
230233
IncludedHeader = *File;
231-
checkForExport(HashFID, HashLine, std::move(IncludedHeader), File);
234+
checkForExport(HashFID, HashLine, IncludedHeader, File);
232235
checkForKeep(HashLine, File);
236+
checkForDeducedAssociated(IncludedHeader);
233237
}
234238

235239
void checkForExport(FileID IncludingFile, int HashLine,
@@ -269,6 +273,27 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
269273
KeepStack.pop_back(); // Pop immediately for single-line keep pragma.
270274
}
271275

276+
// Consider marking H as the "associated header" of the main file.
277+
//
278+
// Our heuristic:
279+
// - it must be the first #include in the main file
280+
// - it must have the same name stem as the main file (foo.h and foo.cpp)
281+
// (IWYU pragma: associated is also supported, just not by this function).
282+
//
283+
// We consider the associated header as if it had a keep pragma.
284+
// (Unlike IWYU, we don't treat #includes inside the associated header as if
285+
// they were written in the main file.)
286+
void checkForDeducedAssociated(std::optional<Header> H) {
287+
namespace path = llvm::sys::path;
288+
if (!InMainFile || SeenAssociatedCandidate)
289+
return;
290+
SeenAssociatedCandidate = true; // Only the first #include is our candidate.
291+
if (!H || H->kind() != Header::Physical)
292+
return;
293+
if (path::stem(H->physical().getName(), path::Style::posix) == MainFileStem)
294+
Out->ShouldKeep.insert(H->physical().getUniqueID());
295+
}
296+
272297
bool HandleComment(Preprocessor &PP, SourceRange Range) override {
273298
auto &SM = PP.getSourceManager();
274299
auto Pragma =
@@ -280,7 +305,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
280305
int CommentLine = SM.getLineNumber(CommentFID, CommentOffset);
281306

282307
if (InMainFile) {
283-
if (Pragma->starts_with("keep")) {
308+
if (Pragma->starts_with("keep") ||
309+
// Limited support for associated headers: never consider unused.
310+
Pragma->starts_with("associated")) {
284311
KeepStack.push_back({CommentLine, false});
285312
} else if (Pragma->starts_with("begin_keep")) {
286313
KeepStack.push_back({CommentLine, true});
@@ -342,6 +369,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
342369
std::shared_ptr<llvm::BumpPtrAllocator> Arena;
343370
/// Intern table for strings. Contents are on the arena.
344371
llvm::StringSaver UniqueStrings;
372+
// Used when deducing associated header.
373+
llvm::StringRef MainFileStem;
374+
bool SeenAssociatedCandidate = false;
345375

346376
struct ExportPragma {
347377
// The line number where we saw the begin_exports or export pragma.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,10 @@ class PragmaIncludeTest : public ::testing::Test {
316316
};
317317
}
318318

319-
TestAST build() { return TestAST(Inputs); }
319+
TestAST build(bool ResetPragmaIncludes = true) {
320+
if (ResetPragmaIncludes) PI = PragmaIncludes();
321+
return TestAST(Inputs);
322+
}
320323

321324
void createEmptyFiles(llvm::ArrayRef<StringRef> FileNames) {
322325
for (llvm::StringRef File : FileNames)
@@ -379,6 +382,56 @@ TEST_F(PragmaIncludeTest, IWYUKeep) {
379382
EXPECT_TRUE(PI.shouldKeep(FM.getFile("std/set").get()));
380383
}
381384

385+
TEST_F(PragmaIncludeTest, AssociatedHeader) {
386+
createEmptyFiles({"foo/main.h", "bar/main.h", "bar/other.h", "std/vector"});
387+
auto IsKeep = [&](llvm::StringRef Name, TestAST &AST) {
388+
return PI.shouldKeep(AST.fileManager().getFile(Name).get());
389+
};
390+
391+
Inputs.FileName = "main.cc";
392+
Inputs.ExtraArgs.push_back("-isystemstd");
393+
{
394+
Inputs.Code = R"cpp(
395+
#include "foo/main.h"
396+
#include "bar/main.h"
397+
)cpp";
398+
auto AST = build();
399+
EXPECT_TRUE(IsKeep("foo/main.h", AST));
400+
EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
401+
}
402+
403+
{
404+
Inputs.Code = R"cpp(
405+
#include "bar/other.h"
406+
#include "bar/main.h"
407+
)cpp";
408+
auto AST = build();
409+
EXPECT_FALSE(IsKeep("bar/other.h", AST));
410+
EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
411+
}
412+
413+
{
414+
Inputs.Code = R"cpp(
415+
#include "foo/main.h"
416+
#include "bar/other.h" // IWYU pragma: associated
417+
#include <vector> // IWYU pragma: associated
418+
)cpp";
419+
auto AST = build();
420+
EXPECT_TRUE(IsKeep("foo/main.h", AST));
421+
EXPECT_TRUE(IsKeep("bar/other.h", AST));
422+
EXPECT_TRUE(IsKeep("std/vector", AST));
423+
}
424+
425+
Inputs.FileName = "vector.cc";
426+
{
427+
Inputs.Code = R"cpp(
428+
#include <vector>
429+
)cpp";
430+
auto AST = build();
431+
EXPECT_FALSE(IsKeep("std/vector", AST)) << "stdlib is not associated";
432+
}
433+
}
434+
382435
TEST_F(PragmaIncludeTest, IWYUPrivate) {
383436
Inputs.Code = R"cpp(
384437
#include "public.h"
@@ -577,7 +630,7 @@ TEST_F(PragmaIncludeTest, OutlivesFMAndSM) {
577630
Inputs.MakeAction = nullptr; // Don't populate PI anymore.
578631

579632
// Now this build gives us a new File&Source Manager.
580-
TestAST Processed = build();
633+
TestAST Processed = build(/*ResetPragmaIncludes=*/false);
581634
auto &FM = Processed.fileManager();
582635
auto PrivateFE = FM.getFile("private.h");
583636
assert(PrivateFE);
@@ -610,7 +663,7 @@ TEST_F(PragmaIncludeTest, CanRecordManyTimes) {
610663
// any IWYU pragmas. Make sure strings from previous recordings are still
611664
// alive.
612665
Inputs.Code = "";
613-
build();
666+
build(/*ResetPragmaIncludes=*/false);
614667
EXPECT_EQ(Public, "\"public.h\"");
615668
}
616669
} // namespace

0 commit comments

Comments
 (0)