Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[jbangert]

Summary

This is the first change as part of developing a clang-query based search and replace tool.

[ioeric]

lib/Tooling/RefactoringCallbacks.cpp

42 ↗

(On Diff #87372)

Could you add a comment here explaining why the replacements in callbacks need to be cleared?

165 ↗

(On Diff #87372)

Just use if since there is a continue?

170 ↗

(On Diff #87372)

You might want to use llvm_unreachable instead so that it also bails out in non-assertion build.

211 ↗

(On Diff #87372)

Looks like you are using clang-format with Google-style. Please reformat your changes with LLVM style.

[sbenza]

include/clang/Tooling/RefactoringCallbacks.h

61 ↗

(On Diff #87372)

Why emplace_back instead of push_back?

lib/Tooling/RefactoringCallbacks.cpp

160 ↗

(On Diff #87372)

We should do the parsing in the constructor, instead of on each match.
We could store a vector of {string, bool is_id;} that we just iterate during each match.
It also has the advantage of hitting the error+assert before in the run.

174 ↗

(On Diff #87372)

This is making two lookups into the map when one is enough.

192 ↗

(On Diff #87372)

X += instead of X = X +

[jbangert]

Address code feedback & Reformat with clang-format --style llvm.

[jbangert]

• change to push_back

[jbangert]

Thank you for the initial feedback!

include/clang/Tooling/RefactoringCallbacks.h

61 ↗

(On Diff #87372)

Changed to push_back. Is there ever an advantage to using push_back over emplace_back (the latter falls back to a copy constructor).

[jbangert]

(marking other comments as done).

[jbangert]

• use iter

[ioeric]

lib/Tooling/RefactoringCallbacks.cpp

160 ↗

(On Diff #87579)

Is this covered in the test?

unittests/Tooling/RefactoringCallbacksTest.cpp

101 ↗

(On Diff #87579)

Have you rerun the tests? Does this still build?

[jbangert]

• fix test

[jbangert]

Fixed the test -- I am still getting used to the different ninja test targets.

lib/Tooling/RefactoringCallbacks.cpp

160 ↗

(On Diff #87579)

Now it is.

[ioeric]

Looks good. Could you also add test cases where ReplaceNodeWithTemplate::create fails to parse templates?

[sbenza]

include/clang/Tooling/RefactoringCallbacks.h

61 ↗

(On Diff #87372)

push_back and emplace_back are equivalent when the value you are passing is already a T.
In that case, push_back should be used from the principle of using the least powerful tool that can do the job you want.
emplace_back allows for calls to explicit constructors, which makes it more powerful in ways that might be surprising to the reader.
Eg:

std::vector<std::vector<int>> v;
... many lines below ...
v.push_back(1);  // fails
v.emplace_back(1);  // works, but it didn't add a 1 to the vector.

[jbangert]

• additional tests

[jbangert]

Thanks, added tests for parser failures.

[sbenza]

lib/Tooling/RefactoringCallbacks.cpp

213 ↗

(On Diff #87879)

I don't know if stderr is the best place for this error output.
Maybe we should take a sink of some sort in the constructor.

214 ↗

(On Diff #87879)

I don't think this is ok.
afaik, llvm_unreachable leads to undefined behavior if it is reached in opt mode.
This error can be triggered from user input. We should not fail that way.

227 ↗

(On Diff #87879)

Same as above. This error can be triggered from user input.

[jbangert]

use llvm::report_fatal_error instead of unreachable.

[jbangert]

lib/Tooling/RefactoringCallbacks.cpp

213 ↗

(On Diff #87879)

There's a FIXME: above that addresses the need for better error handling, and this one just duplicated their workaround.
Ultimately, clang::tooling:: should probably provide infrastructure for reporting problems (I would imagine all refactoring tools have errors occassionally, and they need some way of reporting/handling them).

214 ↗

(On Diff #87879)

Using llvm::report_fatal_error for now. See the above for better error handling.

[jbangert]

• Merge branch 'master' of http://llvm.org/git/clang into replace_template

[jbangert]

unittests/Tooling/RefactoringCallbacksTest.cpp

101 ↗

(On Diff #87579)

ninja check-clang-tools works.

[ioeric]

Still LGTM. Please run ninja check-clang and fix the warning. I'll land this patch for you afterwards.

lib/Tooling/RefactoringCallbacks.cpp

222 ↗

(On Diff #94282)

ninja check-clang
RefactoringCallbacks.cpp:222:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

Please remove the default.

[jbangert]

Ran check-clang

cfe/trunk/include/clang/Tooling/RefactoringCallbacks.h

 public:
   RefactoringCallback();
   Replacements &getReplacements();
 
 protected:
   Replacements Replace;
 };
 
+/// \brief Adaptor between \c ast_matchers::MatchFinder and \c
+/// tooling::RefactoringTool.
+///
+/// Runs AST matchers and stores the \c tooling::Replacements in a map.
+class ASTMatchRefactorer {
+public:
+  ASTMatchRefactorer(std::map<std::string, Replacements> &FileToReplaces);
+
+  template <typename T>
+  void addMatcher(const T &Matcher, RefactoringCallback *Callback) {
+    MatchFinder.addMatcher(Matcher, Callback);
+    Callbacks.push_back(Callback);
+  }
+
+  void addDynamicMatcher(const ast_matchers::internal::DynTypedMatcher &Matcher,
+                         RefactoringCallback *Callback);
+
+  std::unique_ptr<ASTConsumer> newASTConsumer();
+
+private:
+  friend class RefactoringASTConsumer;
+  std::vector<RefactoringCallback *> Callbacks;
+  ast_matchers::MatchFinder MatchFinder;
+  std::map<std::string, Replacements> &FileToReplaces;
+};
+
 /// \brief Replace the text of the statement bound to \c FromId with the text in
 /// \c ToText.
 class ReplaceStmtWithText : public RefactoringCallback {
 public:
   ReplaceStmtWithText(StringRef FromId, StringRef ToText);
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   std::string FromId;
   std::string ToText;
 };
 
+/// \brief Replace the text of an AST node bound to \c FromId with the result of
+/// evaluating the template in \c ToTemplate.
+///
+/// Expressions of the form ${NodeName} in \c ToTemplate will be
+/// replaced by the text of the node bound to ${NodeName}. The string
+/// "$$" will be replaced by "$".
+class ReplaceNodeWithTemplate : public RefactoringCallback {
+public:
+  static llvm::Expected<std::unique_ptr<ReplaceNodeWithTemplate>>
+  create(StringRef FromId, StringRef ToTemplate);
+  void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  struct TemplateElement {
+    enum { Literal, Identifier } Type;
+    std::string Value;
+  };
+  ReplaceNodeWithTemplate(llvm::StringRef FromId,
+                          std::vector<TemplateElement> &&Template);
+  std::string FromId;
+  std::vector<TemplateElement> Template;
+};
+
 /// \brief Replace the text of the statement bound to \c FromId with the text of
 /// the statement bound to \c ToId.
 class ReplaceStmtWithStmt : public RefactoringCallback {
 public:
   ReplaceStmtWithStmt(StringRef FromId, StringRef ToId);
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:

cfe/trunk/lib/Tooling/RefactoringCallbacks.cpp

 //===--- RefactoringCallbacks.cpp - Structural query framework ------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //
 //===----------------------------------------------------------------------===//
-#include "clang/Lex/Lexer.h"
 #include "clang/Tooling/RefactoringCallbacks.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+
+using llvm::StringError;
+using llvm::make_error;
 
 namespace clang {
 namespace tooling {
 
 RefactoringCallback::RefactoringCallback() {}
 tooling::Replacements &RefactoringCallback::getReplacements() {
   return Replace;
 }
 
-static Replacement replaceStmtWithText(SourceManager &Sources,
-                                       const Stmt &From,
+ASTMatchRefactorer::ASTMatchRefactorer(
+    std::map<std::string, Replacements> &FileToReplaces)
+    : FileToReplaces(FileToReplaces) {}
+
+void ASTMatchRefactorer::addDynamicMatcher(
+    const ast_matchers::internal::DynTypedMatcher &Matcher,
+    RefactoringCallback *Callback) {
+  MatchFinder.addDynamicMatcher(Matcher, Callback);
+  Callbacks.push_back(Callback);
+}
+
+class RefactoringASTConsumer : public ASTConsumer {
+public:
+  RefactoringASTConsumer(ASTMatchRefactorer &Refactoring)
+      : Refactoring(Refactoring) {}
+
+  void HandleTranslationUnit(ASTContext &Context) override {
+    // The ASTMatchRefactorer is re-used between translation units.
+    // Clear the matchers so that each Replacement is only emitted once.
+    for (const auto &Callback : Refactoring.Callbacks) {
+      Callback->getReplacements().clear();
+    }
+    Refactoring.MatchFinder.matchAST(Context);
+    for (const auto &Callback : Refactoring.Callbacks) {
+      for (const auto &Replacement : Callback->getReplacements()) {
+        llvm::Error Err =
+            Refactoring.FileToReplaces[Replacement.getFilePath()].add(
+                Replacement);
+        if (Err) {
+          llvm::errs() << "Skipping replacement " << Replacement.toString()
+                       << " due to this error:\n"
+                       << toString(std::move(Err)) << "\n";
+        }
+      }
+    }
+  }
+
+private:
+  ASTMatchRefactorer &Refactoring;
+};
+
+std::unique_ptr<ASTConsumer> ASTMatchRefactorer::newASTConsumer() {
+  return llvm::make_unique<RefactoringASTConsumer>(*this);
+}
+
+static Replacement replaceStmtWithText(SourceManager &Sources, const Stmt &From,
                                        StringRef Text) {
-  return tooling::Replacement(Sources, CharSourceRange::getTokenRange(
-      From.getSourceRange()), Text);
+  return tooling::Replacement(
+      Sources, CharSourceRange::getTokenRange(From.getSourceRange()), Text);
 }
-static Replacement replaceStmtWithStmt(SourceManager &Sources,
-                                       const Stmt &From,
+static Replacement replaceStmtWithStmt(SourceManager &Sources, const Stmt &From,
                                        const Stmt &To) {
-  return replaceStmtWithText(Sources, From, Lexer::getSourceText(
-      CharSourceRange::getTokenRange(To.getSourceRange()),
+  return replaceStmtWithText(
+      Sources, From,
+      Lexer::getSourceText(CharSourceRange::getTokenRange(To.getSourceRange()),
                            Sources, LangOptions()));
 }
 
 ReplaceStmtWithText::ReplaceStmtWithText(StringRef FromId, StringRef ToText)
     : FromId(FromId), ToText(ToText) {}
 
 void ReplaceStmtWithText::run(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (const Stmt *FromMatch = Result.Nodes.getNodeAs<Stmt>(FromId)) {
if (Body) {
       if (Err) {
         llvm::errs() << llvm::toString(std::move(Err)) << "\n";
         assert(false);
       }
     }
   }
 }
 
+ReplaceNodeWithTemplate::ReplaceNodeWithTemplate(
+    llvm::StringRef FromId, std::vector<TemplateElement> &&Template)
+    : FromId(FromId), Template(Template) {}
+
+llvm::Expected<std::unique_ptr<ReplaceNodeWithTemplate>>
+ReplaceNodeWithTemplate::create(StringRef FromId, StringRef ToTemplate) {
+  std::vector<TemplateElement> ParsedTemplate;
+  for (size_t Index = 0; Index < ToTemplate.size();) {
+    if (ToTemplate[Index] == '$') {
+      if (ToTemplate.substr(Index, 2) == "$$") {
+        Index += 2;
+        ParsedTemplate.push_back(
+            TemplateElement{TemplateElement::Literal, "$"});
+      } else if (ToTemplate.substr(Index, 2) == "${") {
+        size_t EndOfIdentifier = ToTemplate.find("}", Index);
+        if (EndOfIdentifier == std::string::npos) {
+          return make_error<StringError>(
+              "Unterminated ${...} in replacement template near " +
+                  ToTemplate.substr(Index),
+              std::make_error_code(std::errc::bad_message));
+        }
+        std::string SourceNodeName =
+            ToTemplate.substr(Index + 2, EndOfIdentifier - Index - 2);
+        ParsedTemplate.push_back(
+            TemplateElement{TemplateElement::Identifier, SourceNodeName});
+        Index = EndOfIdentifier + 1;
+      } else {
+        return make_error<StringError>(
+            "Invalid $ in replacement template near " +
+                ToTemplate.substr(Index),
+            std::make_error_code(std::errc::bad_message));
+      }
+    } else {
+      size_t NextIndex = ToTemplate.find('$', Index + 1);
+      ParsedTemplate.push_back(
+          TemplateElement{TemplateElement::Literal,
+                          ToTemplate.substr(Index, NextIndex - Index)});
+      Index = NextIndex;
+    }
+  }
+  return std::unique_ptr<ReplaceNodeWithTemplate>(
+      new ReplaceNodeWithTemplate(FromId, std::move(ParsedTemplate)));
+}
+
+void ReplaceNodeWithTemplate::run(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto &NodeMap = Result.Nodes.getMap();
+
+  std::string ToText;
+  for (const auto &Element : Template) {
+    switch (Element.Type) {
+    case TemplateElement::Literal:
+      ToText += Element.Value;
+      break;
+    case TemplateElement::Identifier: {
+      auto NodeIter = NodeMap.find(Element.Value);
+      if (NodeIter == NodeMap.end()) {
+        llvm::errs() << "Node " << Element.Value
+                     << " used in replacement template not bound in Matcher \n";
+        llvm::report_fatal_error("Unbound node in replacement template.");
+      }
+      CharSourceRange Source =
+          CharSourceRange::getTokenRange(NodeIter->second.getSourceRange());
+      ToText += Lexer::getSourceText(Source, *Result.SourceManager,
+                                     Result.Context->getLangOpts());
+      break;
+    }
+    }
+  }
+  if (NodeMap.count(FromId) == 0) {
+    llvm::errs() << "Node to be replaced " << FromId
+                 << " not bound in query.\n";
+    llvm::report_fatal_error("FromId node not bound in MatchResult");
+  }
+  auto Replacement =
+      tooling::Replacement(*Result.SourceManager, &NodeMap.at(FromId), ToText,
+                           Result.Context->getLangOpts());
+  llvm::Error Err = Replace.add(Replacement);
+  if (Err) {
+    llvm::errs() << "Query and replace failed in " << Replacement.getFilePath()
+                 << "! " << llvm::toString(std::move(Err)) << "\n";
+    llvm::report_fatal_error("Replacement failed");
+  }
+}
+
 } // end namespace tooling
 } // end namespace clang

cfe/trunk/unittests/Tooling/RefactoringCallbacksTest.cpp

 //===- unittest/Tooling/RefactoringCallbacksTest.cpp ----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template <typename T>
-void expectRewritten(const std::string &Code,
-                     const std::string &Expected,
-                     const T &AMatcher,
-                     RefactoringCallback &Callback) {
-  MatchFinder Finder;
+void expectRewritten(const std::string &Code, const std::string &Expected,
+                     const T &AMatcher, RefactoringCallback &Callback) {
+  std::map<std::string, Replacements> FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, &Callback);
   std::unique_ptr<tooling::FrontendActionFactory> Factory(
       tooling::newFrontendActionFactory(&Finder));
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
       << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
                                             Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtsWithString) {
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { ; }";
   ReplaceStmtWithText Callback("id", ";");
TEST(RefactoringCallbacksTest, IgnoresStmtsInUncalledMacros) {
   ReplaceStmtWithText Callback("id", ";");
   expectRewritten(Code, Expected, id("id", declStmt()), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesInteger) {
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-                  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-      id("always-false", conditionalOperator(
-          hasCondition(cxxBoolLiteral(equals(false))),
+  expectRewritten(
+      Code, Expected,
+      id("always-false",
+         conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
                              hasFalseExpression(id("should-be", expr())))),
       Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-      id("id", ifStmt(
-          hasCondition(implicitCastExpr(hasSourceExpression(
+  expectRewritten(
+      Code, Expected,
+      id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
                    declRefExpr(to(varDecl(hasName("a"))))))))),
       Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
                   id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false))))),
                   Callback);
 }
 
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  auto Callback = ReplaceNodeWithTemplate::create("id", "FOO");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected, id("id", declStmt()), **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl", "long x = ${init}");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected,
+                  id("decl", varDecl(hasInitializer(id("init", expr())))),
+                  **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl",
+                                                  "string x = \"$$-${init}\"");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected,
+                  id("decl", varDecl(hasInitializer(id("init", expr())))),
+                  **Callback);
+}
+
+static void ExpectStringError(const std::string &Expected,
+                              llvm::Error E) {
+  std::string Found;
+  handleAllErrors(std::move(E), [&](const llvm::StringError &SE) {
+      llvm::raw_string_ostream Stream(Found);
+      SE.log(Stream);
+    });
+  EXPECT_EQ(Expected, Found);
+}
+
+TEST(RefactoringCallbacksTest, TemplateUnterminated) {
+  auto Callback = ReplaceNodeWithTemplate::create("decl",
+                                                  "string x = \"$$-${init\"");
+  ExpectStringError("Unterminated ${...} in replacement template near ${init\"",
+                    Callback.takeError());
+}
+
+TEST(RefactoringCallbacksTest, TemplateUnknownDollar) {
+  auto Callback = ReplaceNodeWithTemplate::create("decl",
+                                                  "string x = \"$<");
+  ExpectStringError("Invalid $ in replacement template near $<",
+                    Callback.takeError());
+}
+
+
 } // end namespace ast_matchers
 } // end namespace clang