[clangd] Fix rename for explicit destructor calls

[kbobyrev]

Summary

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

[kbobyrev]

I'm not sure if leaving both ReferenceLocs pointing to the same location is a sensible solution, but merging them seems quite complicated and probably not really worth the effort. I've considered multiple alternative solutions, as described in https://github.com/clangd/clangd/issues/236 and converged to the one presented in this patch.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

[kbobyrev]

Changed reviewer to @kadircet because Sam would be out until the end of the week.

[kadircet]

clang-tools-extra/clangd/FindTarget.cpp

638

instead of patching the source location for destructors. we should probably not report anything for them in here, as they will be reported correctly as typelocs.

So you can check for E->getMemberNameInfo().getNamedTypoInfo() and bail out if its non-null, which means this is a constructor/destructor/operator with a typeloc that will be visited separately and result in the same ref. That way we would also get rid of duplicate refs being reported by findExplicitReferences, API doesn't mention anything about those but most of the callers would fail in the presence of duplicates, therefore I think we shouldn't be reporting duplicates. (for example the main visitor actually makes sure typelocs are not visited twice).

also could you add unittests for constructor and operator cases as well to make sure we get exactly one reference for those as well.

[kbobyrev]

Going to investigate few interesting cases of findExplicitReferences not working as intended and opting out for a clean approach that prevents duplicates from getting into the reference set.

clang-tools-extra/clangd/FindTarget.cpp

638

instead of patching the source location for destructors. we should probably not report anything for them in here, as they will be reported correctly as typelocs.

Yes, I thought about that, but I decided against it because IIUC this would prevent destructors/ctors from appearing in x-refs which might be a regression. Also, this might be incorrect for cases like this one:

class Bar {};

class Foo {
public:
  operator Bar() const { return Bar(); }
};

int main() {
  Foo foo;
  Bar bar = foo.operator Bar();
}

Here, if we bail out on foo.operator Bar() we would miss the reference to the operator Bar() which we might want to preserve.

Actually, looking at the FindTargets unittests, there is something interesting happening: references to constructor have the constructor as the target, but destructor references target typename itself.

Another thing that is happening is that the following code has two references of bar at the same location:

class Foo {
public:
  operator $0^$1^Bar() const { return Bar(); }
};

I'm on it right now.

also could you add unittests for constructor and operator cases as well to make sure we get exactly one reference for those as well.

Sure!

[kbobyrev]

Avoid duplicate references by filtering out destructor calls

[kbobyrev]

The patch should probably be correct now.

I've submitted the issue I encountered while trying to add more tests for this patch since it seems to be a separate issue (hopefully, I'm doing everything correctly while setting up that test): https://github.com/clangd/clangd/issues/254.

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

900

$12 would not be captured if we simply do

if (E->getMemberNameInfo().getNamedTypeInfo())
  return;

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

[hokein]

clang-tools-extra/clangd/FindTarget.cpp

619

I think we should use E->getFoundDecl() here, could you add a test case for calling base destructor explicitly, e.g. derived->base::~base()?

638

+1 on not reporting these locations (we also don't report destructor decls in refInDecl).

because IIUC this would prevent destructors/ctors from appearing in x-refs which might be a regression.

we don't use findExplicitReferences in XRefs, so it should be ok.

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

882

could we simplify the newly-added test? I think a test only for explicit destructor calls is sufficient, I think.

void foo () {
  class Foo { ~Foo(); };
 Foo f;
 f.~Foo();
}

900

If we want to keep the test for operator int, just create a new test.

[kbobyrev]

Address comments

[kbobyrev]

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

882

Simplified the test a bit. I don't think it hurts to have more coverage though (if this is not duplication).

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

[hokein]

looks good, thanks.

[kadircet]

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

887

could you rather have a conversion operator to a tag (like struct Bar), with a fixme of course mentionining the issue you have in clangd bug tracker(not a link but rather a short description)

905

this actually looks troublesome, since we are not returning the functiondecl anymore(also for references to it as well), even though it is explicitly spelled in the source code.
I suppose it is OK to keep that behavior for now, as there are no callers caring about those typednames yet.

just thinking out loud:
In case we start to have such clients, it might be more principled to report $1~$2Foo in here(I know it is the old behavior :/ ) and handle those differently in the callers' with
custom needs.

[kbobyrev]

Modify the test to address the comments.

[kbobyrev]

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

905

As discussed, I agree with this point and it's kind of unfortunate behaviour but we already have that for destructor declaration, too (see above FIXME) and in several other cases. Might want to fix that in the future though!

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

clang-tools-extra/clangd/FindTarget.cpp

struct Visitor : ConstStmtVisitor<Visitor> {
     void VisitDeclRefExpr(const DeclRefExpr *E) {
       Refs.push_back(ReferenceLoc{E->getQualifierLoc(),
                                   E->getNameInfo().getLoc(),
                                   /*IsDecl=*/false,
                                   {E->getFoundDecl()}});
     }
 
     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,
                                   {E->getFoundDecl()}});
     }
 
     void VisitOverloadExpr(const OverloadExpr *E) {
       Refs.push_back(ReferenceLoc{E->getQualifierLoc(),
                                   E->getNameInfo().getLoc(),
                                   /*IsDecl=*/false,
                                   llvm::SmallVector<const NamedDecl *, 1>(
                                       E->decls().begin(), E->decls().end())});
     }
 
     void VisitSizeOfPackExpr(const SizeOfPackExpr *E) {
       Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   E->getPackLoc(),
                                   /*IsDecl=*/false,
                                   {E->getPack()}});
     }
   };
 
   Visitor V;
   V.Visit(E);
   return V.Refs;
 }

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

std::pair</*Code*/ llvm::StringRef, /*References*/ llvm::StringRef> Cases[] =
         "3: targets = {foo()::Foo::field}, decl\n"
         "4: targets = {Var}, decl\n"
         "5: targets = {E}, decl\n"
         "6: targets = {foo()::ABC}, decl\n"
         "7: targets = {INT}, decl\n"
         "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 {};
              void foo() {
                // member initializer
                class $0^X {
                  int $1^abc;
                  $2^X(): $3^abc() {}

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

llvm::StringRef Tests[] = {
           const Baz &BazReference = foo;
           const Baz *BazPointer = &foo;
           reinterpret_cast<const [[^Foo]] *>(BazPointer)->getValue();
           static_cast<const [[^Foo]] &>(BazReference).getValue();
           static_cast<const [[^Foo]] *>(BazPointer)->getValue();
         }
       )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 {};
         class Qux {
           Baz [[F^oo]];
         public:
           Qux();
         };