[clang][tooling] Fix early termination when there are nested expansions

[kadircet]

Summary

This also does some cleanups, I am happy to undo them (or send as
separate patches):

• Change the early exit to stop only once we hit an expansion inside the main file, to make sure we keep following the nested expansions.
• Add more tests to cover all the cases mentioned in the implementation
• Drop the adjustments for prev/next tokens. We do the final checks based on the expansion locations anyway, so any intermediate mapping was a no-op.

[sammccall]

clang/lib/Tooling/Syntax/Tokens.cpp

116

(no action needed)

we had extensive discussion of why this change is a no-op, and how to prove it, and what comments to leave.
However in the end I don't think such a comment is necessary, because the code makes sense in isolation, and equivalence-to-previous-version isn't very interesting!

clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp

public:
   // Do not offer code action when operating on a banned namespace.
   EXPECT_UNAVAILABLE(Header + "void fun() { ban::fo^o(); }");
   EXPECT_UNAVAILABLE(Header + "void fun() { ::ban::fo^o(); }");
   EXPECT_AVAILABLE(Header + "void fun() { banana::fo^o(); }");
 
   // NestedNameSpecifier, but no namespace.
   EXPECT_UNAVAILABLE(Header + "class Foo {}; class F^oo foo;");
 
+  // Nested macro case.
+  EXPECT_AVAILABLE(R"cpp(
+  #define ID2(X) X
+  #define ID(Y, X) Y;ID2(X)
+  namespace ns { struct Foo{}; }
+  ID(int xyz, ns::F^oo) f;)cpp");
+
   // Check that we do not trigger in header files.
   FileName = "test.h";
   ExtraArgs.push_back("-xc++-header"); // .h file is treated a C by default.
   EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
   FileName = "test.hpp";
   EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
 }
 

clang/lib/Tooling/Syntax/Tokens.cpp

while (First.isMacroID() && Last.isMacroID()) {
     if (ExpFirst.getExpansionLocStart() != ExpLast.getExpansionLocStart())
       break;
     // Careful, given:
     //   #define HIDE ID(ID(a))
     //   ID(ID(HIDE))
     // The token `a` is wrapped in 4 arg-expansions, we only want to unwrap 2.
     // We distinguish them by whether the macro expands into the target file.
     // Fortunately, the target file ones will always appear first.
-    auto &ExpMacro =
-        SM.getSLocEntry(SM.getFileID(ExpFirst.getExpansionLocStart()))
-            .getExpansion();
-    if (ExpMacro.getExpansionLocStart().isMacroID())
+    auto ExpFileID = SM.getFileID(ExpFirst.getExpansionLocStart());
+    if (ExpFileID == TargetFile)
       break;
     // Replace each endpoint with its spelling inside the macro arg.
     // (This is getImmediateSpellingLoc without repeating lookups).
     First = ExpFirst.getSpellingLoc().getLocWithOffset(DecFirst.second);
     Last = ExpLast.getSpellingLoc().getLocWithOffset(DecLast.second);
-
-    // Now: how do we adjust the previous/next bounds? Three cases:
-    // A) If they are also part of the same macro arg, we translate them too.
-    //   This will ensure that we don't select any macros nested within the
-    //   macro arg that cover extra tokens. Critical case:
-    //      #define ID(X) X
-    //      ID(prev target) // selecting 'target' succeeds
-    //      #define LARGE ID(prev target)
-    //      LARGE // selecting 'target' fails.
-    // B) They are not in the macro at all, then their expansion range is a
-    //    sibling to it, and we can safely substitute that.
-    //      #define PREV prev
-    //      #define ID(X) X
-    //      PREV ID(target) // selecting 'target' succeeds.
-    //      #define LARGE PREV ID(target)
-    //      LARGE // selecting 'target' fails.
-    // C) They are in a different arg of this macro, or the macro body.
-    //    Now selecting the whole macro arg is fine, but the whole macro is not.
-    //    Model this by setting using the edge of the macro call as the bound.
-    //      #define ID2(X, Y) X Y
-    //      ID2(prev, target) // selecting 'target' succeeds
-    //      #define LARGE ID2(prev, target)
-    //      LARGE // selecting 'target' fails
-    auto AdjustBound = [&](SourceLocation &Bound) {
-      if (Bound.isInvalid() || !Bound.isMacroID()) // Non-macro must be case B.
-        return;
-      auto DecBound = SM.getDecomposedLoc(Bound);
-      auto &ExpBound = SM.getSLocEntry(DecBound.first).getExpansion();
-      if (ExpBound.isMacroArgExpansion() &&
-          ExpBound.getExpansionLocStart() == ExpFirst.getExpansionLocStart()) {
-        // Case A: translate to (spelling) loc within the macro arg.
-        Bound = ExpBound.getSpellingLoc().getLocWithOffset(DecBound.second);
-        return;
-      }
-      while (Bound.isMacroID()) {
-        SourceRange Exp = SM.getImmediateExpansionRange(Bound).getAsRange();
-        if (Exp.getBegin() == ExpMacro.getExpansionLocStart()) {
-          // Case B: bounds become the macro call itself.
-          Bound = (&Bound == &Prev) ? Exp.getBegin() : Exp.getEnd();
-          return;
-        }
-        // Either case C, or expansion location will later find case B.
-        // We choose the upper bound for Prev and the lower one for Next:
-        //   ID(prev) target ID(next)
-        //          ^        ^
-        //      new-prev  new-next
-        Bound = (&Bound == &Prev) ? Exp.getEnd() : Exp.getBegin();
-      }
-    };
-    AdjustBound(Prev);
-    AdjustBound(Next);
   }
 
   // In all remaining cases we need the full containing macros.
   // If this overlaps Prev or Next, then no range is possible.
   SourceRange Candidate =
       SM.getExpansionRange(SourceRange(First, Last)).getAsRange();
   auto DecFirst = SM.getDecomposedExpansionLoc(Candidate.getBegin());
