[clang-tools-extra] 38bdb94 - [clangd] Fix rename for explicit destructor calls

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2020-01-21T05:33:39+01:00
New Revision: 38bdb94120b76f8f79cd27d721892673e573895a

URL: https://github.com/llvm/llvm-project/commit/38bdb94120b76f8f79cd27d721892673e573895a
DIFF: https://github.com/llvm/llvm-project/commit/38bdb94120b76f8f79cd27d721892673e573895a.diff

LOG: [clangd] Fix rename for explicit destructor calls

When triggering rename of the class name in the code with explicit destructor
calls, rename fails. Consider the following piece of code:

```
class Foo;

...

Foo f;
f.~/*...*/Foo();
```

`findExplicitReferences` will report two `ReferenceLoc` for destructor call:
one is comming from `MemberExpr` (i.e. destructor call itself) and would point
to the tilde:

```
f.~/*...*/Foo();
  ^
```

And the second one is pointing to the typename and is coming from `TypeLoc`.

```
f.~/*...*/Foo();
          ^
```

This causes rename to produce incorrect textual replacements. This patch
updates `MemberExpr` handler to detect destructor calls and prevents it
from reporting a duplicate reference.

Resolves: https://github.com/clangd/clangd/issues/236

Reviewers: kadircet, hokein

Differential Revision: https://reviews.llvm.org/D72638

Added: 
    

Modified: 
    clang-tools-extra/clangd/FindTarget.cpp
    clang-tools-extra/clangd/unittests/FindTargetTests.cpp
    clang-tools-extra/clangd/unittests/RenameTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp
index 2bab9d4f67b7..0e3c30e16dd5 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -614,6 +614,10 @@ llvm::SmallVector<ReferenceLoc, 2> refInExpr(const Expr *E) {
     }
 
     void VisitMemberExpr(const MemberExpr *E) {
+      // Skip destructor calls to avoid duplication: TypeLoc within will be
+      // visited separately.
+      if (llvm::dyn_cast<CXXDestructorDecl>(E->getFoundDecl().getDecl()))
+        return;
       Refs.push_back(ReferenceLoc{E->getQualifierLoc(),
                                   E->getMemberNameInfo().getLoc(),
                                   /*IsDecl=*/false,

diff  --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 408ebe24e773..a420348fcda8 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -879,6 +879,56 @@ TEST_F(FindExplicitReferencesTest, All) {
         "8: targets = {INT2}, decl\n"
         "9: targets = {NS}, decl\n"
         "10: targets = {ns}\n"},
+       // User-defined conversion operator.
+       {R"cpp(
+            void foo() {
+               class $0^Bar {};
+               class $1^Foo {
+               public:
+                 // FIXME: This should have only one reference to Bar.
+                 $2^operator $3^$4^Bar();
+               };
+
+               $5^Foo $6^f;
+               $7^f.$8^operator $9^Bar();
+            }
+        )cpp",
+        "0: targets = {Bar}, decl\n"
+        "1: targets = {Foo}, decl\n"
+        "2: targets = {foo()::Foo::operator Bar}, decl\n"
+        "3: targets = {Bar}\n"
+        "4: targets = {Bar}\n"
+        "5: targets = {Foo}\n"
+        "6: targets = {f}, decl\n"
+        "7: targets = {f}\n"
+        "8: targets = {foo()::Foo::operator Bar}\n"
+        "9: targets = {Bar}\n"},
+       // Destructor.
+       {R"cpp(
+             void foo() {
+               class $0^Foo {
+               public:
+                 ~$1^Foo() {}
+
+                 void $2^destructMe() {
+                   this->~$3^Foo();
+                 }
+               };
+
+               $4^Foo $5^f;
+               $6^f.~ /*...*/ $7^Foo();
+             }
+           )cpp",
+        "0: targets = {Foo}, decl\n"
+        // FIXME: It's better to target destructor's FunctionDecl instead of
+        // the type itself (similar to constructor).
+        "1: targets = {Foo}\n"
+        "2: targets = {foo()::Foo::destructMe}, decl\n"
+        "3: targets = {Foo}\n"
+        "4: targets = {Foo}\n"
+        "5: targets = {f}, decl\n"
+        "6: targets = {f}\n"
+        "7: targets = {Foo}\n"},
        // cxx constructor initializer.
        {R"cpp(
              class Base {};

diff  --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 80930b9096d4..d0fc4d1033b5 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -265,6 +265,33 @@ TEST(RenameTest, WithinFileRename) {
         }
       )cpp",
 
+      // Destructor explicit call.
+      R"cpp(
+        class [[F^oo]] {
+        public:
+          ~[[^Foo]]();
+        };
+
+        [[Foo^]]::~[[^Foo]]() {}
+
+        int main() {
+          [[Fo^o]] f;
+          f.~/*something*/[[^Foo]]();
+          f.~[[^Foo]]();
+        }
+      )cpp",
+
+      // Derived destructor explicit call.
+      R"cpp(
+        class [[Bas^e]] {};
+        class Derived : public [[Bas^e]] {}
+
+        int main() {
+          [[Bas^e]] *foo = new Derived();
+          foo->[[^Base]]::~[[^Base]]();
+        }
+      )cpp",
+
       // CXXConstructor initializer list.
       R"cpp(
         class Baz {};