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

45

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

258

Just use if since there is a continue?

263

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

304

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

[sbenza]

include/clang/Tooling/RefactoringCallbacks.h

61

Why emplace_back instead of push_back?

lib/Tooling/RefactoringCallbacks.cpp

253

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.

267

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

285

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

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

Is this covered in the test?

unittests/Tooling/RefactoringCallbacksTest.cpp

101

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

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

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

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

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

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

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

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

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

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

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:

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;
+    }
+    default:
+      llvm_unreachable("Element.Type not recognized");
+    }
+  }
+  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

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