[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

143 ↗

(On Diff #120555)

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?

154 ↗

(On Diff #120555)

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

143 ↗

(On Diff #120555)

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

154 ↗

(On Diff #120555)

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

143 ↗

(On Diff #120555)

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

154 ↗

(On Diff #120555)

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

154 ↗

(On Diff #120555)

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

154 ↗

(On Diff #120555)

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

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

154 ↗

(On Diff #120555)

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

154 ↗

(On Diff #120555)

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

154 ↗

(On Diff #120555)

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

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101 ↗

(On Diff #120792)

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

105 ↗

(On Diff #120792)

missing ::? → ::describe()

[hokein]

• add USRRenameRule interface.
• fix typos.

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101 ↗

(On Diff #120792)

Done. Introduced a common interface USRRenameRule.

105 ↗

(On Diff #120792)

Good Catch!

[ioeric]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

115 ↗

(On Diff #121266)

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

101 ↗

(On Diff #120792)

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.

[arphaman]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101 ↗

(On Diff #120792)

Yep, I meant that indeed.

[hokein]

Document the qualified rename.

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

115 ↗

(On Diff #121266)

Sound good! Done.

101 ↗

(On Diff #120792)

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.

[sammccall]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

101 ↗

(On Diff #120792)

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

104 ↗

(On Diff #121456)

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

101 ↗

(On Diff #120792)

ok, that's fine for now then.

[hokein]

• remove USRRenameRule
• rebase to master

[hokein]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

104 ↗

(On Diff #121456)

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.

101 ↗

(On Diff #120792)

Done. Reverted it back.

[arphaman]

lgtm

[sammccall]

lib/Tooling/Refactoring/Rename/RenamingAction.cpp

116 ↗

(On Diff #121941)

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

132 ↗

(On Diff #121941)

fter -> after

[hokein]

Fix remaining nits.

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

private:
 
   Expected<AtomicChanges>
   createSourceReplacements(RefactoringRuleContext &Context) override;
 
   const NamedDecl *ND;
   std::string NewName;
 };
 
+class QualifiedRenameRule final : public SourceChangeRefactoringRule {
+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)
+      : ND(ND), NewQualifiedName(std::move(NewQualifiedName)) {}
+
+  Expected<AtomicChanges>
+  createSourceReplacements(RefactoringRuleContext &Context) override;
+
+  // A NamedDecl which indentifies the the symbol being renamed.
+  const NamedDecl *ND;
+  // The new qualified name to change the symbol to.
+  std::string NewQualifiedName;
+};
+
 /// 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);
 
 /// Rename all symbols identified by the given USRs.
 class QualifiedRenamingAction {

cfe/trunk/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;

cfe/trunk/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=*/
+      R"(Finds and renames qualified symbols in code within a translation unit.
+It is used to move/rename a symbol to a new namespace/name:
+  * Supported symbols: classes, class members, functions, enums, and type alias.
+  * Renames all symbol occurrences from the old qualified name to the new
+    qualified name. All symbol references will be correctly qualified; For
+    symbol definitions, only name will be changed.
+For example, rename "A::Foo" to "B::Bar":
+  Old code:
+    namespace foo {
+    class A {};
+    }
+
+    namespace bar {
+    void f(foo::A a) {}
+    }
+
+  New code after rename:
+    namespace foo {
+    class B {};
+    }
+
+    namespace bar {
+    void f(B b) {}
+    })"
+  };
+  return Descriptor;
+}
+
+Expected<AtomicChanges>
+QualifiedRenameRule::createSourceReplacements(RefactoringRuleContext &Context) {
+  auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
+  assert(!USRs.empty());
+  return tooling::createRenameAtomicChanges(
+      USRs, NewQualifiedName, 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() &&

cfe/trunk/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: }

cfe/trunk/tools/clang-refactor/ClangRefactor.cpp

 
 /// A subcommand that corresponds to individual refactoring action.
 class RefactoringActionSubcommand : public cl::SubCommand {
 public:
   RefactoringActionSubcommand(std::unique_ptr<RefactoringAction> Action,
                               RefactoringActionRules ActionRules,
                               cl::OptionCategory &Category)
       : SubCommand(Action->getCommand(), Action->getDescription()),
-        Action(std::move(Action)), ActionRules(std::move(ActionRules)),
-        HasSelection(false) {
+        Action(std::move(Action)), ActionRules(std::move(ActionRules)) {
     // Check if the selection option is supported.
     for (const auto &Rule : this->ActionRules) {
-      if ((HasSelection = Rule->hasSelectionRequirement()))
-        break;
-    }
-    if (HasSelection) {
+      if (Rule->hasSelectionRequirement()) {
         Selection = llvm::make_unique<cl::opt<std::string>>(
             "selection",
-          cl::desc("The selected source range in which the refactoring should "
-                   "be initiated (<file>:<line>:<column>-<line>:<column> or "
-                   "<file>:<line>:<column>)"),
+            cl::desc(
+                "The selected source range in which the refactoring should "
             cl::cat(Category), cl::sub(*this));
+        break;
+      }
     }
     // Create the refactoring options.
     for (const auto &Rule : this->ActionRules) {
       CommandLineRefactoringOptionCreator OptionCreator(Category, *this,
                                                         Options);
       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;
   }
 
-  // Whether the selection is supported by any rule in the subcommand.
-  bool hasSelection() const { return HasSelection; }
-
   SourceSelectionArgument *getSelection() const {
     assert(Selection && "selection not supported!");
     return ParsedSelection.get();
   }
 
   const RefactoringActionCommandLineOptions &getOptions() const {
     return Options;
   }
 
 private:
   std::unique_ptr<RefactoringAction> Action;
   RefactoringActionRules ActionRules;
   std::unique_ptr<cl::opt<std::string>> Selection;
   std::unique_ptr<SourceSelectionArgument> ParsedSelection;
   RefactoringActionCommandLineOptions Options;
-  // Whether the selection is supported by any rule in the subcommand.
-  bool HasSelection;
 };
 
 class ClangRefactorConsumer final : public ClangRefactorToolConsumerInterface {
 public:
   ClangRefactorConsumer(AtomicChanges &Changes) : SourceChanges(&Changes) {}
 
   void handleError(llvm::Error Err) override {
     Optional<PartialDiagnosticAt> Diag = DiagnosticError::take(Err);
public:
   void callback(ASTContext &AST) {
     assert(SelectedSubcommand && MatchingRule && Consumer);
     RefactoringRuleContext Context(AST.getSourceManager());
     Context.setASTContext(AST);
 
     // If the selection option is test specific, we use a test-specific
     // consumer.
     std::unique_ptr<ClangRefactorToolConsumerInterface> TestConsumer;
-    if (SelectedSubcommand->hasSelection())
+    bool HasSelection = MatchingRule->hasSelectionRequirement();
+    if (HasSelection)
       TestConsumer = SelectedSubcommand->getSelection()->createCustomConsumer();
     ClangRefactorToolConsumerInterface *ActiveConsumer =
         TestConsumer ? TestConsumer.get() : Consumer.get();
     ActiveConsumer->beginTU(AST);
-    // FIXME (Alex L): Implement non-selection based invocation path.
-    if (SelectedSubcommand->hasSelection()) {
+
+    auto InvokeRule = [&](RefactoringResultConsumer &Consumer) {
+      if (opts::Verbose)
+        logInvocation(*SelectedSubcommand, Context);
+      MatchingRule->invoke(*ActiveConsumer, Context);
+    };
+    if (HasSelection) {
       assert(SelectedSubcommand->getSelection() &&
              "Missing selection argument?");
       if (opts::Verbose)
         SelectedSubcommand->getSelection()->print(llvm::outs());
       if (SelectedSubcommand->getSelection()->forAllRanges(
               Context.getSources(), [&](SourceRange R) {
                 Context.setSelectionRange(R);
-                if (opts::Verbose)
-                  logInvocation(*SelectedSubcommand, Context);
-                MatchingRule->invoke(*ActiveConsumer, Context);
+                InvokeRule(*ActiveConsumer);
               }))
         HasFailed = true;
       ActiveConsumer->endTU();
       return;
     }
+    InvokeRule(*ActiveConsumer);
     ActiveConsumer->endTU();
   }
 
   llvm::Expected<std::unique_ptr<FrontendActionFactory>>
   getFrontendActionFactory() {
     class ToolASTConsumer : public ASTConsumer {
     public:
       TUCallbackType Callback;
if (Context.getSelectionRange().isValid()) {
       R.getBegin().print(llvm::outs(), Context.getSources());
       llvm::outs() << " -> ";
       R.getEnd().print(llvm::outs(), Context.getSources());
       llvm::outs() << "\n";
     }
   }
 
   llvm::Expected<RefactoringActionRule *>
-  getMatchingRule(const RefactoringActionSubcommand &Subcommand) {
+  getMatchingRule(RefactoringActionSubcommand &Subcommand) {
     SmallVector<RefactoringActionRule *, 4> MatchingRules;
     llvm::StringSet<> MissingOptions;
 
     for (const auto &Rule : Subcommand.getActionRules()) {
-      bool SelectionMatches = true;
-      if (Rule->hasSelectionRequirement()) {
-        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());
-        continue;
+        } else {
+          Subcommand.parseSelectionArgument();
+          if (Subcommand.getSelection()) {
+            MatchingRules.push_back(Rule.get());
+          } else {
+            MissingOptions.insert("selection");
+          }
+        }
       }
       for (const RefactoringOption *Opt : Visitor.getMissingRequiredOptions())
         MissingOptions.insert(Opt->getName());
     }
     if (MatchingRules.empty()) {
       std::string Error;
       llvm::raw_string_ostream OS(Error);
       OS << "ERROR: '" << Subcommand.getName()
if (It == SubCommands.end()) {
       OS << "note: the following actions are supported:\n";
       for (const auto &Subcommand : SubCommands)
         OS.indent(2) << Subcommand->getName() << "\n";
       OS.flush();
       return llvm::make_error<llvm::StringError>(
           Error, llvm::inconvertibleErrorCode());
     }
     RefactoringActionSubcommand *Subcommand = &(**It);
-    if (Subcommand->parseArguments())
-      return llvm::make_error<llvm::StringError>(
-          llvm::Twine("Failed to parse arguments for subcommand ") +
-              Subcommand->getName(),
-          llvm::inconvertibleErrorCode());
     return Subcommand;
   }
 
   std::vector<std::unique_ptr<RefactoringActionSubcommand>> SubCommands;
   RefactoringActionSubcommand *SelectedSubcommand;
   RefactoringActionRule *MatchingRule;
   std::unique_ptr<ClangRefactorToolConsumerInterface> Consumer;
   AtomicChanges Changes;