[clang-refactor] Introduce a new rename rule for qualified symbols

[hokein]

Summary

Prototype of a new rename rule for renaming qualified symbol.

[hokein]

Add test.

[sammccall]

Implementation LG.

My questions are mostly framework-level:

• should this be an Action, or just a Rule under the existing rename Action?
• {qname vs selection} x {local vs global} x ... seems like a combinatorial explosion in the number of rules. Shouldn't concerns like format of the input be taken care of before we get to the Rule?

@arphaman

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

141

if this is a local refactor, and there are no USRs (symbol doesn't exist), is this an error that needs to be signaled somehow?

152

As discussed offline, it's not clear why this is a separate Action, rather than a different Rule that's part of the same Action.

@arphaman how does the framework answer this question?

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

141

This case should not be happened IMO. If we find the ND, we will rename ND at least.

152

There is a document describing it, but still ambiguous.

We also had some questions about local-rename from the discussion, need @arphaman's input:

• OccurrenceFinder is not exposed now, it is merely used in RenameOccurrences. We think there should be a public interface to the clients, like for implementing interactive mode in IDE?
• Currently the rules defined in the same action must have mutual command-line options, otherwise clang-refactor would complain the command-line option are being registered more than once. It might be very strict for some cases. For example, -new-name is most likely being used by many rules in local-rename action.

[sammccall]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

141

Oops, of course. Maybe an assert?

[hokein]

Add an assert for non-empty USRs.

[arphaman]

I've committed https://reviews.llvm.org/D38985, so you'd have to rebase unfortunately. Things are still somewhat unstable :)

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

152

I think that this should be just a rule in local-rename.

So you'd be able to call:

clang-refactor local-rename -selection=X -new-name=foo
clang-refactor local-rename -old-qualified-name=bar -new-name=foo.

[ioeric]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

152

We need your help to understand how exactly local-rename is intended to be used.

From the current code and previous conversations we had, it seems to me that the action would support the use case where a user selects an identifier in the editor (say, with cursor) and initiates a local-rename action but without providing the new name in the beginning. The rename rule finds and returns all occurrences (including token ranges) to the editor, and users can then start typing in the new name, and in the same time, the editor performs text replacements according to ranges of occurrences and the new name typed in. Is this how RenameOccurrences is intended to be used in the future?

If this is how local-rename is expected to be used, it would be hard to merge qualified rename into it, because both qualified old name and new name are required in order to calculate the range of a symbol reference, and this doesn't fit with the above workflow. But if my understanding is simply wrong (e.g. the editor would invoke local-rename again to perform the actual refactoring), then I think it makes a lot of sense to merge qualified rename into the current local-rename action.

[ioeric]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

152

Sorry, by "your help", I was referring to Alex ;) @arphaman

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

152

You're right that rename should deal with occurrences conceptually, but I believe that's more of requirement imposed onto the editor clients. Rename in particular is basically impossible to map to all clients using just one generic model, so I think it's fine if RenameOccurrences class returns source replacements that local-rename in clang-refactor consumes. I don't think this will change in the future, if anything we will lift OccurrenceFinderclass into the header so that the editor clients can use it.
So I think in terms of the tool it should be ok to have immediate local-rename action that behaves similarly to clang-rename and deals with source changes and not replacements.

[ioeric]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

152

Thanks for the clarification! In that case, I agree that qualified rename should be a rule in the local-rename action.

[hokein]

• rebase to master
• move the implementation to a refactoring rule in local-rename
• support non-selection based refactoring action invocation call path

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

152

Thanks for the explanation, that makes sense. Moved to local-rename action.

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

It might be better to find the declaration (and report error if needed) during in initiation, and then pass the ND to the class. Maybe then both RenameOccurrences and QualifiedRenameRule could subclass from one base class that actually does just this:

auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
  assert(!USRs.empty());
  return tooling::createRenameAtomicChanges(
      USRs, NewQualifiedName, Context.getASTContext().getTranslationUnitDecl());

[jklaehn]

I spotted two typos. :) Also, the commit message needs to be updated.

lib/Tooling/Refactoring/RefactoringActions.cpp

61 ↗

