[clangd] IncludeCleaner: Handle macros coming from ScratchBuffer

[kbobyrev]

Mention the ScratchBuffer explicitly in the test.

[kbobyrev]

Rename the test to have more meaning.

[sammccall]

clang-tools-extra/clangd/IncludeCleaner.cpp

123–124

Add a comment

123–124

I'm not 100% sure these should be unconditional. It's *probably* right, but...
Can you add a test of the form:

#define ab x
#define concat(x, y) x##y
int foo(a, b);

and verify that we get the expected set of file IDs when seeding with the location of the VarDecl x?

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

170

this doesn't seem to test very much, a comment should indicate that this is guarding against a crash.

Ideally we'd (also?) have a test that calls findReferencedFiles and actually assert which IDs we get, rather than run some big blob of code and hope not to crash.

[kbobyrev]

Address review comments.

[kbobyrev]

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

170

Good point, I just ran into this for D111711:

foo.h

void foo();

foo.cpp

void foo() {}

computeUnusedIncludes() does not return foo.h but it wasn't processed (e.g. its ID was never in the ReferencedFiles in the first place) :(

[sammccall]

LG with the new test

clang-tools-extra/clangd/IncludeCleaner.cpp

123–124

This is about token pasting, nested macro expansions are neither necessary nor sufficient.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

198

Nit: I think this should be asserting on FileIDs (i.e. before translation). It's narrower, and the contract is that findReferecedFiles should filter them out already.

198

Comment: no "<scratch space>" FID

199

Comment: should not crash due to "files" missing from include structure

[kbobyrev]

Stash

[kbobyrev]

Resolve review comments.

[kbobyrev]

Fix comment.

clang-tools-extra/clangd/IncludeCleaner.cpp

void add(FileID FID, SourceLocation Loc) {
     assert(SM.isInFileID(Loc, FID));
     if (Loc.isFileID()) {
       Files.insert(FID);
       return;
     }
     // Don't process the same macro FID twice.
     if (!Macros.insert(FID).second)
       return;
     const auto &Exp = SM.getSLocEntry(FID).getExpansion();
+    // For token pasting operator in macros, spelling and expansion locations
+    // can be within a temporary buffer that Clang creates (scratch space or
+    // ScratchBuffer). That is not a real file we can include.
+    if (!SM.isWrittenInScratchSpace(Exp.getSpellingLoc()))
       add(Exp.getSpellingLoc());
+    if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocStart()))
       add(Exp.getExpansionLocStart());
+    if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocEnd()))
       add(Exp.getExpansionLocEnd());
   }
 };
 
 } // namespace
 
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
   ReferencedLocations Result;
   ReferencedLocationCrawler Crawler(Result);

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

TEST(IncludeCleaner, GetUnusedHeaders) {
   std::vector<std::string> UnusedHeaders;
   UnusedHeaders.reserve(UnusedIncludes.size());
   for (const auto &Include : UnusedIncludes)
     UnusedHeaders.push_back(Include->Written);
   EXPECT_THAT(UnusedHeaders,
               UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
 }
 
+TEST(IncludeCleaner, ScratchBuffer) {
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.Code = R"cpp(
+    #include "macro_spelling_in_scratch_buffer.h"
+
+    using flags::FLAGS_FOO;
+
+    int concat(a, b) = 42;
+    )cpp";
+  // The pasting operator in combination with DEFINE_FLAG will create
+  // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
+  // FileEntry.
+  TU.AdditionalFiles["macro_spelling_in_scratch_buffer.h"] = R"cpp(
+    #define DEFINE_FLAG(X) \
+    namespace flags { \
+    int FLAGS_##X; \
+    } \
+
+    DEFINE_FLAG(FOO)
+
+    #define ab x
+    #define concat(x, y) x##y
+    )cpp";
+  ParsedAST AST = TU.build();
+  auto &SM = AST.getSourceManager();
+  auto &Includes = AST.getIncludeStructure();
+  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto Entry = SM.getFileManager().getFile(
+      testPath("macro_spelling_in_scratch_buffer.h"));
+  ASSERT_TRUE(Entry);
+  auto FID = SM.translateFile(*Entry);
+  // No "<scratch space>" FID.
+  EXPECT_THAT(ReferencedFiles, UnorderedElementsAre(FID));
+  // Should not crash due to <scratch space> "files" missing from include
+  // structure.
+  EXPECT_THAT(
+      getUnused(Includes, translateToHeaderIDs(ReferencedFiles, Includes, SM)),
+      ::testing::IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang