Add tests to the cpp_refactoring tool. (#539)

This refactors the main.cpp out into a file structure that should make it easier to add more matchers/tests.

migrate_cpp/cpp_refactoring/BUILD

@@ -2,15 +2,56 @@
 # Exceptions. See /LICENSE for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-load("@rules_cc//cc:defs.bzl", "cc_binary")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library", "cc_test")
 
 package(default_visibility = ["//visibility:public"])
 
 cc_binary(
     name = "cpp_refactoring",
     srcs = ["main.cpp"],
+    deps = [
+        ":fn_inserter",
+        "@llvm-project//clang:tooling",
+    ],
+)
+
+cc_library(
+    name = "fn_inserter",
+    srcs = ["fn_inserter.cpp"],
+    hdrs = ["fn_inserter.h"],
+    deps = [":matcher"],
+)
+
+cc_test(
+    name = "fn_inserter_test",
+    srcs = ["fn_inserter_test.cpp"],
+    deps = [
+        ":fn_inserter",
+        ":matcher_test_base",
+        "@llvm-project//clang:tooling",
+        "@llvm-project//llvm:gtest_main",
+    ],
+)
+
+cc_library(
+    name = "matcher",
+    srcs = ["matcher.cpp"],
+    hdrs = ["matcher.h"],
+    deps = [
+        "@llvm-project//clang:ast_matchers",
+        "@llvm-project//clang:tooling_core",
+    ],
+)
+
+cc_library(
+    name = "matcher_test_base",
+    testonly = 1,
+    srcs = ["matcher_test_base.cpp"],
+    hdrs = ["matcher_test_base.h"],
     deps = [
         "@llvm-project//clang:ast_matchers",
         "@llvm-project//clang:tooling",
+        "@llvm-project//llvm:gmock",
+        "@llvm-project//llvm:gtest",
     ],
 )

migrate_cpp/cpp_refactoring/fn_inserter.cpp

@@ -0,0 +1,36 @@
+// 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/fn_inserter.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace cam = ::clang::ast_matchers;
+
+namespace Carbon {
+
+FnInserter::FnInserter(std::map<std::string, Replacements>& in_replacements,
+                       cam::MatchFinder* finder)
+    : Matcher(in_replacements) {
+  // TODO: Switch from isExpansionInMainFile to isDefinition. That should then
+  // include `for (const auto* redecl : func->redecls())` to generate
+  // replacements.
+  finder->addMatcher(
+      cam::functionDecl(cam::isExpansionInMainFile(), cam::hasTrailingReturn())
+          .bind(Label),
+      this);
+}
+
+void FnInserter::run(const cam::MatchFinder::MatchResult& result) {
+  const auto* decl = result.Nodes.getNodeAs<clang::FunctionDecl>(Label);
+  if (!decl) {
+    llvm::report_fatal_error(std::string("getNodeAs failed for ") + Label);
+  }
+  auto begin = decl->getBeginLoc();
+  // Replace the first token in the range, `auto`.
+  auto range = clang::CharSourceRange::getTokenRange(begin, begin);
+  AddReplacement(*(result.SourceManager), range, "fn");
+}
+
+}  // namespace Carbon

migrate_cpp/cpp_refactoring/fn_inserter.h

@@ -0,0 +1,26 @@
+// 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_FN_INSERTER_H_
+#define MIGRATE_CPP_CPP_REFACTORING_FN_INSERTER_H_
+
+#include "migrate_cpp/cpp_refactoring/matcher.h"
+
+namespace Carbon {
+
+// Inserts `fn` for functions and methods.
+class FnInserter : public Matcher {
+ public:
+  explicit FnInserter(std::map<std::string, Replacements>& in_replacements,
+                      MatchFinder* finder);
+
+  void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  static constexpr char Label[] = "FnInserter";
+};
+
+}  // namespace Carbon
+
+#endif  // MIGRATE_CPP_CPP_REFACTORING_FN_INSERTER_H_

migrate_cpp/cpp_refactoring/fn_inserter_test.cpp

@@ -0,0 +1,86 @@
+// 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/fn_inserter.h"
+
+#include "migrate_cpp/cpp_refactoring/matcher_test_base.h"
+
+namespace Carbon {
+namespace {
+
+class FnInserterTest : public MatcherTestBase {
+ protected:
+  FnInserterTest() : fn_inserter(replacements, &finder) {}
+
+  Carbon::FnInserter fn_inserter;
+};
+
+TEST_F(FnInserterTest, TrailingReturn) {
+  constexpr char Before[] = "auto A() -> int;";
+  constexpr char After[] = "fn A() -> int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(FnInserterTest, Inline) {
+  // TODO: Need to re-lex tokens, this should probably be "fn inline" for now.
+  constexpr char Before[] = "inline auto A() -> int;";
+  constexpr char After[] = "fn auto A() -> int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(FnInserterTest, Void) {
+  // TODO: void needs to be handled.
+  constexpr char Before[] = "void A();";
+  ExpectReplacement(Before, Before);
+}
+
+TEST_F(FnInserterTest, Methods) {
+  // TODO: void needs to be handled.
+  // TODO: Need to re-lex tokens, this should probably be "fn virtual" for now.
+  constexpr char Before[] = R"cpp(
+    class Shape {
+     public:
+      virtual void Draw() = 0;
+      virtual auto NumSides() -> int = 0;
+    };
+
+    class Circle : public Shape {
+     public:
+      void Draw() override;
+      auto NumSides() -> int override;
+      auto Radius() -> double { return radius_; }
+
+     private:
+      double radius_;
+    };
+  )cpp";
+  constexpr char After[] = R"(
+    class Shape {
+     public:
+      virtual void Draw() = 0;
+      fn auto NumSides() -> int = 0;
+    };
+
+    class Circle : public Shape {
+     public:
+      void Draw() override;
+      fn NumSides() -> int override;
+      fn Radius() -> double { return radius_; }
+
+     private:
+      double radius_;
+    };
+  )";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(FnInserterTest, LegacyReturn) {
+  // Code should be migrated to trailing returns by clang-tidy, so this is okay
+  // to miss.
+  constexpr char Before[] = "int A();";
+  ExpectReplacement(Before, Before);
+}
+
+}  // namespace
+}  // namespace Carbon

