Start handling variable declarations (#571)

TODOs in the test for known issues.

I may switch the approach to getting the variable type (on examination, this isn't working quite as well as I'd thought) but for now I think it's okay. I had an earlier approach though that may work better overall -- I'd been thinking this would work better, but as you can see in the null check for type information, I think I missed a key point.

Anyways, what'd really been vexing me was `int i, j` which I think I handle passably well now. There's obviously room for improvement, but given I've been going at this for a couple days now, I thought it best to checkpoint where I was.

This also includes some related framework changes to fix bumps I was running into. Overall the tool should operate a bit more smoothly with these changes. There are still issues with overlapping replacements, but I think it's primarily with range-based for loops which I just need to take some time to fix.

migrate_cpp/cpp_refactoring/BUILD

@@ -11,28 +11,11 @@ cc_binary(
     srcs = ["main.cpp"],
     deps = [
         ":fn_inserter",
+        ":var_decl",
         "@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"],
@@ -55,3 +38,41 @@ cc_library(
         "@llvm-project//llvm:gtest",
     ],
 )
+
+# Individual matchers
+
+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 = "var_decl",
+    srcs = ["var_decl.cpp"],
+    hdrs = ["var_decl.h"],
+    deps = [":matcher"],
+)
+
+cc_test(
+    name = "var_decl_test",
+    srcs = ["var_decl_test.cpp"],
+    deps = [
+        ":matcher_test_base",
+        ":var_decl",
+        "@llvm-project//clang:tooling",
+        "@llvm-project//llvm:gtest_main",
+    ],
+)

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/var_decl.h"
 
 namespace cam = ::clang::ast_matchers;
 namespace ct = ::clang::tooling;
@@ -32,8 +33,10 @@ auto main(int argc, const char** argv) -> int {
   InitReplacements(&tool);
 
   // Set up AST matcher callbacks.
+  auto& repl = tool.getReplacements();
   cam::MatchFinder finder;
-  Carbon::FnInserter fn_inserter(tool.getReplacements(), &finder);
+  Carbon::FnInserter fn_inserter(repl, &finder);
+  Carbon::VarDecl var_decl(repl, &finder);
 
   return tool.runAndSave(
       clang::tooling::newFrontendActionFactory(&finder).get());

migrate_cpp/cpp_refactoring/matcher.cpp

@@ -12,19 +12,16 @@ 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";
+    // Invalid range.
     return;
   }
   if (sm.getDecomposedLoc(range.getBegin()).first !=
       sm.getDecomposedLoc(range.getEnd()).first) {
-    llvm::errs() << "Range spans macro expansions: "
-                 << range.getAsRange().printToString(sm) << "\n";
+    // Range spans macro expansions.
     return;
   }
   if (sm.getFileID(range.getBegin()) != sm.getFileID(range.getEnd())) {
-    llvm::errs() << "Range spans files: "
-                 << range.getAsRange().printToString(sm) << "\n";
+    // Range spans files.
     return;
   }
 
@@ -38,8 +35,8 @@ void Matcher::AddReplacement(const clang::SourceManager& sm,
 
   auto err = entry->second.add(rep);
   if (err) {
-    llvm::report_fatal_error("Error with replacement `" + rep.toString() +
-                             "`: " + llvm::toString(std::move(err)) + "\n");
+    llvm::errs() << "Error with replacement `" << rep.toString()
+                 << "`: " << llvm::toString(std::move(err)) << "\n";
   }
 }
 

migrate_cpp/cpp_refactoring/matcher_test_base.cpp

@@ -24,12 +24,16 @@ void MatcherTestBase::ExpectReplacement(llvm::StringRef before,
       ct::FileContentMappings()));
   EXPECT_THAT(replacements, testing::ElementsAre(testing::Key(Filename)));
   auto actual = ct::applyAllReplacements(before, replacements[Filename]);
-  // 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));
+  if (after.find('\n') == std::string::npos) {
+    EXPECT_THAT(*actual, testing::Eq(after.str()));
+  } else {
+    // 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));
+  }
 }
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/var_decl.cpp

