For-range in replacement logic (#631)

This replaces GetAstMatcher with AddMatcher because cxxForRangeStmt is a StatementMatcher. addMatcher has multiple definitions (https://clang.llvm.org/doxygen/classclang_1_1ast__matchers_1_1MatchFinder.html) and so this approach allows using the right addMatcher without writing per-call overloads.

To handle the `var`, I'm considering something like moving VarDecl logic into a VarMatcherBase so that I can just use CXXForRangeStmt's getLoopVariable. The problem is a for-range statement has multiple VarDecls, and getLoopVariable may be the easiest way to identify the real one.

migrate_cpp/cpp_refactoring/BUILD

@@ -11,6 +11,7 @@ cc_binary(
     srcs = ["main.cpp"],
     deps = [
         ":fn_inserter",
+        ":for_range",
         ":var_decl",
         "@llvm-project//clang:tooling",
     ],
@@ -62,6 +63,24 @@ cc_test(
     ],
 )
 
+cc_library(
+    name = "for_range",
+    srcs = ["for_range.cpp"],
+    hdrs = ["for_range.h"],
+    deps = [":matcher"],
+)
+
+cc_test(
+    name = "for_range_test",
+    srcs = ["for_range_test.cpp"],
+    deps = [
+        ":for_range",
+        ":matcher_test_base",
+        "@llvm-project//clang:tooling",
+        "@llvm-project//llvm:gtest_main",
+    ],
+)
+
 cc_library(
     name = "var_decl",
     srcs = ["var_decl.cpp"],

migrate_cpp/cpp_refactoring/fn_inserter.cpp

@@ -35,12 +35,15 @@ void FnInserter::Run() {
   AddReplacement(range, new_text);
 }
 
-auto FnInserterFactory::GetAstMatcher() -> cam::DeclarationMatcher {
-  return cam::functionDecl(cam::anyOf(cam::hasTrailingReturn(),
-                                      cam::returns(cam::asString("void"))),
-                           cam::unless(cam::anyOf(cam::cxxConstructorDecl(),
-                                                  cam::cxxDestructorDecl())))
-      .bind(Label);
+void FnInserterFactory::AddMatcher(cam::MatchFinder* finder,
+                                   cam::MatchFinder::MatchCallback* callback) {
+  finder->addMatcher(
+      cam::functionDecl(cam::anyOf(cam::hasTrailingReturn(),
+                                   cam::returns(cam::asString("void"))),
+                        cam::unless(cam::anyOf(cam::cxxConstructorDecl(),
+                                               cam::cxxDestructorDecl())))
+          .bind(Label),
+      callback);
 }
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/fn_inserter.h

@@ -18,7 +18,9 @@ class FnInserter : public Matcher {
 
 class FnInserterFactory : public MatcherFactoryBase<FnInserter> {
  public:
-  auto GetAstMatcher() -> clang::ast_matchers::DeclarationMatcher override;
+  void AddMatcher(
+      clang::ast_matchers::MatchFinder* finder,
+      clang::ast_matchers::MatchFinder::MatchCallback* callback) override;
 };
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/for_range.cpp

@@ -0,0 +1,29 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "migrate_cpp/cpp_refactoring/for_range.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace cam = ::clang::ast_matchers;
+
+namespace Carbon {
+
+static constexpr char Label[] = "ForRange";
+
+void ForRange::Run() {
+  const auto& stmt = GetNodeAsOrDie<clang::CXXForRangeStmt>(Label);
+
+  // Wrap `in` with spaces so that `for (auto i:items)` has valid results.
+  AddReplacement(clang::CharSourceRange::getTokenRange(stmt.getColonLoc(),
+                                                       stmt.getColonLoc()),
+                 " in ");
+}
+
+void ForRangeFactory::AddMatcher(cam::MatchFinder* finder,
+                                 cam::MatchFinder::MatchCallback* callback) {
+  finder->addMatcher(cam::cxxForRangeStmt().bind(Label), callback);
+}
+
+}  // namespace Carbon

migrate_cpp/cpp_refactoring/for_range.h

@@ -0,0 +1,31 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef MIGRATE_CPP_CPP_REFACTORING_FOR_RANGE_H_
+#define MIGRATE_CPP_CPP_REFACTORING_FOR_RANGE_H_
+
+#include "migrate_cpp/cpp_refactoring/matcher.h"
+
+namespace Carbon {
+
+// Updates variable declarations for `var name: Type`.
+class ForRange : public Matcher {
+ public:
+  using Matcher::Matcher;
+  void Run() override;
+
+ private:
+  auto GetTypeStr(const clang::VarDecl& decl) -> std::string;
+};
+
+class ForRangeFactory : public MatcherFactoryBase<ForRange> {
+ public:
+  void AddMatcher(
+      clang::ast_matchers::MatchFinder* finder,
+      clang::ast_matchers::MatchFinder::MatchCallback* callback) override;
+};
+
+}  // namespace Carbon
+
+#endif  // MIGRATE_CPP_CPP_REFACTORING_FOR_RANGE_H_

migrate_cpp/cpp_refactoring/for_range_test.cpp

@@ -0,0 +1,52 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "migrate_cpp/cpp_refactoring/for_range.h"
+
+#include "migrate_cpp/cpp_refactoring/matcher_test_base.h"
+
+namespace Carbon {
+namespace {
+
+class ForRangeTest : public MatcherTestBase<ForRangeFactory> {};
+
+TEST_F(ForRangeTest, Basic) {
+  constexpr char Before[] = R"cpp(
+    void Foo() {
+      int items[] = {1};
+      for (int i : items) {
+      }
+    }
+  )cpp";
+  constexpr char After[] = R"(
+    void Foo() {
+      int items[] = {1};
+      for (int i  in  items) {
+      }
+    }
+  )";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(ForRangeTest, NoSpace) {
+  // Do not mark `cpp` so that clang-format won't "fix" the `:` spacing.
+  constexpr char Before[] = R"(
+    void Foo() {
+      int items[] = {1};
+      for (int i:items) {
+      }
+    }
+  )";
+  constexpr char After[] = R"(
+    void Foo() {
+      int items[] = {1};
+      for (int i in items) {
+      }
+    }
+  )";
+  ExpectReplacement(Before, After);
+}
+
+}  // namespace
+}  // namespace Carbon

migrate_cpp/cpp_refactoring/main.cpp

@@ -5,6 +5,7 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "migrate_cpp/cpp_refactoring/fn_inserter.h"
+#include "migrate_cpp/cpp_refactoring/for_range.h"
 #include "migrate_cpp/cpp_refactoring/matcher_manager.h"
 #include "migrate_cpp/cpp_refactoring/var_decl.h"
 
@@ -35,6 +36,7 @@ auto main(int argc, const char** argv) -> int {
   // Set up AST matcher callbacks.
   Carbon::MatcherManager matchers(&tool.getReplacements());
   matchers.Register(std::make_unique<Carbon::FnInserterFactory>());
+  matchers.Register(std::make_unique<Carbon::ForRangeFactory>());
   matchers.Register(std::make_unique<Carbon::VarDeclFactory>());
 
   return tool.runAndSave(

migrate_cpp/cpp_refactoring/matcher.h

@@ -71,9 +71,10 @@ class MatcherFactory {
       const clang::ast_matchers::MatchFinder::MatchResult* match_result,
       Matcher::ReplacementMap* replacements) -> std::unique_ptr<Matcher> = 0;
 
-  // Returns the AST matcher which determines when the Matcher is instantiated
-  // and run.
-  virtual auto GetAstMatcher() -> clang::ast_matchers::DeclarationMatcher = 0;
+  // Adds the Matcher to the finder with the provided callback.
+  virtual void AddMatcher(
+      clang::ast_matchers::MatchFinder* finder,
+      clang::ast_matchers::MatchFinder::MatchCallback* callback) = 0;
 };
 
 // A convenience factory that implements CreateMatcher for Matchers that have a

migrate_cpp/cpp_refactoring/matcher_manager.h

@@ -34,7 +34,7 @@ class MatcherManager {
                                   std::unique_ptr<MatcherFactory> in_factory,
                                   Matcher::ReplacementMap* in_replacements)
         : factory(std::move(in_factory)), replacements(in_replacements) {
-      finder->addMatcher(factory->GetAstMatcher(), this);
+      factory->AddMatcher(finder, this);
     }
 
     void run(const clang::ast_matchers::MatchFinder::MatchResult& match_result)

migrate_cpp/cpp_refactoring/var_decl.cpp

@@ -131,10 +131,12 @@ void VarDecl::Run() {
       clang::CharSourceRange::getCharRange(replace_start, replace_end), after);
 }
 
-auto VarDeclFactory::GetAstMatcher() -> cam::DeclarationMatcher {
-  return cam::varDecl(cam::unless(cam::hasParent(cam::declStmt(
-                          cam::hasParent(cam::cxxForRangeStmt())))))
-      .bind(Label);
+void VarDeclFactory::AddMatcher(cam::MatchFinder* finder,
+                                cam::MatchFinder::MatchCallback* callback) {
+  finder->addMatcher(cam::varDecl(cam::unless(cam::hasParent(cam::declStmt(
+                                      cam::hasParent(cam::cxxForRangeStmt())))))
+                         .bind(Label),
+                     callback);
 }
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/var_decl.h

@@ -21,7 +21,9 @@ class VarDecl : public Matcher {
 
 class VarDeclFactory : public MatcherFactoryBase<VarDecl> {
  public:
-  auto GetAstMatcher() -> clang::ast_matchers::DeclarationMatcher override;
+  void AddMatcher(
+      clang::ast_matchers::MatchFinder* finder,
+      clang::ast_matchers::MatchFinder::MatchCallback* callback) override;
 };
 
 }  // namespace Carbon