clang: Add a tool for MessageLoop refactoring

tools/clang/refactor_message_loop/RefactorMessageLoop.cpp

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// This tool performs the following transformations on base::MessageLoop and
+// related APIs:
+//
+// MessageLoop:
+// - base::MessageLoop::PostTask*
+//   => base::MessageLoop::task_runner()->PostTask*()

[dcheng, 2015/05/06 17:31:18]

Dumb question: why not just update MessageLoop::PostTask to use task_runner()
internally? It seems like we're just adding more typing.

[Sami, 2015/05/07 10:37:37]

There is some more discussion on the linked design doc and bug, but the tl;dr is
that we can make code more modular by not having it depend on the concrete
base::MessageLoop but rather a more abstract task runner.

+//
+// Thread:
+// - base::Thread::message_loop_proxy()
+//   => task_runner()
+//
+// MessageLoopProxy:
+// - base::MessageLoopProxy
+//   => base::SingleThreadTaskRunner
+// - scoped_refptr<base::MessageLoopProxy>
+//   => scoped_refptr<base::SingleThreadTaskRunner>
+// - base::MessageLoopProxy::current()
+//   => base::ThreadTaskRunnerHandle::Get()
+// - base::MessageLoop::message_loop_proxy()
+//   => base::MessageLoop::task_runner() (done)
+//
+
+#include <memory>
+#include "clang/AST/ExprCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/CommandLine.h"
+
+using clang::ast_matchers::MatchFinder;
+using clang::ast_matchers::anyOf;
+using clang::ast_matchers::asString;
+using clang::ast_matchers::callExpr;
+using clang::ast_matchers::callee;
+using clang::ast_matchers::decl;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::hasAncestor;
+using clang::ast_matchers::hasAnyTemplateArgument;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::hasType;
+using clang::ast_matchers::isSameOrDerivedFrom;
+using clang::ast_matchers::isTemplateInstantiation;
+using clang::ast_matchers::memberCallExpr;
+using clang::ast_matchers::methodDecl;
+using clang::ast_matchers::ofClass;
+using clang::ast_matchers::pointsTo;
+using clang::ast_matchers::recordDecl;
+using clang::ast_matchers::refersToType;
+using clang::ast_matchers::templateSpecializationType;
+using clang::ast_matchers::thisPointerType;
+using clang::ast_matchers::unless;
+using clang::ast_matchers::varDecl;
+
+using clang::tooling::CommonOptionsParser;
+using clang::tooling::Replacement;
+using clang::tooling::Replacements;
+
+using clang::CXXMemberCallExpr;
+using clang::CallExpr;
+using clang::Expr;
+using clang::MemberExpr;
+using clang::VarDecl;
+
+namespace {
+
+std::string ReplaceFirst(const std::string& input,

[dcheng, 2015/05/06 17:31:18]

Consider using llvm::StringRef where appropriate, which is roughly equivalent to
StringPiece.

[Sami, 2015/05/07 10:37:37]

Neat, thanks for the tip. Done.

+                         const char* from,
+                         const char* to) {
+  size_t pos = input.find(from);
+  if (pos == std::string::npos)
+    return input;
+  return input.substr(0, pos) + to + input.substr(pos + strlen(from));
+}
+
+// Handles conversion of the Post*Task APIs.
+class PostTaskCallback : public MatchFinder::MatchCallback {
+ public:
+  PostTaskCallback(Replacements* replacements) : replacements_(replacements) {}

[dcheng, 2015/05/06 17:31:18]

explicit

[Sami, 2015/05/07 10:37:37]

Fixed (here and elsewhere).

+
+  virtual void run(const MatchFinder::MatchResult& result) override;

[dcheng, 2015/05/06 17:31:17]

No virtual.

[Sami, 2015/05/07 10:37:37]

Fixed (here and elsewhere).

+
+ private:
+  Replacements* const replacements_;
+};
+
+// Refactors base::MessageLoopProxy::current() callers.
+class CurrentProxyCallback : public MatchFinder::MatchCallback {
+ public:
+  CurrentProxyCallback(Replacements* replacements)
+      : replacements_(replacements) {}
+
+  virtual void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  Replacements* const replacements_;
+};
+
+// Refactors callers to base::MessageLoop::message_loop_proxy() and
+// base::Thread::message_loop_proxy().
+class ProxyGetterCallback : public MatchFinder::MatchCallback {
+ public:
+  ProxyGetterCallback(Replacements* replacements)
+      : replacements_(replacements) {}
+
+  virtual void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  Replacements* const replacements_;
+};
+
+// Rewrites variables of type MessageLoopProxy*.
+class ProxyVariableCallback : public MatchFinder::MatchCallback {
+ public:
+  ProxyVariableCallback(Replacements* replacements)
+      : replacements_(replacements) {}
+
+  virtual void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  Replacements* const replacements_;
+};
+
+// Rewrites variables of type scoped_refptr<MessageLoopProxy>.
+class RefPtrProxyVariableCallback : public MatchFinder::MatchCallback {
+ public:
+  RefPtrProxyVariableCallback(Replacements* replacements)
+      : replacements_(replacements) {}
+
+  virtual void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  Replacements* const replacements_;
+};
+
+class MessageLoopRefactorer {
+ public:
+  explicit MessageLoopRefactorer(Replacements* replacements)
+      : post_callback_(replacements),
+        current_proxy_callback_(replacements),
+        proxy_getter_callback_(replacements),
+        proxy_variable_callback_(replacements),
+        refptr_proxy_variable_callback_(replacements) {}
+
+  void SetupMatchers(MatchFinder* match_finder);
+
+ private:
+  PostTaskCallback post_callback_;
+  CurrentProxyCallback current_proxy_callback_;
+  ProxyGetterCallback proxy_getter_callback_;
+  ProxyVariableCallback proxy_variable_callback_;
+  RefPtrProxyVariableCallback refptr_proxy_variable_callback_;
+};
+
+void MessageLoopRefactorer::SetupMatchers(MatchFinder* match_finder) {
+  auto is_message_loop = recordDecl(isSameOrDerivedFrom("base::MessageLoop"));
+  auto is_thread = recordDecl(isSameOrDerivedFrom("base::Thread"));
+  auto is_message_loop_proxy =
+      recordDecl(isSameOrDerivedFrom("base::MessageLoopProxy"));
+
+  // Matches calls to the Post*Task APIs.
+  auto post_matcher =
+      memberCallExpr(thisPointerType(is_message_loop),
+                     callee(methodDecl(anyOf(
+                         hasName("PostTask"), hasName("PostDelayedTask"),
+                         hasName("PostNonNestableTask"),
+                         hasName("PostNonNestableDelayedTask"))))).bind("call");
+
+  // Matches calls to MessageLoopProxy::current().
+  auto current_proxy_matcher =
+      callExpr(callee(methodDecl(ofClass(is_message_loop_proxy),
+                                 hasName("current")))).bind("call");
+
+  auto message_loop_proxy_callee =
+      callee(methodDecl(hasName("message_loop_proxy")));
+
+  // Matches calls to MessageLoop::message_loop_proxy().
+  auto loop_proxy_getter_matcher =
+      memberCallExpr(thisPointerType(is_message_loop),
+                     message_loop_proxy_callee).bind("call");
+
+  // Matches calls to Thread::message_loop_proxy().
+  auto thread_proxy_getter_matcher =
+      memberCallExpr(thisPointerType(is_thread), message_loop_proxy_callee)
+          .bind("call");
+
+  // Matches variables pointing to a MessageLoopProxy that aren't inside
+  // template instantiations (e.g., scoped_retpr<>).
+  auto proxy_variable_matcher =
+      varDecl(hasType(pointsTo(recordDecl(hasName("base::MessageLoopProxy")))),
+              unless(hasAncestor(decl(anyOf(
+                  recordDecl(isTemplateInstantiation()),
+                  functionDecl(isTemplateInstantiation())))))).bind("var");
+
+  // Matches scoped_refptr<MessageLoopProxy>.
+  auto refptr_proxy_variable_matcher =
+      varDecl(hasType(recordDecl(hasName("::scoped_refptr"))),
+              hasDescendant(templateSpecializationType(hasAnyTemplateArgument(
+                  refersToType(asString("class base::MessageLoopProxy"))))))
+          .bind("var");
+
+  match_finder->addMatcher(current_proxy_matcher, &current_proxy_callback_);
+  match_finder->addMatcher(loop_proxy_getter_matcher, &proxy_getter_callback_);
+  match_finder->addMatcher(thread_proxy_getter_matcher,
+                           &proxy_getter_callback_);
+  match_finder->addMatcher(post_matcher, &post_callback_);
+  match_finder->addMatcher(proxy_variable_matcher, &proxy_variable_callback_);
+  match_finder->addMatcher(refptr_proxy_variable_matcher,
+                           &refptr_proxy_variable_callback_);
+}
+
+void PostTaskCallback::run(const MatchFinder::MatchResult& result) {
+  const CXXMemberCallExpr* call =
+      result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  const Expr* obj = call->getImplicitObjectArgument();
+  const MemberExpr* callee = clang::dyn_cast<MemberExpr>(call->getCallee());
+
+  clang::CharSourceRange range =
+      clang::CharSourceRange::getTokenRange(callee->getSourceRange());
+  clang::CharSourceRange obj_range =
+      clang::CharSourceRange::getTokenRange(obj->getSourceRange());
+
+  std::string text = clang::Lexer::getSourceText(range, *result.SourceManager,
+                                                 result.Context->getLangOpts());
+  std::string obj_text = clang::Lexer::getSourceText(
+      obj_range, *result.SourceManager, result.Context->getLangOpts());
+  std::string method = call->getMethodDecl()->getNameInfo().getAsString();
+
+  // Rewrite the call to go through the task runner.
+  bool is_arrow = callee->isArrow();
+  std::string replacement =
+      obj_text + (is_arrow ? "->" : ".") + "task_runner()->" + method;

[dcheng, 2015/05/06 17:31:17]

Isn't this always -> in Chrome?

[Sami, 2015/05/07 10:37:37]

That's almost always the case, but some tests do have an explicit MessageLoop
instance on the stack, e.g:

  MessageLoop loop;
  loop.PostTask(...);

+
+  // Rewrite MessageLoop::current() as ThreadTaskRunnerHandle::Get().

[dcheng, 2015/05/06 17:31:18]

Why is this chunk here? I would have expected to see this in
CurrentProxyCallback below.

[Sami, 2015/05/07 10:37:37]

CurrentProxyCallback matches calls to MessageLoopProxy::current(), but this one
looks for calls like MessageLoop::current()->PostTask(...). That is, they're
operating on different classes (MessageLoopProxy vs. MessageLoop).

+  if (obj_text.find("MessageLoop::current") != obj_text.npos ||
+      obj_text.find("MessageLoopForIO::current") != obj_text.npos ||
+      obj_text.find("MessageLoopForUI::current") != obj_text.npos) {
+    std::string ns = "";
+    if (obj_text.find("base::") == 0) {
+      ns = "base::";
+    }
+    replacement = ns + "ThreadTaskRunnerHandle::Get()->" + method;
+  }
+
+  replacements_->insert(Replacement(*result.SourceManager, range, replacement));
+}
+
+void CurrentProxyCallback::run(const MatchFinder::MatchResult& result) {
+  const CallExpr* call = result.Nodes.getNodeAs<CallExpr>("call");
+  clang::CharSourceRange range =
+      clang::CharSourceRange::getTokenRange(call->getSourceRange());
+  std::string text = clang::Lexer::getSourceText(range, *result.SourceManager,
+                                                 result.Context->getLangOpts());
+
+  std::string ns = "";
+  if (text.find("base::") == 0) {

[dcheng, 2015/05/06 17:31:18]

Hmm... I guess I should finish my patch to try to figure out the minimum
namespace qualifiers needed for a symbol...

[Sami, 2015/05/07 10:37:37]

Got a link? Sounds useful :)

Although for this particular rewrite pretty much everyone outside base/ needs
the namespace qualifier so I wouldn't imagine being smarter here would help a
lot.

+    ns = "base::";
+  }
+  std::string replacement = ns + "ThreadTaskRunnerHandle::Get()";
+  replacements_->insert(Replacement(*result.SourceManager, range, replacement));
+}
+
+void ProxyGetterCallback::run(const MatchFinder::MatchResult& result) {
+  const CXXMemberCallExpr* call =
+      result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  const MemberExpr* callee = clang::dyn_cast<MemberExpr>(call->getCallee());
+  clang::CharSourceRange range =
+      clang::CharSourceRange::getTokenRange(callee->getSourceRange());
+
+  std::string text = clang::Lexer::getSourceText(range, *result.SourceManager,
+                                                 result.Context->getLangOpts());
+
+  text = ReplaceFirst(text, "message_loop_proxy", "task_runner");
+  replacements_->insert(Replacement(*result.SourceManager, range, text));
+}
+
+void ProxyVariableCallback::run(const MatchFinder::MatchResult& result) {
+  const VarDecl* var = result.Nodes.getNodeAs<VarDecl>("var");
+  clang::CharSourceRange range =
+      clang::CharSourceRange::getTokenRange(var->getSourceRange());
+
+  std::string text = clang::Lexer::getSourceText(range, *result.SourceManager,
+                                                 result.Context->getLangOpts());
+
+  text = ReplaceFirst(text, "MessageLoopProxy", "SingleThreadTaskRunner");
+  replacements_->insert(Replacement(*result.SourceManager, range, text));
+}
+
+void RefPtrProxyVariableCallback::run(const MatchFinder::MatchResult& result) {
+  const VarDecl* var = result.Nodes.getNodeAs<VarDecl>("var");
+  clang::CharSourceRange range =
+      clang::CharSourceRange::getTokenRange(var->getSourceRange());
+
+  std::string text = clang::Lexer::getSourceText(range, *result.SourceManager,
+                                                 result.Context->getLangOpts());
+
+  text = ReplaceFirst(text, "MessageLoopProxy", "SingleThreadTaskRunner");
+  replacements_->insert(Replacement(*result.SourceManager, range, text));
+}
+
+}  // namespace
+
+static llvm::cl::extrahelp common_help(CommonOptionsParser::HelpMessage);
+
+int main(int argc, const char* argv[]) {
+  llvm::cl::OptionCategory category("MessageLoop refactoring tool");
+  CommonOptionsParser options(argc, argv, category);
+  clang::tooling::ClangTool tool(options.getCompilations(),
+                                 options.getSourcePathList());
+
+  Replacements replacements;
+  MessageLoopRefactorer refactorer(&replacements);
+  MatchFinder match_finder;
+  refactorer.SetupMatchers(&match_finder);
+
+  std::unique_ptr<clang::tooling::FrontendActionFactory> frontend_factory =
+      clang::tooling::newFrontendActionFactory(&match_finder);
+  int result = tool.run(frontend_factory.get());
+  if (result != 0)
+    return result;
+
+  // Each replacement line should have the following format:
+  // r:<file path>:<offset>:<length>:<replacement text>
+  // Only the <replacement text> field can contain embedded ":" characters.
+  // TODO(dcheng): Use a more clever serialization. Ideally we'd use the YAML
+  // serialization and then use clang-apply-replacements, but that would require
+  // copying and pasting a larger amount of boilerplate for all Chrome clang
+  // tools.
+  llvm::outs() << "==== BEGIN EDITS ====\n";
+  for (const auto& r : replacements) {
+    llvm::outs() << "r:::" << r.getFilePath() << ":::" << r.getOffset()
+                 << ":::" << r.getLength() << ":::" << r.getReplacementText()
+                 << "\n";
+  }
+  llvm::outs() << "==== END EDITS ====\n";
+
+  return 0;
+}