(On Diff #120792)

There is a typo here → "qualified name to change"

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

183

missing ::? → ::describe()

[hokein]

• add USRRenameRule interface.
• fix typos.

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

Done. Introduced a common interface USRRenameRule.

183

Good Catch!

[ioeric]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

USRRenameRule doesn't seem to be a very useful abstraction. I think what Alex proposed is a base class that implements createSourceReplacements which could be shared by both RenameOccurrences and QualifiedRenameRule. Intuitively, un-qualified rename is a special case of qualified rename (maybe?), where namespace is not changed.

193

We should also document the behavior when namespace is changed. What would happen to symbol definitions and symbol references?

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

Yep, I meant that indeed.

[hokein]

Document the qualified rename.

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

Ah, sorry for the misunderstanding.

Unfortunately, RenameOccurrences and QualifiedRenameRule could not share the same createSourceReplacements implementation,

• Their supported use cases are different. QualifiedRenameRule only supports renaming class, function, enums, alias and class members, which doesn't cover all the cases of RenameOccurrences (like renaming local variable in function body).

• we have separated implementations in the code base(createRenameAtomicChanges for qualified rename, createRenameReplacements for un-qualified rename). The implementation of qualified rename does more things, including calculating the shortest prefix qualifiers, getting correct range of replacement.

193

Sound good! Done.

[sammccall]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

That makes sense (I think), but then we should remove USRRenameRule again if we're not actually reusing anything.

(And ideally we can hide any such future reuse as functions in the cc file, rather than a class hierarchy exposed in the header)

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

ok, that's fine for now then.

182

You can use a refactoring diagnostic here (see RenameOccurrences::initiate).

[hokein]

• remove USRRenameRule
• rebase to master

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

179

Done. Reverted it back.

182

I think using StringError is sufficient here -- didn't see much value using diagnostic:

1. we need to add a new diagnostic type clang_refactor_no_symbol.
2. we can't include the detailed information (OldQualifiedName) in the diagnostic, and we don't have SourceLocation in the code for the diagnostic.

[arphaman]

lgtm

[sammccall]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

193

"with no indexer support" isn't meaningful to users I think - "within a translation unit" or "within a source file"?

209

fter -> after

[hokein]

Fix remaining nits.

include/clang/Tooling/Refactoring/RefactoringActionRegistry.def

 #ifndef REFACTORING_ACTION
 #define REFACTORING_ACTION(Name)
 #endif
 
 REFACTORING_ACTION(LocalRename)
+REFACTORING_ACTION(LocalQualifiedRename)
 REFACTORING_ACTION(Extract)
 
 #undef REFACTORING_ACTION

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

public:
   RefactoringActionRules createActionRules() const override {
     RefactoringActionRules Rules;
     Rules.push_back(createRefactoringActionRule<RenameOccurrences>(
         SymbolSelectionRequirement(), OptionRequirement<NewNameOption>()));
     return Rules;
   }
 };
 
+class OldNameOption : public RequiredRefactoringOption<std::string> {
+ public:
+  StringRef getName() const override { return "old-name"; }
+  StringRef getDescription() const override {
+    return "The old qualified name to be renamed";
+  }
+};
+
+class QualifiedRenameOccurrences : public SourceChangeRefactoringRule {
+public:
+  QualifiedRenameOccurrences(std::string OldName, std::string NewName)
+      : OldName(OldName), NewName(NewName) {}
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override {
+    const NamedDecl* ND = getNamedDeclFor(Context.getASTContext(), OldName);
+    if (!ND)
+      return Context.createDiagnosticError(
+          SourceLocation(), diag::err_refactor_selection_no_symbol);
+
+    auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
+    return tooling::createRenameAtomicChanges(
+        USRs, NewName, Context.getASTContext().getTranslationUnitDecl());
+  }
+
+private:
+  std::string OldName;
+  std::string NewName;
+};
+
+class LocalQualifiedRename final : public RefactoringAction {
+public:
+  StringRef getCommand() const override {
+    return "local-qualified-rename";
+  }
+
+  StringRef getDescription() const override {
+    return "Finds and rename qualified symbol in code with no indexer support";
+  }
+
+  /// Returns a set of refactoring actions rules that are defined by this
+  /// action.
+  RefactoringActionRules createActionRules() const override {
+    RefactoringActionRules Rules;
+    Rules.push_back(createRefactoringActionRule<QualifiedRenameOccurrences>(
+        OptionRequirement<OldNameOption>(),
+        OptionRequirement<NewNameOption>()));
+    return Rules;
+  }
+};
+
+
 } // end anonymous namespace
 
 std::unique_ptr<RefactoringAction> createLocalRenameAction() {
   return llvm::make_unique<LocalRename>();
 }
 
+std::unique_ptr<RefactoringAction> createLocalQualifiedRenameAction() {
+  return llvm::make_unique<LocalQualifiedRename>();
+}
+
 Expected<std::vector<AtomicChange>>
 createRenameReplacements(const SymbolOccurrences &Occurrences,
                          const SourceManager &SM, const SymbolName &NewName) {
   // FIXME: A true local rename can use just one AtomicChange.
   std::vector<AtomicChange> Changes;
   for (const auto &Occurrence : Occurrences) {
     ArrayRef<SourceRange> Ranges = Occurrence.getNameRanges();
     assert(NewName.getNamePieces().size() == Ranges.size() &&
            "Mismatching number of ranges and name pieces");
     AtomicChange Change(SM, Ranges[0].getBegin());
     for (const auto &Range : llvm::enumerate(Ranges)) {
       auto Error =
           Change.replace(SM, CharSourceRange::getCharRange(Range.value()),
                          NewName.getNamePieces()[Range.index()]);
       if (Error)
         return std::move(Error);
     }
     Changes.push_back(std::move(Change));
   }
   return std::move(Changes);
 }
 
 /// Takes each atomic change and inserts its replacements into the set of
 /// replacements that belong to the appropriate file.
 static void convertChangesToFileReplacements(
     ArrayRef<AtomicChange> AtomicChanges,
     std::map<std::string, tooling::Replacements> *FileToReplaces) {
   for (const auto &AtomicChange : AtomicChanges) {
     for (const auto &Replace : AtomicChange.getReplacements()) {
       llvm::Error Err = (*FileToReplaces)[Replace.getFilePath()].add(Replace);
       if (Err) {
         llvm::errs() << "Renaming failed in " << Replace.getFilePath() << "! "
                      << llvm::toString(std::move(Err)) << "\n";
       }
     }
   }
 }
 
 class RenamingASTConsumer : public ASTConsumer {