[Clangd] Fixed SelectionTree bug for macros

[SureYeaah]

Summary

Fixed SelectionTree bug for macros

• Fixed SelectionTree claimRange for macros and template instantiations
• Fixed SelectionTree unit tests

[SureYeaah]

Removed debugging code

[SureYeaah]

Removed extra includes

[sammccall]

clang-tools-extra/clangd/Selection.cpp

245

the new wording seems less clear to me

clang-tools-extra/clangd/SourceCode.cpp

447 ↗

(On Diff #208389)

It looks like your editor might be set up to format the whole file, rather than just changed lines (git clang-format will also do this).

Please avoid this in general as it messes up blame history.
No need to go back and revert everything though.

655 ↗

(On Diff #208389)

This is Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts), I think

663 ↗

(On Diff #208389)

Returns a half-open CharSourceRange.
(Because closed character CharSourceRanges are also a thing...)

683 ↗

(On Diff #208389)

nit: do the conversion from the previous line here, and write the loop as a regular while loop?

clang-tools-extra/clangd/SourceCode.h

204 ↗

(On Diff #208389)

This is closely related to toHalfOpenFileRange and should be moved up there.

Actually... is this just a less-buggy replacement for toHalfOpenFileRange? If so, we should give it that name. It'd be nice to add a few unit tests (and verify that they fail with the old version of the function).

205 ↗

(On Diff #208389)

"end of the last token" is confusing: does it point to the last char, or one-past-last?

The latter seems nicest and is easily described by the existing "half-open" name

[SureYeaah]

Changed claimRange to use toHalfOpenFileRange

[SureYeaah]

Changed a breaking test in ExtractVariable

[SureYeaah]

Added a test case

[sammccall]

Just doc nits I think.

This eliminates a semi-fast path (that uses approximate token matching to avoid running the lexer in claimRange sometimes) but I think we can live without it.

clang-tools-extra/clangd/Selection.cpp

242

I'm not sure why the FIXME is on this function, it can't be fixed here.

If you're not sure where to put it, I'd suggest putting the FIXME on the unit test that shows the wrong behavior.

257–258

The rest of this function talks about cheap vs precise checks etc - you've removed the cheap path (which is OK to improve correctness, I think), so the comments need to be rewritten

[SureYeaah]

Updated comments

[sammccall]

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

399

can you uncomment this case, and assert the actual behavior? The FIXME should say what it should be instead.

clang-tools-extra/clangd/Selection.cpp

 //===--- Selection.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Selection.h"
 #include "ClangdUnit.h"
+#include "SourceCode.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
 #include "llvm/ADT/STLExtras.h"
 #include <algorithm>
 
 namespace clang {
 namespace clangd {
 namespace {
 using Node = SelectionTree::Node;
 using ast_type_traits::DynTypedNode;
if (const Decl *D = N.get<Decl>()) {
         return VD->getLocation();
     }
     return SourceRange();
   }
 
   // Perform hit-testing of a complete Node against the selection.
   // This runs for every node in the AST, and must be fast in common cases.
   // This is usually called from pop(), so we can take children into account.
   SelectionTree::Selection claimRange(SourceRange S) {
     if (!S.isValid())
       return SelectionTree::Unselected;
-    // getTopMacroCallerLoc() allows selection of constructs in macro args. e.g:
+    // toHalfOpenFileRange() allows selection of constructs in macro args. e.g:
     //   #define LOOP_FOREVER(Body) for(;;) { Body }
     //   void IncrementLots(int &x) {
     //     LOOP_FOREVER( ++x; )
     //   }
     // Selecting "++x" or "x" will do the right thing.
-    auto B = SM.getDecomposedLoc(SM.getTopMacroCallerLoc(S.getBegin()));
-    auto E = SM.getDecomposedLoc(SM.getTopMacroCallerLoc(S.getEnd()));
+    auto Range = toHalfOpenFileRange(SM, LangOpts, S);
+    assert(Range && "We should be able to get the File Range");
+    auto B = SM.getDecomposedLoc(Range->getBegin());
+    auto E = SM.getDecomposedLoc(Range->getEnd());
     // Otherwise, nodes in macro expansions can't be selected.
     if (B.first != SelFile || E.first != SelFile)
       return SelectionTree::Unselected;
-    // Cheap test: is there any overlap at all between the selection and range?
-    // Note that E.second is the *start* of the last token, which is why we
-    // compare against the "rounded-down" SelBegin.
-    if (B.second >= SelEnd || E.second < SelBeginTokenStart)
+    // Is there any overlap at all between the selection and range?
+    if (B.second >= SelEnd || E.second < SelBegin)
       return SelectionTree::Unselected;
-
-    // We may have hit something, need some more precise checks.
-    // Adjust [B, E) to be a half-open character range.
-    E.second += Lexer::MeasureTokenLength(S.getEnd(), SM, LangOpts);
+    // We may have hit something.
     auto PreciseBounds = std::make_pair(B.second, E.second);
     // Trim range using the selection, drop it if empty.
     B.second = std::max(B.second, SelBegin);
     E.second = std::min(E.second, SelEnd);
     if (B.second >= E.second)
       return SelectionTree::Unselected;
     // Attempt to claim the remaining range. If there's nothing to claim, only
     // children were selected.

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

default:
     ADD_FAILURE() << "Expected 1-2 points for selection.\n" << MarkedCode;
     return SelectionTree(AST.getASTContext(), 0u, 0u);
   }
 }
 
 Range nodeRange(const SelectionTree::Node *N, ParsedAST &AST) {
   if (!N)
     return Range{};
-  SourceManager &SM = AST.getSourceManager();
+  const SourceManager &SM = AST.getSourceManager();
+  const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
   StringRef Buffer = SM.getBufferData(SM.getMainFileID());
-  SourceRange SR = N->ASTNode.getSourceRange();
-  SR.setBegin(SM.getFileLoc(SR.getBegin()));
-  SR.setEnd(SM.getFileLoc(SR.getEnd()));
-  CharSourceRange R =
-      Lexer::getAsCharRange(SR, SM, AST.getASTContext().getLangOpts());
-  return Range{offsetToPosition(Buffer, SM.getFileOffset(R.getBegin())),
-               offsetToPosition(Buffer, SM.getFileOffset(R.getEnd()))};
+  auto FileRange =
+      toHalfOpenFileRange(SM, LangOpts, N->ASTNode.getSourceRange());
+  assert(FileRange && "We should be able to get the File Range");
+  return Range{
+      offsetToPosition(Buffer, SM.getFileOffset(FileRange->getBegin())),
+      offsetToPosition(Buffer, SM.getFileOffset(FileRange->getEnd()))};
 }
 
 std::string nodeKind(const SelectionTree::Node *N) {
   if (!N)
     return "<null>";
   return N->ASTNode.getNodeKind().asStringRef().str();
 }
 
Case Cases[] = {
             void bar() { CALL_FUNCTION([[f^o^o]]); }
           )cpp",
           "DeclRefExpr",
       },
       {
           R"cpp(
             void foo();
             #define CALL_FUNCTION(X) X()
-            void bar() [[{ CALL_FUNC^TION(fo^o); }]]
+            void bar() { [[CALL_FUNC^TION(fo^o)]]; }
           )cpp",
-          "CompoundStmt",
+          "CallExpr",
       },
       {
           R"cpp(
             void foo();
             #define CALL_FUNCTION(X) X()
-            void bar() [[{ C^ALL_FUNC^TION(foo); }]]
+            void bar() { [[C^ALL_FUNC^TION(foo)]]; }
           )cpp",
-          "CompoundStmt",
+          "CallExpr",
       },
       {
           R"cpp(
             void foo();
             #define CALL_FUNCTION(X) X^()^
             void bar() { CALL_FUNCTION(foo); }
           )cpp",
           nullptr,
TEST(SelectionTest, InjectedClassName) {
   const char* Code = "struct ^X { int x; };";
   auto AST = TestTU::withCode(Annotations(Code).code()).build();
   auto T = makeSelectionTree(Code, AST);
   ASSERT_EQ("CXXRecordDecl", nodeKind(T.commonAncestor())) << T;
   auto *D = dyn_cast<CXXRecordDecl>(T.commonAncestor()->ASTNode.get<Decl>());
   EXPECT_FALSE(D->isInjectedClassName());
 }
 
+// FIXME: Doesn't select the binary operator node in
+//          #define FOO(X) X + 1
+//          int a, b = [[FOO(a)]];
 TEST(SelectionTest, Selected) {
   // Selection with ^marks^.
   // Partially selected nodes marked with a [[range]].
   // Completely selected nodes marked with a $C[[range]].
   const char *Cases[] = {
       R"cpp( int abc, xyz = [[^ab^c]]; )cpp",
       R"cpp( int abc, xyz = [[a^bc^]]; )cpp",
       R"cpp( int abc, xyz = $C[[^abc^]]; )cpp",
       R"cpp(
         void foo() {
           [[if ([[1^11]]) $C[[{
             $C[[return]];
           }]] else [[{^
           }]]]]
         }
       )cpp",
       R"cpp(
           template <class T>
           struct unique_ptr {};
-          void foo(^$C[[unique_ptr<unique_ptr<$C[[int]]>>]]^ a) {}
+          void foo(^$C[[unique_ptr<$C[[unique_ptr<$C[[int]]>]]>]]^ a) {}
+      )cpp",
+      R"cpp(int a = [[5 >^> 1]];)cpp",
+      R"cpp(
+        #define ECHO(X) X
+        ECHO(EC^HO([[$C[[int]]) EC^HO(a]]));
       )cpp",
   };
   for (const char *C : Cases) {
     Annotations Test(C);
     auto AST = TestTU::withCode(Test.code()).build();
     auto T = makeSelectionTree(C, AST);
 
     std::vector<Range> Complete, Partial;

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

const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
                            case 1:
                              a = dummy;
                              break;
                            default:
                              break;
                        }
                  })cpp"},*/
           // ensure InsertionPoint isn't inside a macro