migrate_cpp/cpp_refactoring/main.cpp

@@ -2,90 +2,17 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
+#include "migrate_cpp/cpp_refactoring/fn_inserter.h"
 
 namespace cam = ::clang::ast_matchers;
 namespace ct = ::clang::tooling;
 
-namespace Carbon {
-
-class Matcher : public cam::MatchFinder::MatchCallback {
- public:
-  explicit Matcher(std::map<std::string, ct::Replacements>& in_replacements)
-      : replacements(&in_replacements) {}
-
-  ~Matcher() override = default;
-
-  void AddReplacement(const clang::SourceManager& sm,
-                      clang::CharSourceRange range,
-                      llvm::StringRef replacement_text) {
-    if (!range.isValid()) {
-      llvm::errs() << "Invalid range: " << range.getAsRange().printToString(sm)
-                   << "\n";
-      return;
-    }
-    if (sm.getDecomposedLoc(range.getBegin()).first !=
-        sm.getDecomposedLoc(range.getEnd()).first) {
-      llvm::errs() << "Range spans macro expansions: "
-                   << range.getAsRange().printToString(sm) << "\n";
-      return;
-    }
-    if (sm.getFileID(range.getBegin()) != sm.getFileID(range.getEnd())) {
-      llvm::errs() << "Range spans files: "
-                   << range.getAsRange().printToString(sm) << "\n";
-      return;
-    }
-
-    auto rep =
-        ct::Replacement(sm, sm.getExpansionRange(range), replacement_text);
-    auto err = (*replacements)[std::string(rep.getFilePath())].add(rep);
-    if (err) {
-      llvm::report_fatal_error("Error with replacement `" + rep.toString() +
-                               "`: " + llvm::toString(std::move(err)) + "\n");
-    }
-  }
-
- private:
-  std::map<std::string, ct::Replacements>* replacements;
-};
-
-class FnInserter : public Matcher {
- public:
-  explicit FnInserter(std::map<std::string, ct::Replacements>& in_replacements,
-                      cam::MatchFinder* finder)
-      : Matcher(in_replacements) {
-    finder->addMatcher(cam::functionDecl(cam::isExpansionInMainFile(),
-                                         cam::hasTrailingReturn())
-                           .bind(Label),
-                       this);
-  }
-
-  void run(const cam::MatchFinder::MatchResult& result) override {
-    // The matched 'if' statement was bound to 'ifStmt'.
-    const auto* decl = result.Nodes.getNodeAs<clang::FunctionDecl>(Label);
-    if (!decl) {
-      llvm::report_fatal_error(std::string("getNodeAs failed for ") + Label);
-    }
-    auto begin = decl->getBeginLoc();
-    // Replace the first token in the range, `auto`.
-    auto range = clang::CharSourceRange::getTokenRange(begin, begin);
-    AddReplacement(*(result.SourceManager), range, "fn");
-  }
-
- private:
-  static constexpr char Label[] = "FnInserter";
-};
-
-}  // namespace Carbon
-
 auto main(int argc, const char** argv) -> int {
   llvm::cl::OptionCategory category("C++ refactoring options");
-  clang::tooling::CommonOptionsParser op(argc, argv, category);
-  clang::tooling::RefactoringTool tool(op.getCompilations(),
-                                       op.getSourcePathList());
+  ct::CommonOptionsParser op(argc, argv, category);
+  ct::RefactoringTool tool(op.getCompilations(), op.getSourcePathList());
 
   // Set up AST matcher callbacks.
   cam::MatchFinder finder;

migrate_cpp/cpp_refactoring/matcher.cpp

@@ -0,0 +1,39 @@
+// 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/matcher.h"
+
+namespace ct = ::clang::tooling;
+
+namespace Carbon {
+
+void Matcher::AddReplacement(const clang::SourceManager& sm,
+                             clang::CharSourceRange range,
+                             llvm::StringRef replacement_text) {
+  if (!range.isValid()) {
+    llvm::errs() << "Invalid range: " << range.getAsRange().printToString(sm)
+                 << "\n";
+    return;
+  }
+  if (sm.getDecomposedLoc(range.getBegin()).first !=
+      sm.getDecomposedLoc(range.getEnd()).first) {
+    llvm::errs() << "Range spans macro expansions: "
+                 << range.getAsRange().printToString(sm) << "\n";
+    return;
+  }
+  if (sm.getFileID(range.getBegin()) != sm.getFileID(range.getEnd())) {
+    llvm::errs() << "Range spans files: "
+                 << range.getAsRange().printToString(sm) << "\n";
+    return;
+  }
+
+  auto rep = ct::Replacement(sm, sm.getExpansionRange(range), replacement_text);
+  auto err = (*replacements)[std::string(rep.getFilePath())].add(rep);
+  if (err) {
+    llvm::report_fatal_error("Error with replacement `" + rep.toString() +
+                             "`: " + llvm::toString(std::move(err)) + "\n");
+  }
+}
+
+}  // namespace Carbon

migrate_cpp/cpp_refactoring/matcher.h

@@ -0,0 +1,35 @@
+// 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_MATCHER_H_
+#define MIGRATE_CPP_CPP_REFACTORING_MATCHER_H_
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Core/Replacement.h"
+
+namespace Carbon {
+
+// This is an abstract class with helpers to make it easier to write matchers.
+class Matcher : public clang::ast_matchers::MatchFinder::MatchCallback {
+ public:
+  // Alias these to elide the namespaces in subclass headers.
+  using MatchFinder = clang::ast_matchers::MatchFinder;
+  using Replacements = clang::tooling::Replacements;
+
+  explicit Matcher(std::map<std::string, Replacements>& in_replacements)
+      : replacements(&in_replacements) {}
+
+ protected:
+  // Replaces the given range with the specified text.
+  void AddReplacement(const clang::SourceManager& sm,
+                      clang::CharSourceRange range,
+                      llvm::StringRef replacement_text);
+
+ private:
+  std::map<std::string, Replacements>* const replacements;
+};
+
+}  // namespace Carbon
+
+#endif  // MIGRATE_CPP_CPP_REFACTORING_MATCHER_H_

migrate_cpp/cpp_refactoring/matcher_test_base.cpp

@@ -0,0 +1,38 @@
+// 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/matcher_test_base.h"
+
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace ct = ::clang::tooling;
+
+namespace Carbon {
+
+void MatcherTestBase::ExpectReplacement(llvm::StringRef before,
+                                        llvm::StringRef after) {
+  auto factory = ct::newFrontendActionFactory(&finder);
+  constexpr char Filename[] = "test.cc";
+  ASSERT_TRUE(ct::runToolOnCodeWithArgs(
+      factory->create(), before, {}, Filename, "clang-tool",
+      std::make_shared<clang::PCHContainerOperations>(),
+      ct::FileContentMappings()));
+  auto it = replacements.find(Filename);
+  if (it != replacements.end()) {
+    auto actual = ct::applyAllReplacements(before, it->second);
+    // Split lines to get gmock to get an easier-to-read error.
+    llvm::SmallVector<llvm::StringRef, 0> actual_lines;
+    llvm::SplitString(*actual, actual_lines, "\n");
+    llvm::SmallVector<llvm::StringRef, 0> after_lines;
+    llvm::SplitString(after, after_lines, "\n");
+    EXPECT_THAT(actual_lines, testing::ContainerEq(after_lines));
+  } else {
+    // No replacements; before and after should match.
+    EXPECT_THAT(before, testing::Eq(after));
+  }
+}
+
+}  // namespace Carbon

migrate_cpp/cpp_refactoring/matcher_test_base.h

@@ -0,0 +1,27 @@
+// 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_MATCHER_TEST_BASE_H_
+#define MIGRATE_CPP_CPP_REFACTORING_MATCHER_TEST_BASE_H_
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace Carbon {
+
+class MatcherTestBase : public ::testing::Test {
+ protected:
+  // Expects that that the replacements produced by running the finder result in
+  // the specified code transformation.
+  void ExpectReplacement(llvm::StringRef before, llvm::StringRef after);
+
+  std::map<std::string, clang::tooling::Replacements> replacements;
+  clang::ast_matchers::MatchFinder finder;
+};
+
+}  // namespace Carbon
+
+#endif  // MIGRATE_CPP_CPP_REFACTORING_MATCHER_TEST_BASE_H_