This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement Decl canonicalization rules for rename
ClosedPublic

Authored by kbobyrev on Dec 25 2019, 2:12 PM.

Details

Summary

This patch introduces new canonicalization rules which are used for AST-based
rename in Clangd. By comparing two canonical declarations of inspected nodes,
Clangd determines whether both of them belong to the same entity user would
like to rename. Such functionality is relatively concise compared to the
Clang-Rename API that is used right now. It also helps to overcome the
limitations that Clang-Rename originally had and helps to eliminate several
classes of bugs.

Clangd AST-based rename currently relies on Clang-Rename which has design
limitations and also lacks some features. This patch breaks this dependency and
significantly reduces the amount of code to maintain (Clang-Rename is ~2000 LOC,
this patch is just <30 LOC of replacement code).

We eliminate technical debt by simultaneously

  • Maintaining feature parity and ensuring no regressions
  • Opening a straightforward path to improving existing rename bugs
  • Making it possible to add more capabilities to rename feature which would not be possible with Clang-Rename

Diff Detail

Event Timeline

kbobyrev created this revision.Dec 25 2019, 2:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 25 2019, 2:12 PM
kbobyrev planned changes to this revision.Dec 25 2019, 2:21 PM

This is a WIP draft. Few items to address before pushing for a review

  • More tests: renaming is a complex feature and I want to make sure there are no regressions/corner-cases I did not handle. It might make sense to adopt a bunch of tests from Clang-Rename and also add a number of new tests.
  • Detailed documentation: explanation of AST navigation used here, description of limitations (virtual functions "incorrect" handling, notes on performance, etc).
  • Better naming

The logical change after this patch might be changing Clang-Rename to rely on this API and completely removing USR-based approach, because it is no longer maintained and is unlikely to evolve to match the expectations of existing users, but this might require some substantial infrastructure changes and I'm not planning to do that soon.

Unit tests: pass. 61107 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

kbobyrev updated this revision to Diff 303397.Nov 6 2020, 3:09 AM

Continue implementation, integrate more tests from clang-rename.

kbobyrev updated this revision to Diff 303733.Nov 8 2020, 11:55 AM

Store progress.

kbobyrev planned changes to this revision.Nov 8 2020, 11:25 PM
kbobyrev updated this revision to Diff 303940.Nov 9 2020, 11:09 AM

Finish fields canonicalization.

kbobyrev planned changes to this revision.Nov 9 2020, 1:43 PM

Still WIP, moving test changes to a separate chain of patches (starts with D91102) to make review easier and start the pipeline.

nridge added a subscriber: nridge.Nov 9 2020, 1:52 PM
kbobyrev edited the summary of this revision. (Show Details)Nov 10 2020, 1:29 AM
kbobyrev added a reviewer: hokein.
kbobyrev updated this revision to Diff 304463.Nov 11 2020, 3:53 AM

Reset unittests to HEAD.

kbobyrev updated this revision to Diff 304464.Nov 11 2020, 3:54 AM

Actually reset RenameTests.cpp

kbobyrev updated this revision to Diff 304466.Nov 11 2020, 3:55 AM

Rebase on top of master.

kbobyrev planned changes to this revision.Nov 11 2020, 3:55 AM

Still WIP.

kbobyrev updated this revision to Diff 304500.Nov 11 2020, 6:07 AM

I think this is the first version that is ready for a review. Let me know if
you have any questions, @hokein!

kbobyrev updated this revision to Diff 304729.Nov 11 2020, 11:41 PM

Use early return in FieldDecl canonicalization.

don't dig into details, first round of comments.

I think the scope is a bit ambiguous, my reading is that it 1) removes old clang-rename API usage *and* 2) fixes some existing bugs. I would suggest just scoping it on 1).

clang-tools-extra/clangd/refactor/Rename.cpp
251

yeah, this is quite tricky. I'd suggest to defer it to a separate patch.

The behavior you described in https://github.com/clangd/clangd/issues/582 makes sense to me.

Another missing case is about static class members, they are VarDecl in the AST.

283

I'm not sure the motivation of the change, the same as line 244.

299

Looks like we're missing the VarDecl case (looks like a regression). I think we should probably add tests from https://github.com/llvm/llvm-project/commit/1e32df2f91f1aa1f8cd400ce50a621578fa0534e to clangd rename test.

