value_cleanup: Rewrite more base::ListValue::Append(base::Value*) calls.

tools/clang/value_cleanup/ListValueRewriter.cpp

Index: tools/clang/value_cleanup/ListValueRewriter.cpp
diff --git a/tools/clang/value_cleanup/ListValueRewriter.cpp b/tools/clang/value_cleanup/ListValueRewriter.cpp
index b5e5260eab1e6210d71425bb85079440e94f6246..4847d5ca82a14e15b86284c00e1f3a3699c5fba2 100644
--- a/tools/clang/value_cleanup/ListValueRewriter.cpp
+++ b/tools/clang/value_cleanup/ListValueRewriter.cpp
@@ -8,6 +8,8 @@
 #include <algorithm>
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersMacros.h"
@@ -135,17 +137,212 @@ void ListValueRewriter::AppendReleasedUniquePtrCallback::run(
   replacements_->emplace(*result.SourceManager, insertion_range, "std::move(");
 }
 
+ListValueRewriter::AppendRawPtrCallback::AppendRawPtrCallback(
+    Replacements* replacements)
+    : replacements_(replacements) {}
+
+void ListValueRewriter::AppendRawPtrCallback::run(
+    const MatchFinder::MatchResult& result) {
+  auto* var_decl = result.Nodes.getNodeAs<clang::VarDecl>("varDecl");
+  // As an optimization, skip processing if it's already been visited, since
+  // this match callback walks the entire function body.
+  if (visited_.find(var_decl) != visited_.end())
+    return;
+  visited_.insert(var_decl);
+  auto* function_context = var_decl->getParentFunctionOrMethod();
+  assert(function_context && "local var not in function context?!");
+  auto* function_decl = clang::cast<clang::FunctionDecl>(function_context);
+
+  auto* type_source_info = var_decl->getTypeSourceInfo();
+  assert(type_source_info && "no type source info for VarDecl?!");
+  // Don't bother trying to handle qualifiers.
+  clang::QualType qual_type = var_decl->getType();
+  if (qual_type.hasQualifiers()) {
+    return;
+  }
+
+  // Helper class to visit each DeclRefExpr for this VarDecl. If it finds a
+  // DeclRefExpr it can't figure out how to rewrite, the traversal will be
+  // terminated early.
+  class CollectDeclRefExprVisitor

[danakj, 2016/06/10 21:59:55]

move this out of the method i think

[dcheng, 2016/06/10 22:39:43]

Done.

+      : public clang::RecursiveASTVisitor<CollectDeclRefExprVisitor> {
+   public:
+    CollectDeclRefExprVisitor(clang::SourceManager* source_manager,
+                              clang::ASTContext* ast_context,
+                              const clang::VarDecl* decl,
+                              const clang::FunctionDecl* containing_function)
+        : source_manager_(source_manager),
+          ast_context_(ast_context),
+          decl_(decl),
+          is_valid_(decl->hasInit()),
+          map_(containing_function->getBody()) {}
+
+    bool VisitDeclRefExpr(const clang::DeclRefExpr* expr) {
+      if (expr->getDecl() != decl_)
+        return true;
+
+      const clang::Stmt* stmt = expr;
+      while (stmt) {
+        // TODO(dcheng): Add a const version of getParentIgnoreParenImpCasts.
+        stmt =
+            map_.getParentIgnoreParenImpCasts(const_cast<clang::Stmt*>(stmt));
+
+        if (clang::isa<clang::MemberExpr>(stmt)) {
+          // Member expressions need no special rewriting since std::unique_ptr
+          // overloads `.' and `->'.
+          return is_valid_;
+        } else if (auto* member_call_expr =
+                       clang::dyn_cast<clang::CXXMemberCallExpr>(stmt)) {
+          return HandleMemberCallExpr(member_call_expr, expr);
+        } else if (auto* binary_op =
+                       clang::dyn_cast<clang::BinaryOperator>(stmt)) {
+          return HandleBinaryOp(binary_op);
+        } else {
+          // Can't handle this so cancel the rewrite.
+          stmt->dump();
+          return false;
+        }
+      }
+
+      assert(false);
+      return false;
+    }
+
+    const Replacements& replacements() const { return replacements_; }
+
+   private:
+    bool HandleMemberCallExpr(const clang::CXXMemberCallExpr* member_call_expr,
+                              const clang::DeclRefExpr* decl_ref_expr) {
+      // If this isn't a ListValue::Append() call, cancel the rewrite: it
+      // will require manual inspection to determine if it's an ownership
+      // transferring call or not.
+      auto* method_decl = member_call_expr->getMethodDecl();
+      if (method_decl->getQualifiedNameAsString() != "base::ListValue::Append")
+        return false;
+      // Use-after-move is also a fatal error.
+      if (!is_valid_)
+        return false;
+
+      is_valid_ = false;
+
+      // Surround the DeclRefExpr with std::move().
+      replacements_.emplace(*source_manager_, decl_ref_expr->getLocStart(), 0,
+                            "std::move(");
+
+      clang::SourceLocation end = clang::Lexer::getLocForEndOfToken(
+          decl_ref_expr->getLocEnd(), 0, *source_manager_,
+          ast_context_->getLangOpts());
+      replacements_.emplace(*source_manager_, end, 0, ")");
+      return true;
+    }
+
+    bool HandleBinaryOp(const clang::BinaryOperator* op) {
+      if (op->isRelationalOp() || op->isEqualityOp() || op->isLogicalOp()) {
+        // Supported binary operations for which no rewrites need to be done.
+        return is_valid_;
+      }
+      if (!op->isAssignmentOp()) {
+        // Pointer arithmetic or something else clever. Just cancel the rewrite.
+        return false;
+      }
+      if (op->isCompoundAssignmentOp()) {
+        // +=, -=, etc. Give up and cancel the rewrite.
+        return false;
+      }
+
+      const clang::Expr* rhs = op->getRHS()->IgnoreParenImpCasts();
+      const clang::CXXNewExpr* new_expr =
+          clang::dyn_cast<clang::CXXNewExpr>(rhs);
+      if (!new_expr) {
+        // The variable isn't being assigned the result of a new operation. Just
+        // cancel the rewrite.
+        return false;
+      }
+
+      is_valid_ = true;
+
+      // Rewrite the assignment operation to use std::unique_ptr::reset().
+      clang::CharSourceRange range = clang::CharSourceRange::getCharRange(
+          op->getOperatorLoc(), op->getRHS()->getLocStart());
+      replacements_.emplace(*source_manager_, range, ".reset(");
+
+      clang::SourceLocation expr_end = clang::Lexer::getLocForEndOfToken(
+          op->getLocEnd(), 0, *source_manager_, ast_context_->getLangOpts());
+      replacements_.emplace(*source_manager_, expr_end, 0, ")");
+      return true;
+    }
+
+    clang::SourceManager* const source_manager_;
+    clang::ASTContext* const ast_context_;
+    const clang::VarDecl* const decl_;
+    // Tracks the state of |decl_| during the traversal. |decl_| becomes valid
+    // upon initialization/assignment and becomes invalid when passed as an
+    // argument to base::ListValue::Append(base::Value*).
+    bool is_valid_;
+    clang::ParentMap map_;
+    Replacements replacements_;
+  };
+
+  CollectDeclRefExprVisitor visitor(result.SourceManager, result.Context,
+                                    var_decl, function_decl);
+  if (!visitor.TraverseStmt(function_decl->getBody()))
+    return;
+
+  // Rewrite the variable type to use std::unique_ptr.
+  clang::CharSourceRange type_range = clang::CharSourceRange::getTokenRange(
+      type_source_info->getTypeLoc().getSourceRange());
+  std::string replacement_type = "std::unique_ptr<";
+  while (true) {
+    const clang::Type* type = qual_type.getTypePtr();
+    if (auto* auto_type = type->getAs<clang::AutoType>()) {
+      if (!auto_type->isDeduced()) {
+        // If an AutoType isn't deduced, the rewriter can't do anything.
+        return;
+      }
+      qual_type = auto_type->getDeducedType();
+    } else if (auto* pointer_type = type->getAs<clang::PointerType>()) {
+      qual_type = pointer_type->getPointeeType();
+    } else {
+      break;
+    }
+  }
+  replacement_type += qual_type.getAsString();
+  replacement_type += ">";
+  replacements_->emplace(*result.SourceManager, type_range, replacement_type);
+
+  // Initialized with `='
+  if (var_decl->hasInit() &&
+      var_decl->getInitStyle() == clang::VarDecl::CInit) {
+    clang::SourceLocation name_end = clang::Lexer::getLocForEndOfToken(
+        var_decl->getLocation(), 0, *result.SourceManager,
+        result.Context->getLangOpts());
+    clang::CharSourceRange range = clang::CharSourceRange::getCharRange(
+        name_end, var_decl->getInit()->getLocStart());
+    replacements_->emplace(*result.SourceManager, range, "(");

[danakj, 2016/06/10 21:59:55]

Can you skip the () if the range is empty (there will be nothing inside them)?

[dcheng, 2016/06/10 22:39:43]

I could, but that makes this already complicated logic more complicated: we'll
need to emit a replacement to remove the () or do something more clever. This
code is only emitting the outer parens in:

std::unique_ptr<base::ListValue> x(new base::ListValue());

The inner parens come from the original text.

We can clean these up separate in a much simpler tool if we want to.

+
+    clang::SourceLocation init_end = clang::Lexer::getLocForEndOfToken(
+        var_decl->getInit()->getLocEnd(), 0, *result.SourceManager,
+        result.Context->getLangOpts());
+    replacements_->emplace(*result.SourceManager, init_end, 0, ")");
+  }
+
+  // Also append the collected replacements from visiting the DeclRefExprs.
+  replacements_->insert(visitor.replacements().begin(),
+                        visitor.replacements().end());
+}
+
 ListValueRewriter::ListValueRewriter(Replacements* replacements)
     : append_boolean_callback_(replacements),
       append_integer_callback_(replacements),
       append_double_callback_(replacements),
       append_string_callback_(replacements),
