[Clangd] Changed ExtractVariable to only work on non empty selections

[SureYeaah]

Summary

• For now, we don't trigger in any case if it's an empty selection
• Fixed unittests

[SureYeaah]

Added tests

[kadircet]

clang-tools-extra/clangd/refactor/Tweak.h

51

maybe expose a SourceLocation SelectionEnd ?

[SureYeaah]

clang-tools-extra/clangd/refactor/Tweak.h

51

Should I also have a SelectionStart? Or just use Cursor?

[SureYeaah]

Added selectionend

[SureYeaah]

Fixed comment

[kadircet]

clang-tools-extra/clangd/refactor/Tweak.h

51

I think it makes more sense to have SelectionBegin than Cursor, AFAIK we usually don't have a notion of Cursor` when provided with a selection.
It only makes sense for inputs coming from clients like vim, in which you only have a cursor rather then a selection. But I would like to know what @sammccall thinks about it as well.

[sammccall]

clang-tools-extra/clangd/refactor/Tweak.h

51

I'd suggest unsigned SelectionStart, SelectionEnd as it more clearly communicates this is main-file only and can be easily used with Code.

I'd add a FIXME to remove Cursor as it's redundant (we can replace it with a method)

[SureYeaah]

Added SelectionBegin and SelectionEnd to Tweak::Selection

clang-tools-extra/clangd/refactor/Tweak.h

public:
   struct Selection {
     Selection(ParsedAST &AST, unsigned RangeBegin, unsigned RangeEnd);
     /// The text of the active document.
     llvm::StringRef Code;
     /// Parsed AST of the active file.
     ParsedAST *
     /// A location of the cursor in the editor.
     SourceLocation Cursor;
+    /// The Length of the selection
+    unsigned SelectionLength;
     /// The AST nodes that were selected.
     SelectionTree ASTSelection;
     // FIXME: provide a way to get sources and ASTs for other files.
   };
 
   /// Output of a tweak.
   enum Intent {
     /// Apply changes that preserve the behavior of the code.

clang-tools-extra/clangd/refactor/Tweak.cpp

 } // namespace
 
 Tweak::Selection::Selection(ParsedAST &AST, unsigned RangeBegin,
                             unsigned RangeEnd)
     : AST(AST), ASTSelection(AST.getASTContext(), RangeBegin, RangeEnd) {
   auto &SM = AST.getSourceManager();
   Code = SM.getBufferData(SM.getMainFileID());
   Cursor = SM.getComposedLoc(SM.getMainFileID(), RangeBegin);
+  SelectionLength = RangeBegin > RangeEnd ? 0 : RangeEnd - RangeBegin;
 }
 
 std::vector<std::unique_ptr<Tweak>>
 prepareTweaks(const Tweak::Selection &S,
               llvm::function_ref<bool(const Tweak &)> Filter) {
   validateRegistry();
 
   std::vector<std::unique_ptr<Tweak>> Available;

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

private:
   // the expression to extract
   std::unique_ptr<ExtractionContext> Target;
 };
 REGISTER_TWEAK(ExtractVariable)
 bool ExtractVariable::prepare(const Selection &Inputs) {
   const ASTContext &Ctx = Inputs.AST.getASTContext();
   const SourceManager &SM = Inputs.AST.getSourceManager();
   const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
-  if (!N)
+  // we don't trigger on empty selections for now
+  if (!N || !Inputs.SelectionLength)
     return false;
   Target = llvm::make_unique<ExtractionContext>(N, SM, Ctx);
   return Target->isExtractable();
 }
 
 Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
   tooling::Replacements Result;
   // FIXME: get variable name from user or suggest based on type

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

