Page MenuHomePhabricator

[clangd] Fix rename for explicit destructor calls
ClosedPublic

Authored by kbobyrev on Jan 13 2020, 12:09 PM.

Details

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

Diff Detail

Event Timeline

kbobyrev created this revision.Jan 13 2020, 12:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 12:10 PM
kbobyrev edited the summary of this revision. (Show Details)Jan 13 2020, 12:11 PM

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.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

kbobyrev edited reviewers, added: kadircet; removed: sammccall.Jan 14 2020, 6:26 PM
kbobyrev added a subscriber: sammccall.

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

kadircet added inline comments.Jan 15 2020, 5:14 AM
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 planned changes to this revision.Jan 16 2020, 9:31 AM

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 updated this revision to Diff 238541.Jan 16 2020, 10:18 AM

Avoid duplicate references by filtering out destructor calls

kbobyrev marked 3 inline comments as done.Jan 16 2020, 10:31 AM

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;

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

kbobyrev edited the summary of this revision. (Show Details)Jan 16 2020, 10:54 AM
hokein added a subscriber: hokein.Jan 17 2020, 5:02 AM
hokein added inline comments.
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 updated this revision to Diff 238791.Jan 17 2020, 8:50 AM
kbobyrev marked 5 inline comments as done.

Address comments

kbobyrev added inline comments.Jan 17 2020, 8:52 AM
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).

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

hokein accepted this revision.Jan 20 2020, 3:44 AM

looks good, thanks.

This revision is now accepted and ready to land.Jan 20 2020, 3:44 AM
kadircet added inline comments.Jan 20 2020, 3:54 AM
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 updated this revision to Diff 239182.Jan 20 2020, 11:32 AM
kbobyrev marked 2 inline comments as done.

Modify the test to address the comments.

kbobyrev added inline comments.Jan 20 2020, 11:34 AM
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!

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

This revision was automatically updated to reflect the committed changes.