@@ -0,0 +1,61 @@
+// 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/var_decl.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
+
+namespace cam = ::clang::ast_matchers;
+
+namespace Carbon {
+
+VarDecl::VarDecl(std::map<std::string, Replacements>& in_replacements,
+                 cam::MatchFinder* finder)
+    : Matcher(in_replacements) {
+  finder->addMatcher(cam::varDecl().bind(Label), this);
+}
+
+void VarDecl::run(const cam::MatchFinder::MatchResult& result) {
+  const auto* decl = result.Nodes.getNodeAs<clang::VarDecl>(Label);
+  if (!decl) {
+    llvm::report_fatal_error(std::string("getNodeAs failed for ") + Label);
+  }
+
+  auto& sm = *(result.SourceManager);
+  auto lang_opts = result.Context->getLangOpts();
+
+  std::string after;
+  // Start the replacement with "var" unless it's a parameter.
+  if (result.Nodes.getNodeAs<clang::ParmVarDecl>(Label) == nullptr) {
+    after = "var ";
+  }
+  // Finish the "type: name" replacement.
+  after += decl->getNameAsString() + ": " +
+           clang::QualType::getAsString(decl->getType().split(), lang_opts);
+
+  if (decl->getTypeSourceInfo() == nullptr) {
+    // TODO: Need to understand what's happening in this case. Not sure if we
+    // need to address it.
+    return;
+  }
+
+  // This decides the range to replace. Normally the entire decl is replaced,
+  // but for code like `int i, j` we need to detect the comma between the
+  // declared names. That case currently results in `var i: int, var j: int`.
+  auto type_loc = decl->getTypeSourceInfo()->getTypeLoc();
+  auto after_type_loc =
+      clang::Lexer::getLocForEndOfToken(type_loc.getEndLoc(), 0, sm, lang_opts);
+  // If there's a comma, this range will be non-empty.
+  auto comma_source_text = clang::Lexer::getSourceText(
+      clang::CharSourceRange::getCharRange(after_type_loc, decl->getLocation()),
+      sm, lang_opts);
+  bool has_comma = !comma_source_text.trim().empty();
+  clang::CharSourceRange replace_range = clang::CharSourceRange::getTokenRange(
+      has_comma ? decl->getLocation() : decl->getBeginLoc(), decl->getEndLoc());
+
+  AddReplacement(sm, replace_range, after);
+}
+
+}  // namespace Carbon

migrate_cpp/cpp_refactoring/var_decl.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_VAR_DECL_H_
+#define MIGRATE_CPP_CPP_REFACTORING_VAR_DECL_H_
+
+#include "migrate_cpp/cpp_refactoring/matcher.h"
+
+namespace Carbon {
+
+// Updates variable declarations for `var name: Type`.
+class VarDecl : public Matcher {
+ public:
+  explicit VarDecl(std::map<std::string, Replacements>& in_replacements,
+                   MatchFinder* finder);
+
+  void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  static constexpr char Label[] = "VarDecl";
+};
+
+}  // namespace Carbon
+
+#endif  // MIGRATE_CPP_CPP_REFACTORING_VAR_DECL_H_

migrate_cpp/cpp_refactoring/var_decl_test.cpp

@@ -0,0 +1,125 @@
+// 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/var_decl.h"
+
+#include "migrate_cpp/cpp_refactoring/matcher_test_base.h"
+
+namespace Carbon {
+namespace {
+
+class VarDeclTest : public MatcherTestBase {
+ protected:
+  VarDeclTest() : var_decl(replacements, &finder) {}
+
+  Carbon::VarDecl var_decl;
+};
+
+TEST_F(VarDeclTest, Declaration) {
+  constexpr char Before[] = "int i;";
+  constexpr char After[] = "var i: int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, DeclarationArray) {
+  constexpr char Before[] = "int i[4];";
+  constexpr char After[] = "var i: int [4];";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, DeclarationComma) {
+  // TODO: Maybe replace the comma with a `;`.
+  constexpr char Before[] = "int i, j;";
+  constexpr char After[] = "var i: int, var j: int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, Assignment) {
+  constexpr char Before[] = "int i = 0;";
+  // TODO: Include init.
+  constexpr char After[] = "var i: int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, Auto) {
+  constexpr char Before[] = "auto i = 0;";
+  // TODO: Keep auto.
+  constexpr char After[] = "var i: int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, Const) {
+  // TODO: Include init, have `const` indicate `let`.
+  constexpr char Before[] = "const int i = 0;";
+  constexpr char After[] = "var i: const int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, Params) {
+  constexpr char Before[] = "auto Foo(int i) -> int;";
+  constexpr char After[] = "auto Foo(i: int) -> int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, ParamsDefault) {
+  // TODO: Include init.
+  constexpr char Before[] = "auto Foo(int i = 0) -> int;";
+  constexpr char After[] = "auto Foo(i: int) -> int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, ParamsConst) {
+  constexpr char Before[] = "auto Foo(const int i) -> int;";
+  constexpr char After[] = "auto Foo(i: const int) -> int;";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, ParamStruct) {
+  // This is to ensure the 'struct' keyword doesn't get added to the call type.
+  constexpr char Before[] = R"cpp(
+    struct Circle {};
+    auto Draw(int times, const Circle& circle) -> bool;
+  )cpp";
+  constexpr char After[] = R"(
+    struct Circle {};
+    auto Draw(times: int, circle: const Circle &) -> bool;
+  )";
+  ExpectReplacement(Before, After);
+}
+
+TEST_F(VarDeclTest, Member) {
+  // TODO: Handle member variables.
+  constexpr char Before[] = R"cpp(
+    struct Circle {
+      Circle() : x(0), y(0), radius(1) {}
+
+      int x;
+      int y;
+      int radius;
+    };
+  )cpp";
+  ExpectReplacement(Before, Before);
+}
+
+TEST_F(VarDeclTest, RangeFor) {
+  // TODO: Handle range based for loops.
+  constexpr char Before[] = R"cpp(
+    void Foo() {
+      int items[] = {1};
+      for (int i : items) {
+      }
+    }
+  )cpp";
+  constexpr char After[] = R"(
+    void Foo() {
+      var items: int [1];
+      for (int i var __begin1: int * var __range1: int (&)[1]) {
+      }
+    }
+  )";
+  ExpectReplacement(Before, After);
+}
+
+}  // namespace
+}  // namespace Carbon