300

we're comparing Decl pointers to check whether they point to the same symbol, I think we should use Candidate->getCanonicalDecl() here.

thinking of a case, in the AST, we have two Decl*s, one points to the forward decl, the other one points to the definition. but they points to the same symbol.

class Foo; // forward decl, this is the canonical decl.

class Foo {
};
307

since we call canonicalRenameDecl in locateDeclAt, I think ND is already canonical, right?

kbobyrev updated this revision to Diff 304776.Nov 12 2020, 4:24 AM
kbobyrev marked 5 inline comments as done.
  • Handle VarDecl
  • Handle FunctionTemplateDecl
  • Remove FieldDecl handling (leave for the future patches)
  • Simplify code
  • Prevent regressions by adding more tests
kbobyrev updated this revision to Diff 304783.Nov 12 2020, 4:41 AM

Add missing user-defined conversion test and a couple of missing cases with
ctor/dtor out-of-line implementation.

kbobyrev updated this revision to Diff 304784.Nov 12 2020, 4:47 AM

Add a test with static class member.

kbobyrev updated this revision to Diff 304785.Nov 12 2020, 4:48 AM

Remove (now) outdated comment.

kbobyrev updated this revision to Diff 304787.Nov 12 2020, 4:53 AM

Completely remove new tests from this patch (to be provided in a different
one).

kbobyrev edited the summary of this revision. (Show Details)Nov 12 2020, 4:59 AM
kbobyrev edited the summary of this revision. (Show Details)
kbobyrev edited the summary of this revision. (Show Details)Nov 12 2020, 5:38 AM
kbobyrev updated this revision to Diff 304893.Nov 12 2020, 10:20 AM

Remove unused header (was added for llvm_unreachable).

didn't finish all parts (looked at the FunctionDecl and RecordDecl), I think we need more documentation/comments in the code to specify the canonical behavior (templates are tricky).

clang-tools-extra/clangd/refactor/Rename.cpp
240

the auto + varies canonicalRenameDecl overrides make the code hard to follow.

since these functions are small, I think we can inline them into the main canonicalRenameDecl(const NamedDecl *D)

247

didn't quite follow the code here, the code looks like we get the FunctionTemplateDecl back via the code path (FunctionTemplateDecl -> FunctionDecl -> FunctionTemplateDecl), and it looks like we're not using the specialized declaration as the canonical decl.

256

want to confirm my understanding:

given the example below:

template<typename T>
class Foo {};

template<>
class Foo<int> {};

the AST is like:

ClassTemplateDecl
  |-CXXRecordDecl (Foo definition) -> (1)
  |-ClassTemplateSpecialization. 

ClassTemplateSpecializationDecl -> call canonicalRenameDecl on it.
  |-Template Argument int
  |-CXXRecordDecl -> (2)

if we pass ClassTemplateSpecializationDecl to this function, this function will return (2)?

262

I think the motivation is for merely renaming the explicit template specialization, but not the primary template?

627

getCanonicalDecl is not needed now.

kbobyrev updated this revision to Diff 306026.Nov 18 2020, 3:05 AM
kbobyrev marked 2 inline comments as done.

Resolve some comments, simplify the code.

clang-tools-extra/clangd/refactor/Rename.cpp
240

I removed autos but I believe merging them into a single function would not be great for two reasons:

  • Some classes are already handled by the same function outside of the main one: ctor/dtor's parent CXXRecordDecl, etc: in the main function we'd have to have a weird logic behind extracting those and this is likely to result in more code
  • Some functions call other ones (e.g. here we have canonicalRenameDecl(Template) - sure, right now it's just D->getTemplatedDecl() but if we decide to change something and when we introduce more code it'd be easy to forget and code duplication is not really a good idea).

WDYT?

247

Uh, good catch, thanks. This one is simply not needed because we handle TemplateDecl which does the right thing for FunctionTemplateDecl.

256

No, this will return (1): getSpecializedTemplate() returns ClassTemplateDecl and then getTemplatedDecl() gets to CXXRecordDecl in it.

262

How come? The specialized template is the primary template, am I missing something?

kbobyrev updated this revision to Diff 306027.Nov 18 2020, 3:06 AM

Remove leftover test.