-          {R"cpp(#define LOOP(x) {int a = x + 1;}
+          // FIXME: SelectionTree needs to be fixed for macros
+          /*{R"cpp(#define LOOP(x) while (1) {a = x;}
                  void f(int a) {
                    if(1)
                     LOOP(5 + ^3)
                  })cpp",
-           R"cpp(#define LOOP(x) {int a = x + 1;}
+             R"cpp(#define LOOP(x) while (1) {a = x;}
+                 void f(int a) {
+                   auto dummy = 3; if(1)
+                    LOOP(5 + dummy)
+                 })cpp"},*/
+          {R"cpp(#define LOOP(x) do {x;} while(1);
+                 void f(int a) {
+                   if(1)
+                    LOOP(5 + ^3)
+                 })cpp",
+           R"cpp(#define LOOP(x) do {x;} while(1);
                  void f(int a) {
                    auto dummy = 3; if(1)
                     LOOP(5 + dummy)
                  })cpp"},
           // label and attribute testing
           {R"cpp(void f(int a) {
                     label: [ [gsl::suppress("type")] ] for (;;) a = ^1;
                  })cpp",
            R"cpp(void f(int a) {
                     auto dummy = 1; label: [ [gsl::suppress("type")] ] for (;;) a = dummy;
                  })cpp"},
           // FIXME: Doesn't work because bug in selection tree
           /*{R"cpp(#define PLUS(x) x++
                  void f(int a) {
                    PLUS(^a);
                  })cpp",
            R"cpp(#define PLUS(x) x++
                  void f(int a) {
                    auto dummy = a; PLUS(dummy);
                  })cpp"},*/
-          // FIXME: Doesn't work correctly for \[\[clang::uninitialized\]\] int b
-          // = 1; since the attr is inside the DeclStmt and the bounds of
-          // DeclStmt don't cover the attribute
+          // FIXME: Doesn't work correctly for this code since the attr is
+          // inside the DeclStmt and the bounds of DeclStmt don't cover the attr
+          // \[\[clang::uninitialized\]\] int b = 1;
       };
   for (const auto &IO : InputOutputs) {
     checkTransform(ID, IO.first, IO.second);
   }
 }
 
 TEST(TweakTest, AnnotateHighlightings) {
   llvm::StringLiteral ID = "AnnotateHighlightings";