[PATCH] D72071: [clangd] Add correctness checks for index-based rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 235841.
kbobyrev added a comment.

Improve wording in the comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72071/new/

https://reviews.llvm.org/D72071

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/unittests/RenameTests.cpp


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -795,6 +795,7 @@
 
         void func() {
           [[Foo]] foo;
+          foo.~[[Foo]];
         }
       )cpp",
       },
@@ -868,6 +869,20 @@
         }
       )cpp",
       },
+      {
+          // Macros and implicit references.
+          R"cpp(
+        class [[Fo^o]] {};
+        #define FooFoo Foo
+      )cpp",
+          R"cpp(
+        #include "foo.h"
+        void bar() {
+          [[Foo]] foo;
+          FooFoo z;
+        }
+      )cpp",
+      },
   };
 
   for (const auto& T : Cases) {
@@ -920,7 +935,7 @@
   auto LSPRange = Code.range();
   llvm::StringRef FilePath = "/test/TestTU.cpp";
   llvm::StringRef NewName = "abc";
-  auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+  auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName, "😂");
   ASSERT_TRUE(bool(Edit)) << Edit.takeError();
   ASSERT_EQ(1UL, Edit->Replacements.size());
   EXPECT_EQ(FilePath, Edit->Replacements.begin()->getFilePath());
@@ -928,7 +943,7 @@
 
   // Test invalid range.
   LSPRange.end = {10, 0}; // out of range
-  Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+  Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName, "😂");
   EXPECT_FALSE(Edit);
   EXPECT_THAT(llvm::toString(Edit.takeError()),
               testing::HasSubstr("fail to convert"));
@@ -939,7 +954,7 @@
               [[range]]
       [[range]]
   )cpp");
-  Edit = buildRenameEdit(FilePath, T.code(), T.ranges(), NewName);
+  Edit = buildRenameEdit(FilePath, T.code(), T.ranges(), NewName, "range");
   ASSERT_TRUE(bool(Edit)) << Edit.takeError();
   EXPECT_EQ(applyEdits(FileEdits{{T.code(), std::move(*Edit)}}).front().second,
             expectedResult(T, NewName));
@@ -1013,11 +1028,6 @@
     llvm::StringRef IndexedCode;
     llvm::StringRef LexedCode;
   } Tests[] = {
-    {
-      // no lexed ranges.
-      "[[]]",
-      "",
-    },
     {
       // both line and column are changed, not a near miss.
       R"([[]])",
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -55,7 +55,8 @@
 llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
                                      llvm::StringRef InitialCode,
                                      std::vector<Range> Occurrences,
-                                     llvm::StringRef NewName);
+                                     llvm::StringRef NewName,
+                                     llvm::StringRef OldName);
 
 /// Adjusts indexed occurrences to match the current state of the file.
 ///
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -374,7 +374,8 @@
           llvm::inconvertibleErrorCode());
     }
     auto RenameEdit =
-        buildRenameEdit(FilePath, *AffectedFileCode, *RenameRanges, NewName);
+        buildRenameEdit(FilePath, *AffectedFileCode, *RenameRanges, NewName,
+                        RenameDecl.getNameAsString());
     if (!RenameEdit) {
       return llvm::make_error<llvm::StringError>(
           llvm::formatv("fail to build rename edit for file {0}: {1}", FilePath,
@@ -522,7 +523,8 @@
 llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
                                      llvm::StringRef InitialCode,
                                      std::vector<Range> Occurrences,
-                                     llvm::StringRef NewName) {
+                                     llvm::StringRef NewName,
+                                     llvm::StringRef OldName) {
   assert(std::is_sorted(Occurrences.begin(), Occurrences.end()));
   assert(std::unique(Occurrences.begin(), Occurrences.end()) ==
              Occurrences.end() &&
@@ -557,7 +559,12 @@
     auto EndOffset = Offset(R.end);
     if (!EndOffset)
       return EndOffset.takeError();
-    OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
+    // FIXME: We should not generate invalid replacements in the first place,
+    // but it currently happens for implicit references and in some complicated
+    // cases where the might be incorrectly captured tokens.
+    if (*EndOffset > *StartOffset &&
+        InitialCode.slice(*StartOffset, *EndOffset) == OldName)
+      OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
   }
 
   tooling::Replacements RenameEdit;
@@ -600,10 +607,6 @@
   assert(std::is_sorted(Indexed.begin(), Indexed.end()));
   assert(std::is_sorted(Lexed.begin(), Lexed.end()));
 
-  if (Indexed.size() > Lexed.size()) {
-    vlog("The number of lexed occurrences is less than indexed occurrences");
-    return llvm::None;
-  }
   // Fast check for the special subset case.
   if (std::includes(Indexed.begin(), Indexed.end(), Lexed.begin(), Lexed.end()))
     return Indexed.vec();

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits