Skip to content

Commit d638a56

Browse files
committed
[include-cleaner] don't consider the associated header unused
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 #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 782f1a0 commit d638a56

File tree

2 files changed

+80
-2
lines changed

2 files changed

+80
-2
lines changed

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

Lines changed: 32 additions & 2 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>
@@ -178,7 +179,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
178179
: RecordPragma(CI.getPreprocessor(), Out) {}
179180
RecordPragma(const Preprocessor &P, PragmaIncludes *Out)
180181
: SM(P.getSourceManager()), HeaderInfo(P.getHeaderSearchInfo()), Out(Out),
181-
UniqueStrings(Arena) {}
182+
UniqueStrings(Arena),
183+
MainFileStem(llvm::sys::path::stem(
184+
SM.getNonBuiltinFilenameForID(SM.getMainFileID()).value_or(""))) {}
182185

183186
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
184187
SrcMgr::CharacteristicKind FileType,
@@ -227,6 +230,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
227230
IncludedHeader = *File;
228231
checkForExport(HashFID, HashLine, std::move(IncludedHeader), File);
229232
checkForKeep(HashLine, File);
233+
checkForDeducedAssociated(IncludedHeader);
230234
}
231235

232236
void checkForExport(FileID IncludingFile, int HashLine,
@@ -276,6 +280,27 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
276280
KeepStack.pop_back(); // Pop immediately for single-line keep pragma.
277281
}
278282

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

289314
if (InMainFile) {
290-
if (Pragma->startswith("keep")) {
315+
if (Pragma->startswith("keep") ||
316+
// Limited support for associated headers: never consider unused.
317+
Pragma->startswith("associated")) {
291318
KeepStack.push_back({CommentLine, false});
292319
} else if (Pragma->starts_with("begin_keep")) {
293320
KeepStack.push_back({CommentLine, true});
@@ -348,6 +375,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
348375
llvm::BumpPtrAllocator Arena;
349376
/// Intern table for strings. Contents are on the arena.
350377
llvm::StringSaver UniqueStrings;
378+
// Used when deducing associated header.
379+
llvm::StringRef MainFileStem;
380+
bool SeenAssociatedCandidate = false;
351381

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

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,54 @@ TEST_F(PragmaIncludeTest, IWYUKeep) {
379379
EXPECT_TRUE(PI.shouldKeep(FM.getFile("std/set").get()));
380380
}
381381

382+
TEST_F(PragmaIncludeTest, AssociatedHeader) {
383+
createEmptyFiles({"foo/main.h", "bar/main.h", "bar/other.h", "std/vector"});
384+
auto IsKeep = [&](llvm::StringRef Name, TestAST &AST) {
385+
return PI.shouldKeep(AST.fileManager().getFile(Name).get());
386+
};
387+
388+
Inputs.FileName = "main.cc";
389+
Inputs.ExtraArgs.push_back("-isystemstd");
390+
{
391+
Inputs.Code = R"cpp(
392+
#include "foo/main.h"
393+
#include "bar/main.h"
394+
)cpp";
395+
auto AST = build();
396+
EXPECT_TRUE(IsKeep("foo/main.h", AST));
397+
EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
398+
}
399+
400+
{
401+
Inputs.Code = R"cpp(
402+
#include "bar/other.h"
403+
#include "bar/main.h"
404+
)cpp";
405+
auto AST = build();
406+
EXPECT_FALSE(IsKeep("bar/other.h", AST));
407+
EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
408+
}
409+
410+
{
411+
Inputs.Code = R"cpp(
412+
#include "foo/main.h"
413+
#include "bar/other.h" // IWYU pragma: associated
414+
)cpp";
415+
auto AST = build();
416+
EXPECT_TRUE(IsKeep("foo/main.h", AST));
417+
EXPECT_TRUE(IsKeep("bar/other.h", AST));
418+
}
419+
420+
Inputs.FileName = "vector.cc";
421+
{
422+
Inputs.Code = R"cpp(
423+
#include <vector>
424+
)cpp";
425+
auto AST = build();
426+
EXPECT_FALSE(IsKeep("std/vector", AST)) << "stdlib is never associated";
427+
}
428+
}
429+
382430
TEST_F(PragmaIncludeTest, IWYUPrivate) {
383431
Inputs.Code = R"cpp(
384432
#include "public.h"

0 commit comments

Comments
 (0)