[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

98

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?

109

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

98

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

109

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

98

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

109

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

109

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

109

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

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

109

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

109

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

109

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

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101

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

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

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

105

missing ::? → ::describe()

[hokein]

• add USRRenameRule interface.
• fix typos.

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101

Done. Introduced a common interface USRRenameRule.

105

Good Catch!

[ioeric]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101

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.

115

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

101

Yep, I meant that indeed.

[hokein]

Document the qualified rename.

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101

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.

115

Sound good! Done.

[sammccall]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101

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

101

ok, that's fine for now then.

104

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

[hokein]

• remove USRRenameRule
• rebase to master

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101

Done. Reverted it back.

104

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

115

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

131

fter -> after

[hokein]

Fix remaining nits.

include/clang/Tooling/Refactoring/Rename/RenamingAction.h

 
 private:
   const std::vector<std::string> &NewNames, &PrevNames;
   const std::vector<std::vector<std::string>> &USRList;
   std::map<std::string, tooling::Replacements> &FileToReplaces;
   bool PrintLocations;
 };
 
-class RenameOccurrences final : public SourceChangeRefactoringRule {
+/// A refactoring action rule for USR-based rename.
+class USRRenameRule : public SourceChangeRefactoringRule {
+protected:
+  USRRenameRule(const NamedDecl *ND, std::string NewName)
+      : ND(ND), NewName(std::move(NewName)) {
+    assert(ND);
+  }
+
+  // A NamedDecl which indentifies the the symbol being renamed.
+  const NamedDecl *ND;
+  // The new name to change the symbol to.
+  std::string NewName;
+};
+
+class RenameOccurrences final : public USRRenameRule {
 public:
   static Expected<RenameOccurrences> initiate(RefactoringRuleContext &Context,
                                               SourceRange SelectionRange,
                                               std::string NewName);
 
   static const RefactoringDescriptor &describe();
 
 private:
   RenameOccurrences(const NamedDecl *ND, std::string NewName)
-      : ND(ND), NewName(std::move(NewName)) {}
+      : USRRenameRule(ND, std::move(NewName)) {}
 
   Expected<AtomicChanges>
   createSourceReplacements(RefactoringRuleContext &Context) override;
+};
 
-  const NamedDecl *ND;
-  std::string NewName;
+class QualifiedRenameRule final : public USRRenameRule {
+public:
+  static Expected<QualifiedRenameRule> initiate(RefactoringRuleContext &Context,
+                                                std::string OldQualifiedName,
+                                                std::string NewQualifiedName);
+
+  static const RefactoringDescriptor &describe();
+
+private:
+  QualifiedRenameRule(const NamedDecl *ND,
+                      std::string NewQualifiedName)
+      : USRRenameRule(ND, std::move(NewQualifiedName)) {}
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override;
 };
 
 /// Returns source replacements that correspond to the rename of the given
 /// symbol occurrences.
 llvm::Expected<std::vector<AtomicChange>>
 createRenameReplacements(const SymbolOccurrences &Occurrences,
                          const SourceManager &SM, const SymbolName &NewName);
 

lib/Tooling/Refactoring/RefactoringActions.cpp

RefactoringActionRules createActionRules() const override {
     RefactoringActionRules Rules;
     Rules.push_back(createRefactoringActionRule<ExtractFunction>(
         CodeRangeASTSelectionRequirement(),
         OptionRequirement<DeclNameOption>()));
     return Rules;
   }
 };
 
+class OldQualifiedNameOption : public RequiredRefactoringOption<std::string> {
+public:
+  StringRef getName() const override { return "old-qualified-name"; }
+  StringRef getDescription() const override {
+    return "The old qualified name to be renamed";
+  }
+};
+
+class NewQualifiedNameOption : public RequiredRefactoringOption<std::string> {
+public:
+  StringRef getName() const override { return "new-qualified-name"; }
+  StringRef getDescription() const override {
+    return "The new qualified name to change the symbol to";
+  }
+};
+
 class NewNameOption : public RequiredRefactoringOption<std::string> {
 public:
   StringRef getName() const override { return "new-name"; }
   StringRef getDescription() const override {
     return "The new name to change the symbol to";
   }
 };
 
 // FIXME: Rewrite the Actions to avoid duplication of descriptions/names with
 // rules.
 class LocalRename final : public RefactoringAction {
 public:
   StringRef getCommand() const override { return "local-rename"; }
 
   StringRef getDescription() const override {
     return "Finds and renames symbols 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<RenameOccurrences>(
         SourceRangeSelectionRequirement(), OptionRequirement<NewNameOption>()));