-  auto DecLast = SM.getDecomposedLoc(Candidate.getEnd());
+  auto DecLast = SM.getDecomposedExpansionLoc(Candidate.getEnd());
   // Can end up in the wrong file due to bad input or token-pasting shenanigans.
-  if (Candidate.isInvalid() || DecFirst.first != TargetFile || DecLast.first != TargetFile)
+  if (Candidate.isInvalid() || DecFirst.first != TargetFile ||
+      DecLast.first != TargetFile)
     return SourceRange();
   // Check bounds, which may still be inside macros.
   if (Prev.isValid()) {
     auto Dec = SM.getDecomposedLoc(SM.getExpansionRange(Prev).getBegin());
     if (Dec.first != DecFirst.first || Dec.second >= DecFirst.second)
       return SourceRange();
   }
   if (Next.isValid()) {

clang/unittests/Tooling/Syntax/TokensTest.cpp

recordTokens(R"cpp(
     BAR
   )cpp");
   EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("1")),
               ValueIs(SameRange(findSpelled(") BAR").drop_front())));
 
   // Critical cases for mapping of Prev/Next in spelledForExpandedSlow.
   recordTokens(R"cpp(
     #define ID(X) X
-    ID(prev ID(good))
+    ID(prev good)
+    ID(prev ID(good2))
     #define LARGE ID(prev ID(bad))
     LARGE
   )cpp");
   EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
               ValueIs(SameRange(findSpelled("good"))));
+  EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good2")),
+              ValueIs(SameRange(findSpelled("good2"))));
   EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), std::nullopt);
 
   recordTokens(R"cpp(
     #define PREV prev
     #define ID(X) X
     PREV ID(good)
     #define LARGE PREV ID(bad)
     LARGE
   )cpp");
   EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
               ValueIs(SameRange(findSpelled("good"))));
   EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), std::nullopt);
 
   recordTokens(R"cpp(
     #define ID(X) X
     #define ID2(X, Y) X Y
-    ID2(prev, ID(good))
+    ID2(prev, good)
+    ID2(prev, ID(good2))
     #define LARGE ID2(prev, bad)
     LARGE
   )cpp");
   EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
               ValueIs(SameRange(findSpelled("good"))));
+  EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good2")),
+              ValueIs(SameRange(findSpelled("good2"))));
   EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("bad")), std::nullopt);
+
+  // Prev from macro body.
+  recordTokens(R"cpp(
+    #define ID(X) X
+    #define ID2(X, Y) X prev ID(Y)
+    ID2(not_prev, good)
+  )cpp");
+  EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("good")),
+              ValueIs(SameRange(findSpelled("good"))));
+  EXPECT_EQ(Buffer.spelledForExpanded(findExpanded("prev good")), std::nullopt);
 }
 
 TEST_F(TokenBufferTest, ExpandedTokensForRange) {
   recordTokens(R"cpp(
     #define SIGN(X) X##_washere
     A SIGN(B) C SIGN(D) E SIGN(F) G
   )cpp");