-      append_released_unique_ptr_callback_(replacements) {}
+      append_released_unique_ptr_callback_(replacements),
+      append_raw_ptr_callback_(replacements) {}
 
 void ListValueRewriter::RegisterMatchers(MatchFinder* match_finder) {
-  auto is_list_append =
-      allOf(callee(cxxMethodDecl(hasName("::base::ListValue::Append"))),
-            argumentCountIs(1));
+  auto is_list_append = cxxMemberCallExpr(
+      callee(cxxMethodDecl(hasName("::base::ListValue::Append"))),
+      argumentCountIs(1));
 
   // base::ListValue::Append(new base::FundamentalValue(bool))
   //     => base::ListValue::AppendBoolean()
@@ -241,4 +438,25 @@ void ListValueRewriter::RegisterMatchers(MatchFinder* match_finder) {
                                           is_unique_ptr_release,
                                           on(id("objectExpr", expr()))))))),
       &append_released_unique_ptr_callback_);
+
+  // Simple versions of the following pattern. Note the callback itself does
+  // much of the filtering (to detect use-after-move, things that aren't
+  // assigned the result of a new expression, etc).
+  // base::ListValue* this_list = new base::ListValue;

[danakj, 2016/06/10 21:59:55]

Mention what it will turn this code into?

[dcheng, 2016/06/10 22:39:43]

Done.

+  // this_list->AppendInteger(1);
+  // that_list->Append(this_list);
+  match_finder->addMatcher(
+      cxxMemberCallExpr(
+          is_list_append,
+          hasArgument(
+              0, ignoringParenImpCasts(id(
+                     "declRefExpr",
+                     declRefExpr(to(id(
+                         "varDecl",
+                         varDecl(hasLocalStorage(),
+                                 anyOf(hasInitializer(
+                                           ignoringParenImpCasts(cxxNewExpr())),
+                                       unless(hasInitializer(expr()))),
+                                 unless(parmVarDecl()))))))))),

[danakj, 2016/06/10 21:59:55]

This needs more )

+      &append_raw_ptr_callback_);
 }