This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix non-idempotent cases of canonicalRenameDecl()
ClosedPublic

Authored by sammccall on Sep 7 2022, 4:58 AM.
Tokens
"Like" token, awarded by tom-anders.

Details

Summary

The simplest way to ensure full canonicalization is to canonicalize
recursively in most cases.

This fixes an assertion failure and presumably correctness bugs.

It does show up that D132797's index-based virtual method renames doesn't handle
templates well (the AST behavior is different and IMO better).
We could choose to disable in this case or change the index behavior,
but this patch doesn't do either.

Diff Detail

Event Timeline

sammccall created this revision.Sep 7 2022, 4:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 4:58 AM
sammccall requested review of this revision.Sep 7 2022, 4:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 4:58 AM
ilya-biryukov accepted this revision.Sep 9 2022, 4:53 AM

LGTM to unbreak clangd. Agree that a more thorough look at this is needed.
Maybe add a bug to track this?

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

Quick question to help better understand our current behavior. Not requesting to change anything here, just wanted to make sure what we're doing now.

If we run the rename inside the primary template itself, are we going to rename the use in Bar?
I suspect the answer is "yes" because it's in the same file, so we get it from the AST and not from the index. Just to make sure.

This revision is now accepted and ready to land.Sep 9 2022, 4:53 AM
sammccall marked an inline comment as done.Oct 5 2022, 12:20 PM
sammccall added inline comments.
clang-tools-extra/clangd/unittests/RenameTests.cpp
1521

Yes, I just checked - this example works in both directions. And indeed it's because the index isn't involved.

Filed https://github.com/clangd/clangd/issues/1325 for the index issue.

This revision was landed with ongoing or failed builds.Oct 5 2022, 12:23 PM
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.