hokein added inline comments.Nov 18 2020, 12:39 PM
clang-tools-extra/clangd/refactor/Rename.cpp
240

this is more about encapsulation -- my understanding is that canonicalRenameDecl(const NamedDecl) should be the only main entry, others are just implementation details, so we expect client just calls the main function, but not other overrides. The current state leaks these implementation details outside of the world.

Some classes are already handled by the same function outside of the main one: ctor/dtor's parent CXXRecordDecl, etc: in the main function we'd have to have a weird logic behind extracting those and this is likely to result in more code

don't follow this, for ctor/dtor's parent CXXRecordDecl, I think just calling the main canonicalRenameDecl recursively on the parent should be enough?

Some functions call other ones (e.g. here we have canonicalRenameDecl(Template) - sure, right now it's just D->getTemplatedDecl() but if we decide to change something and when we introduce more code it'd be easy to forget and code duplication is not really a good idea).

I agree your points. Given the current code, I think inlining them seems quite straight-forward and clearer to me (the only case is FunctionDecl, which calls the canonicalRenameDecl(const TemplateDecl *D), but we can just call getTemplatedDecl as well).

Regarding your concerns, I think we can always find ways to extend the code when we need to enhance the TemplateDecl case (what kind of enhancement we'll do for templateDecl?)

256

I see. thanks for the explanation. I misunderstood the getSpecializedTemplate.

262

ah, you're right, sorry -- I was confused by the term "specialized template", I thought it is the specialization decl.

I'd suggest using "primary template", I think it is clearer than the specialized decl., and would be nice to have some concrete example in comments explaining these tricky cases.

275

I think we should call getCanonicalDecl before returning the final result (calling it at the beginning doesn't guarantee the final result is canonical).

627

nit: llvm::cast is not needed, just const NamedDecl &RenameDecl = *(*DeclsUnderCursor.begin());

kbobyrev planned changes to this revision.Nov 18 2020, 1:00 PM

Need to address the comments.

kbobyrev updated this revision to Diff 306334.Nov 19 2020, 1:02 AM
kbobyrev marked 8 inline comments as done.

Resolve review comments.

Thanks, this looks nicer now.

clang-tools-extra/clangd/refactor/Rename.cpp
248

ctor & dtor is one of the kinds of CXXMethodDecl, I think we can merge the ctor & dtor cases into the if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {.

258

using else if would be more clear -- as now we modify D is the preceding if, if the D is assigned to a CXXMethodDecl in the preceding if, then it may fall into this branch.

263

oh, it is a tricky case where a method overrides > 1 virtual methods. Looks like we will regress this case in this patch, e.g.

class Foo {
  virtual void foo();
};

class Bar {
  virtual void foo();
};

class C : public Foo, Bar {
  void foo() override; // -> rename foo => bar
};

prior to the patch, both Foo::foo and Bar::foo will be renamed, after this patch, only one of them will be renamed (with within-file rename)? I think it is ok, but probably need some comments here.

266

the same, else

IIUC, CXXMethodDecl must be processed before FunctionDecl, I think we can add an assertion (the FunctionDecl must not be CXXMethodDecl).

kbobyrev updated this revision to Diff 306987.Nov 23 2020, 1:40 AM
kbobyrev marked 4 inline comments as done.

Resolve comments.

kbobyrev updated this revision to Diff 306988.Nov 23 2020, 1:43 AM

Make use of TemplatedDecl.

hokein accepted this revision.Nov 23 2020, 2:27 AM

Thanks, this looks great now.

I think we can get rid of the clangToolingRefactoring dependence from clangd CMake files, clangd now should have no use of any functions there, could you take a look?

This revision is now accepted and ready to land.Nov 23 2020, 2:27 AM
hokein added inline comments.Nov 23 2020, 2:28 AM
clang-tools-extra/clangd/refactor/Rename.cpp
78

nit: maybe we can just move the function definition to here.

kbobyrev updated this revision to Diff 307013.Nov 23 2020, 2:41 AM
kbobyrev marked an inline comment as done.

Get rid of forward declaration.

kbobyrev edited the summary of this revision. (Show Details)Nov 23 2020, 2:42 AM
This revision was landed with ongoing or failed builds.Nov 23 2020, 2:43 AM
This revision was automatically updated to reflect the committed changes.