TEST(TweakTest, DumpRecordLayout) {
   const char *Input = "struct ^X { int x; int y; }";
   EXPECT_THAT(getMessage(ID, Input), ::testing::HasSubstr("0 |   int x"));
 }
 TEST(TweakTest, ExtractVariable) {
   llvm::StringLiteral ID = "ExtractVariable";
   checkAvailable(ID, R"cpp(
     int xyz() {
       // return statement
-      return ^1;
+      return [[1]];
     }
     void f() {
-      int a = 5 + [[4 ^* ^xyz^()]];
+      int a = [[5 +]] [[4 * [[[[xyz]]()]]]];
       // multivariable initialization
       if(1)
-        int x = ^1, y = ^a + 1, a = ^1, z = a + 1;
+        int x = [[1]], y = [[a]] + 1, a = [[1]], z = a + 1;
       // if without else
-      if(^1) {}
+      if([[1]])
+        a = [[1]];
       // if with else
-      if(a < ^3)
-        if(a == ^4)
-          a = ^5;
+      if(a < [[3]])
+        if(a == [[4]])
+          a = [[5]];
         else
-          a = ^6;
-      else if (a < ^4)
-        a = ^4;
+          a = [[5]];
+      else if (a < [[4]])
+        a = [[4]];
       else
-        a = ^5;
+        a = [[5]];
       // for loop 
-      for(a = ^1; a > ^3^+^4; a++)
-        a = ^2;
+      for(a = [[1]]; a > [[[[3]] + [[4]]]]; a++)
+        a = [[2]];
       // while 
-      while(a < ^1)
-        ^a++;
+      while(a < [[1]])
+        [[a]]++;
       // do while 
       do
-        a = ^1;
-      while(a < ^3);
+        a = [[1]];
+      while(a < [[3]]);
     }
   )cpp");
   // Should not crash.
   checkNotAvailable(ID, R"cpp(
     template<typename T, typename ...Args>
     struct Test<T, Args...> {
     Test(const T &v) :val(^) {}
       T val;
     };
   )cpp");
   checkNotAvailable(ID, R"cpp(
-    int xyz(int a = ^1) {
+    int xyz(int a = [[1]]) {
       return 1;
       class T {
-        T(int a = ^1) {};
-        int xyz = ^1;
+        T(int a = [[1]]) {};
+        int xyz = [[1]];
       };
     }
     // function default argument
-    void f(int b = ^1) {
+    void f(int b = [[1]]) {
+      // empty selection
+      int a = ^1 ^+ ^2;
       // void expressions
       auto i = new int, j = new int;
-      de^lete i^, del^ete j;
+      [[[[delete i]], delete j]];
       // if
       if(1)
-        int x = 1, y = a + 1, a = 1, z = ^a + 1;
+        int x = 1, y = a + 1, a = 1, z = [[a + 1]];
       if(int a = 1)
-        if(^a == 4)
-          a = ^a ^+ 1;
+        if([[a]] == 4)
+          a = [[[[a]] +]] 1;
       // for loop 
-      for(int a = 1, b = 2, c = 3; ^a > ^b ^+ ^c; ^a++)
-        a = ^a ^+ 1;
+      for(int a = 1, b = 2, c = 3; [[a]] > [[b + c]]; [[a]]++)
+        a = [[a + 1]];
       // lambda 
-      auto lamb = [&^a, &^b](int r = ^1) {return 1;}
+      auto lamb = [&[[a]], &[[b]]](int r = [[1]]) {return 1;}
     }
   )cpp");
   // vector of pairs of input and output strings
   const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
       InputOutputs = {
           // extraction from variable declaration/assignment
           {R"cpp(void varDecl() {
                    int a = 5 * (4 + (3 [[- 1)]]);
const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>>
                            default:
                              break;
                        }
                  })cpp"},*/
           // ensure InsertionPoint isn't inside a macro
           {R"cpp(#define LOOP(x) {int a = x + 1;}
                  void f(int a) {
                    if(1)
-                    LOOP(5 + ^3)
+                    LOOP(5 + [[3]])
                  })cpp",
            R"cpp(#define LOOP(x) {int a = x + 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;
+                    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);
+                   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
+          // 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
       };
   for (const auto &IO : InputOutputs) {
     checkTransform(ID, IO.first, IO.second);
   }
 }
 
 TEST(TweakTest, AnnotateHighlightings) {