+    // FIXME: Use NewNameOption.
+    Rules.push_back(createRefactoringActionRule<QualifiedRenameRule>(
+        OptionRequirement<OldQualifiedNameOption>(),
+        OptionRequirement<NewQualifiedNameOption>()));
     return Rules;
   }
 };
 
 } // end anonymous namespace
 
 std::vector<std::unique_ptr<RefactoringAction>> createRefactoringActions() {
   std::vector<std::unique_ptr<RefactoringAction>> Actions;

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

 #include "clang/Tooling/Refactoring/RefactoringDiagnostic.h"
 #include "clang/Tooling/Refactoring/RefactoringOptions.h"
 #include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "clang/Tooling/Refactoring/Rename/USRFinder.h"
 #include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include <string>
 #include <vector>
 
 using namespace llvm;
 
 namespace clang {
 namespace tooling {
 
RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   if (!Occurrences)
     return Occurrences.takeError();
   // FIXME: Verify that the new name is valid.
   SymbolName Name(NewName);
   return createRenameReplacements(
       *Occurrences, Context.getASTContext().getSourceManager(), Name);
 }
 
+Expected<QualifiedRenameRule>
+QualifiedRenameRule::initiate(RefactoringRuleContext &Context,
+                              std::string OldQualifiedName,
+                              std::string NewQualifiedName) {
+  const NamedDecl *ND =
+      getNamedDeclFor(Context.getASTContext(), OldQualifiedName);
+  if (!ND)
+    return llvm::make_error<llvm::StringError>("Could not find symbol " +
+                                                   OldQualifiedName,
+                                               llvm::errc::invalid_argument);
+  return QualifiedRenameRule(ND, std::move(NewQualifiedName));
+}
+
+const RefactoringDescriptor &QualifiedRenameRule::describe() {
+  static const RefactoringDescriptor Descriptor = {
+      /*Name=*/"local-qualified-rename",
+      /*Title=*/"Qualified Rename",
+      /*Description=*/
+      "Finds and renames qualified symbols in code with no indexer support",
+  };
+  return Descriptor;
+}
+
+Expected<AtomicChanges>
+QualifiedRenameRule::createSourceReplacements(RefactoringRuleContext &Context) {
+  auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
+  assert(!USRs.empty());
+  return tooling::createRenameAtomicChanges(
+      USRs, NewName, Context.getASTContext().getTranslationUnitDecl());
+}
+
 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 =

test/Refactor/LocalRename/QualifiedRename.cpp

+// RUN: clang-refactor local-rename -old-qualified-name="foo::A" -new-qualified-name="bar::B" %s -- -std=c++11 2>&1 | grep -v CHECK | FileCheck %s
+
+namespace foo {
+class A {};
+}
+// CHECK: namespace foo {
+// CHECK-NEXT: class B {};
+// CHECK-NEXT: }
+
+namespace bar {
+void f(foo::A* a) {
+  foo::A b;
+}
+// CHECK: void f(B* a) {
+// CHECK-NEXT:   B b;
+// CHECK-NEXT: }
+}
+
+void f(foo::A* a) {
+  foo::A b;
+}
+// CHECK: void f(bar::B* a) {
+// CHECK-NEXT:   bar::B b;
+// CHECK-NEXT: }

tools/clang-refactor/ClangRefactor.cpp

for (const auto &Rule : this->ActionRules) {
       Rule->visitRefactoringOptions(OptionCreator);
     }
   }
 
   ~RefactoringActionSubcommand() { unregisterSubCommand(); }
 
   const RefactoringActionRules &getActionRules() const { return ActionRules; }
 
-  /// Parses the command-line arguments that are specific to this rule.
+  /// Parses the "-selection" command-line argument.
   ///
   /// \returns true on error, false otherwise.
-  bool parseArguments() {
+  bool parseSelectionArgument() {
     if (Selection) {
       ParsedSelection = SourceSelectionArgument::fromString(*Selection);
       if (!ParsedSelection)
         return true;
     }
     return false;
   }
 
public:
 
   bool invokeAction(RefactoringActionSubcommand &Subcommand,
                     const CompilationDatabase &DB,
                     ArrayRef<std::string> Sources) {
     // Find a set of matching rules.
     SmallVector<RefactoringActionRule *, 4> MatchingRules;
     llvm::StringSet<> MissingOptions;
 
-    bool HasSelection = false;
     for (const auto &Rule : Subcommand.getActionRules()) {
-      bool SelectionMatches = true;
-      if (Rule->hasSelectionRequirement()) {
-        HasSelection = true;
-        if (!Subcommand.getSelection()) {
-          MissingOptions.insert("selection");
-          SelectionMatches = false;
-        }
-      }
       CommandLineRefactoringOptionVisitor Visitor(Subcommand.getOptions());
       Rule->visitRefactoringOptions(Visitor);
-      if (SelectionMatches && Visitor.getMissingRequiredOptions().empty()) {
+      if (Visitor.getMissingRequiredOptions().empty()) {
+        if (!Rule->hasSelectionRequirement()) {
+          MatchingRules.push_back(Rule.get());
+        } else {
+          Subcommand.parseSelectionArgument();
+          if (Subcommand.getSelection()) {
             MatchingRules.push_back(Rule.get());
+          } else {
+            MissingOptions.insert("selection");
+          }
+        }
         continue;
       }
       for (const RefactoringOption *Opt : Visitor.getMissingRequiredOptions())
         MissingOptions.insert(Opt->getName());
     }
     if (MatchingRules.empty()) {
       llvm::errs() << "error: '" << Subcommand.getName()
                    << "' can't be invoked with the given arguments:\n";
       for (const auto &Opt : MissingOptions)
         llvm::errs() << "  missing '-" << Opt.getKey() << "' option\n";
       return true;
     }
+    if (MatchingRules.size() > 1) {
+      llvm::errs() << "error: find multiple matched action rules for the given "
+                      "arguments\n";
+      return true;
+    }
+
+    RefactoringActionRule* MatchingRule = MatchingRules[0];
+    bool HasSelection = MatchingRule->hasSelectionRequirement();
 
     ClangRefactorConsumer Consumer;
     bool HasFailed = false;
     if (foreachTranslationUnit(DB, Sources, [&](ASTContext &AST) {
           RefactoringRuleContext Context(AST.getSourceManager());
           Context.setASTContext(AST);
 
           auto InvokeRule = [&](RefactoringResultConsumer &Consumer) {
             logInvocation(Subcommand, Context);
-            for (RefactoringActionRule *Rule : MatchingRules) {
-              if (!Rule->hasSelectionRequirement())
-                continue;
-              Rule->invoke(Consumer, Context);
-              return;
-            }
-            // FIXME (Alex L): If more than one initiation succeeded, then the
-            // rules are ambiguous.
-            llvm_unreachable(
-                "The action must have at least one selection rule");
+            MatchingRule->invoke(Consumer, Context);
           };
 
           std::unique_ptr<ClangRefactorToolConsumerInterface> CustomConsumer;
           if (HasSelection)
             CustomConsumer = Subcommand.getSelection()->createCustomConsumer();
           ClangRefactorToolConsumerInterface &ActiveConsumer =
               CustomConsumer ? *CustomConsumer : Consumer;
           ActiveConsumer.beginTU(AST);
           if (HasSelection) {
             assert(Subcommand.getSelection() && "Missing selection argument?");
             if (opts::Verbose)
               Subcommand.getSelection()->print(llvm::outs());
             if (Subcommand.getSelection()->forAllRanges(
                     Context.getSources(), [&](SourceRange R) {
                       Context.setSelectionRange(R);
                       InvokeRule(ActiveConsumer);
                     }))
               HasFailed = true;
             ActiveConsumer.endTU();
             return;
           }
-          // FIXME (Alex L): Implement non-selection based invocation path.
+
+          InvokeRule(ActiveConsumer);
           ActiveConsumer.endTU();
         }))
       return true;
     return HasFailed || applySourceChanges(Consumer.getSourceChanges());
   }
 };
 
 } // end anonymous namespace
if (It == Tool.SubCommands.end()) {
     llvm::errs() << "error: no refactoring action given\n";
     llvm::errs() << "note: the following actions are supported:\n";
     for (const auto &Subcommand : Tool.SubCommands)
       llvm::errs().indent(2) << Subcommand->getName() << "\n";
     return 1;
   }
   RefactoringActionSubcommand &ActionCommand = **It;
 
-  if (ActionCommand.parseArguments())
-    return 1;
   if (Tool.invokeAction(ActionCommand, Options.getCompilations(),
                         Options.getSourcePathList()))
     return 1;
 
   return 0;
 }