Handling of UnresolvedUsingValueDecl nodes.

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.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.
 //
 // Changes Blink-style names to Chrome-style names. Currently transforms:
 //   fields:
 //     int m_operationCount => int operation_count_
 //   variables (including parameters):
 //     int mySuperVariable => int my_super_variable
 //   constants:
@@ skipping 33 matching lines @@
 using clang::tooling::CommonOptionsParser;
 using clang::tooling::Replacement;
 using llvm::StringRef;
 
 namespace {
 
 const char kBlinkFieldPrefix[] = "m_";
 const char kBlinkStaticMemberPrefix[] = "s_";
 const char kGeneratedFileRegex[] = "^gen/|/gen/";
 
+template <typename MatcherType, typename NodeType>
+bool IsMatching(const MatcherType& matcher,
+                const NodeType& node,
+                clang::ASTContext& context) {
+  return !match(matcher, node, context).empty();
+}
+
 const clang::ast_matchers::internal::
     VariadicDynCastAllOfMatcher<clang::Expr, clang::UnresolvedMemberExpr>
         unresolvedMemberExpr;
 
 AST_MATCHER(clang::FunctionDecl, isOverloadedOperator) {
   return Node.isOverloadedOperator();
 }
 
 AST_MATCHER(clang::CXXMethodDecl, isInstanceMethod) {
   return Node.isInstance();
@@ skipping 141 matching lines @@
   // Subclasses of InspectorAgent will subclass "disable()" from both blink and
   // from gen/, which is problematic, but DevTools folks don't want to rename
   // it or split this up. So don't rename it at all.
   if (name.equals("disable") &&
       IsMethodOverrideOf(decl, "blink::InspectorAgent"))
     return true;
 
   return false;
 }
 
+bool IsBlacklistedFunctionOrMethodName(llvm::StringRef name) {
+  static const char* kBlacklistedNames[] = {
+      // From IsBlacklistedFunction:
+      "swap",
+      // From IsBlacklistedMethod:
+      "trace", "traceImpl", "lock", "unlock", "try_lock", "begin", "end",
+      "rbegin", "rend", "disable",
+  };
+  for (const auto& b : kBlacklistedNames) {
+    if (name == b)
+      return true;
+  }
+  return false;
+}
+
 AST_MATCHER(clang::FunctionDecl, isBlacklistedFunction) {
   return IsBlacklistedFunction(Node);
 }
 
 AST_MATCHER(clang::CXXMethodDecl, isBlacklistedMethod) {
   return IsBlacklistedMethod(Node);
 }
 
 // Helper to convert from a camelCaseName to camel_case_name. It uses some
 // heuristics to try to handle acronyms in camel case names correctly.
@@ skipping 90 matching lines @@
   // - Foo foo()  // Direct application of |type_with_same_name_as_function|.
   // - Foo* foo()  // |hasDescendant| traverses references/pointers.
   // - RefPtr<Foo> foo()  // |hasDescendant| traverses template arguments.
   auto type_containing_same_name_as_function =
       qualType(anyOf(type_with_same_name_as_function,
                      hasDescendant(type_with_same_name_as_function)));
   // https://crbug.com/582312: Prepend "Get" if method name conflicts with
   // return type.
   auto conflict_matcher =
       functionDecl(returns(type_containing_same_name_as_function));
-  if (!match(conflict_matcher, decl, context).empty())
+  if (IsMatching(conflict_matcher, decl, context))
     name = "Get" + name;
 
   return true;
 }
 
 bool GetNameForDecl(const clang::EnumConstantDecl& decl,
                     clang::ASTContext& context,
                     std::string& name) {
   StringRef original_name = decl.getName();
 
@@ skipping 193 matching lines @@
 
 template <>
 struct TargetNodeTraits<clang::UnresolvedMemberExpr> {
   static clang::SourceLocation GetLoc(const clang::UnresolvedMemberExpr& expr) {
     return expr.getMemberLoc();
   }
   static const char* GetName() { return "expr"; }
   static const char* GetType() { return "UnresolvedMemberExpr"; }
 };
 
-template <typename DeclNode, typename TargetNode>
+template <>
+struct TargetNodeTraits<clang::UnresolvedUsingValueDecl> {
+  static clang::SourceLocation GetLoc(
+      const clang::UnresolvedUsingValueDecl& decl) {
+    return decl.getNameInfo().getLoc();
+  }
+  static const char* GetName() { return "decl"; }
+  static const char* GetType() { return "UnresolvedUsingValueDecl"; }
+};
+
+template <typename TargetNode>
 class RewriterBase : public MatchFinder::MatchCallback {
  public:
   explicit RewriterBase(std::set<Replacement>* replacements)
       : replacements_(replacements) {}
 
+  const TargetNode& GetTargetNode(const MatchFinder::MatchResult& result) {
+    const TargetNode* target_node = result.Nodes.getNodeAs<TargetNode>(
+        TargetNodeTraits<TargetNode>::GetName());
+    assert(target_node);
+    return *target_node;
+  }
+
+  void AddReplacement(const MatchFinder::MatchResult& result,
+                      llvm::StringRef old_name,
+                      std::string new_name) {
+    if (old_name == new_name)
+      return;
+
+    clang::SourceLocation loc =
+        TargetNodeTraits<TargetNode>::GetLoc(GetTargetNode(result));
+    clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc);
+    replacements_->emplace(*result.SourceManager, range, new_name);
+    replacement_names_.emplace(old_name.str(), std::move(new_name));
+  }
+
+  const std::unordered_map<std::string, std::string>& replacement_names()
+      const {
+    return replacement_names_;
+  }
+
+ private:
+  std::set<Replacement>* const replacements_;
+  std::unordered_map<std::string, std::string> replacement_names_;
+};
+
+template <typename DeclNode, typename TargetNode>
+class DeclRewriterBase : public RewriterBase<TargetNode> {
+ public:
+  using Base = RewriterBase<TargetNode>;
+
+  explicit DeclRewriterBase(std::set<Replacement>* replacements)
+      : Base(replacements) {}
+
   void run(const MatchFinder::MatchResult& result) override {
     const DeclNode* decl = result.Nodes.getNodeAs<DeclNode>("decl");
+    assert(decl);
     // If false, there's no name to be renamed.
     if (!decl->getIdentifier())
       return;
     clang::SourceLocation decl_loc =
         TargetNodeTraits<clang::NamedDecl>::GetLoc(*decl);
     if (decl_loc.isMacroID()) {
       // Get the location of the spelling of the declaration. If token pasting
       // was used this will be in "scratch space" and we don't know how to get
       // from there back to/ the actual macro with the foo##bar text. So just
       // don't replace in that case.
       clang::SourceLocation spell =
           result.SourceManager->getSpellingLoc(decl_loc);
       if (strcmp(result.SourceManager->getBufferName(spell),
                  "<scratch space>") == 0)
         return;
     }
     clang::ASTContext* context = result.Context;
     std::string new_name;
     if (!GetNameForDecl(*decl, *context, new_name))
       return;  // If false, the name was not suitable for renaming.
     llvm::StringRef old_name = decl->getName();
-    if (old_name == new_name)
-      return;
-    clang::SourceLocation loc = TargetNodeTraits<TargetNode>::GetLoc(
-        *result.Nodes.getNodeAs<TargetNode>(
-            TargetNodeTraits<TargetNode>::GetName()));
-    clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc);
-    replacements_->emplace(*result.SourceManager, range, new_name);
-    replacement_names_.emplace(old_name.str(), std::move(new_name));
+    Base::AddReplacement(result, old_name, std::move(new_name));
   }
+};
 
-  const std::unordered_map<std::string, std::string>& replacement_names()
-      const {
-    return replacement_names_;
+using FieldDeclRewriter = DeclRewriterBase<clang::FieldDecl, clang::NamedDecl>;
+using VarDeclRewriter = DeclRewriterBase<clang::VarDecl, clang::NamedDecl>;
+using MemberRewriter = DeclRewriterBase<clang::FieldDecl, clang::MemberExpr>;
+using DeclRefRewriter = DeclRewriterBase<clang::VarDecl, clang::DeclRefExpr>;
+using FieldDeclRefRewriter =
+    DeclRewriterBase<clang::FieldDecl, clang::DeclRefExpr>;
+using FunctionDeclRewriter =
+    DeclRewriterBase<clang::FunctionDecl, clang::NamedDecl>;
+using FunctionRefRewriter =
+    DeclRewriterBase<clang::FunctionDecl, clang::DeclRefExpr>;
+using ConstructorInitializerRewriter =
+    DeclRewriterBase<clang::FieldDecl, clang::CXXCtorInitializer>;
+
+using MethodDeclRewriter =
+    DeclRewriterBase<clang::CXXMethodDecl, clang::NamedDecl>;
+using MethodRefRewriter =
+    DeclRewriterBase<clang::CXXMethodDecl, clang::DeclRefExpr>;
+using MethodMemberRewriter =
+    DeclRewriterBase<clang::CXXMethodDecl, clang::MemberExpr>;
+
+using EnumConstantDeclRewriter =
+    DeclRewriterBase<clang::EnumConstantDecl, clang::NamedDecl>;
+using EnumConstantDeclRefRewriter =
+    DeclRewriterBase<clang::EnumConstantDecl, clang::DeclRefExpr>;
+
+using UnresolvedLookupRewriter =
+    DeclRewriterBase<clang::NamedDecl, clang::UnresolvedLookupExpr>;
+using UnresolvedMemberRewriter =
+    DeclRewriterBase<clang::NamedDecl, clang::UnresolvedMemberExpr>;
+
+using UsingDeclRewriter = DeclRewriterBase<clang::UsingDecl, clang::NamedDecl>;
+
+clang::DeclarationName GetUnresolvedName(
+    const clang::UnresolvedMemberExpr& expr) {
+  return expr.getMemberName();
+}
+
+clang::DeclarationName GetUnresolvedName(
+    const clang::UnresolvedUsingValueDecl& decl) {
+  return decl.getDeclName();
+}
+
+// Returns whether |expr_node| is used as a callee in the AST (i.e. if
+// |expr_node| needs to resolve to a method or a function).
+bool IsCallee(const clang::Expr& expr, clang::ASTContext& context) {
+  auto matcher = stmt(hasParent(callExpr(callee(equalsNode(&expr)))));

[dcheng, 2016/11/28 22:05:48]

Why is it important to test this for UnresolvedMemberExpr? Or more simply, what
breaks if this simply always returns true like the UnresolvedUsingValueDecl
version?

[Łukasz Anforowicz, 2016/11/29 00:45:42]

I don't remember :-(.  I should have added tests when working on this.  I only
vaguely remember that some things were renamed that shouldn't be renamed, but I
don't remember what they were exactly.

I guess what I can do is remove IsCallee entirely from this CL (since the tests
I do have pass without it) and experiment with it to the next CL.  If something
fails without IsCallee, we should find out soon (after the next CL all of WTF
should hopefully build after running the tool).

[Łukasz Anforowicz, 2016/11/29 20:01:05]

Found it!  I've added a regression test in the follow-up CL here:
https://codereview.chromium.org/2256913002/patch/220001/230003

+  return IsMatching(matcher, expr, context);
+}
+
+// Returns whether |decl| will be used as a callee in the AST (i.e. if the value
+// brought by the using declaration will resolve to a method or a function).
+bool IsCallee(const clang::UnresolvedUsingValueDecl& /* decl */,
+              clang::ASTContext& /* context */) {
+  // Heuristic - looking only at the shape of AST, let's assume that |using
+  // Base::foo| refers to a method.  This heuristic can be refined by also
+  // looking at the name of |decl| (this is done both for
+  // UnresolvedUsingValueDecl and for clang::Expr in
+  // GuessNameForUnresolvedDependentNode method).
+  return true;
+}
+
+template <typename TargetNode>
+class UnresolvedRewriterBase : public RewriterBase<TargetNode> {
+ public:
+  using Base = RewriterBase<TargetNode>;
+
+  explicit UnresolvedRewriterBase(std::set<Replacement>* replacements)
+      : RewriterBase<TargetNode>(replacements) {}
+
+  void run(const MatchFinder::MatchResult& result) override {
+    const TargetNode& expr = Base::GetTargetNode(result);
+    llvm::StringRef old_name = GetUnresolvedName(expr).getAsString();
+    std::string new_name;
+    if (GuessNameForUnresolvedDependentNode(expr, *result.Context, old_name,
+                                            new_name)) {
+      Base::AddReplacement(result, old_name, std::move(new_name));
+    }
   }
 
  private:
-  std::set<Replacement>* const replacements_;
-  std::unordered_map<std::string, std::string> replacement_names_;
+  // This method calculates a new name for nodes that depend on template
+  // parameters (http://en.cppreference.com/w/cpp/language/dependent_name).  The
+  // renaming is based on crude heuristics, because such nodes are not bound to
+  // a specific decl until template instantiation - at the point of rename, one
+  // cannot tell whether the node will eventually resolve to a field / method /
+  // constant / etc.
+  bool GuessNameForUnresolvedDependentNode(const TargetNode& node,
+                                           clang::ASTContext& context,
+                                           llvm::StringRef old_name,
+                                           std::string& new_name) {
+    // |m_fieldName| -> |field_name_|.
+    if (old_name.startswith(kBlinkFieldPrefix)) {
+      std::string field_name = old_name.str().substr(strlen(kBlinkFieldPrefix));
+      if (field_name.find('_') == std::string::npos) {
+        new_name = CamelCaseToUnderscoreCase(field_name) + "_";
+        return true;
+      }
+    }
+
+    // |T::myMethod(...)| -> |T::MyMethod(...)|.
+    if ((old_name.find('_') == std::string::npos) && IsCallee(node, context) &&
+        !IsBlacklistedFunctionOrMethodName(old_name)) {
+      new_name = old_name;
+      new_name[0] = clang::toUppercase(new_name[0]);
+      return true;
+    }
+
+    // In the future we can consider more heuristics:
+    // - "s_" and "g_" prefixes
+    // - "ALL_CAPS"
+    // - |T::myStaticField| -> |T::kMyStaticField|
+    //   (but have to be careful not to rename |value| in WTF/TypeTraits.h?)
+    return false;
+  }
 };
 
-using FieldDeclRewriter = RewriterBase<clang::FieldDecl, clang::NamedDecl>;
-using VarDeclRewriter = RewriterBase<clang::VarDecl, clang::NamedDecl>;
-using MemberRewriter = RewriterBase<clang::FieldDecl, clang::MemberExpr>;
-using DeclRefRewriter = RewriterBase<clang::VarDecl, clang::DeclRefExpr>;
-using FieldDeclRefRewriter = RewriterBase<clang::FieldDecl, clang::DeclRefExpr>;
-using FunctionDeclRewriter =
-    RewriterBase<clang::FunctionDecl, clang::NamedDecl>;
-using FunctionRefRewriter =
-    RewriterBase<clang::FunctionDecl, clang::DeclRefExpr>;
-using ConstructorInitializerRewriter =
-    RewriterBase<clang::FieldDecl, clang::CXXCtorInitializer>;
+using UnresolvedDependentMemberRewriter =
+    UnresolvedRewriterBase<clang::UnresolvedMemberExpr>;
 
-using MethodDeclRewriter = RewriterBase<clang::CXXMethodDecl, clang::NamedDecl>;
-using MethodRefRewriter =
-    RewriterBase<clang::CXXMethodDecl, clang::DeclRefExpr>;
-using MethodMemberRewriter =
-    RewriterBase<clang::CXXMethodDecl, clang::MemberExpr>;
-
-using EnumConstantDeclRewriter =
-    RewriterBase<clang::EnumConstantDecl, clang::NamedDecl>;
-using EnumConstantDeclRefRewriter =
-    RewriterBase<clang::EnumConstantDecl, clang::DeclRefExpr>;
-
-using UnresolvedLookupRewriter =
-    RewriterBase<clang::NamedDecl, clang::UnresolvedLookupExpr>;
-using UnresolvedMemberRewriter =
-    RewriterBase<clang::NamedDecl, clang::UnresolvedMemberExpr>;
-
-using UsingDeclRewriter = RewriterBase<clang::UsingDecl, clang::NamedDecl>;
+using UnresolvedUsingValueDeclRewriter =
+    UnresolvedRewriterBase<clang::UnresolvedUsingValueDecl>;
 
 }  // namespace
 
 static llvm::cl::extrahelp common_help(CommonOptionsParser::HelpMessage);
 
 int main(int argc, const char* argv[]) {
   // TODO(dcheng): Clang tooling should do this itself.
   // http://llvm.org/bugs/show_bug.cgi?id=21627
   llvm::InitializeNativeTarget();
   llvm::InitializeNativeTargetAsmParser();
@@ skipping 251 matching lines @@
                 // UnresolvedMemberExpr matcher below are analogous to how the
                 // rewriter has both a MemberRefRewriter matcher to rewrite
                 // &T::method and a MethodMemberRewriter matcher to rewriter
                 // t.method().
                 allOverloadsMatch(anyOf(method_decl_matcher,
                                         method_template_decl_matcher))))));
   UnresolvedLookupRewriter unresolved_lookup_rewriter(&replacements);
   match_finder.addMatcher(unresolved_lookup_matcher,
                           &unresolved_lookup_rewriter);
 
-  // Unresolved member expressions ========
+  // Unresolved member expressions (for non-dependent fields / methods) ========
   // Similar to unresolved lookup expressions, but for methods in a member
   // context, e.g. var_with_templated_type.Method().
   auto unresolved_member_matcher = expr(id(
       "expr",
       unresolvedMemberExpr(
           // Similar to UnresolvedLookupExprs, all the candidate methods must be
           // Blink methods/method templates.
           allOverloadsMatch(
               anyOf(method_decl_matcher, method_template_decl_matcher)))));
   UnresolvedMemberRewriter unresolved_member_rewriter(&replacements);
   match_finder.addMatcher(unresolved_member_matcher,
                           &unresolved_member_rewriter);
 
+  // Unresolved using value decls ========
+  // Example:
+  //  template <typename T>
+  //  class BaseClass {
+  //   public:
+  //    unsigned long m_size;
+  //  };
+  //  template <typename T>
+  //  class DerivedClass : protected BaseClass<T> {
+  //   private:
+  //    using Base = BaseClass<T>;
+  //    using Base::m_size; <- matched by |unresolved_using_value_decl_matcher|.

[dcheng, 2016/11/28 22:05:48]

Super minor nit: Add a // here, or consider just moving the comments out-of-line
to simplify the wrapping + alignment.

[Łukasz Anforowicz, 2016/11/29 00:45:42]

Done.  (this also unifies the formatting of the comment wrt to the one two lines
below).

+  //    void method() {
+  //      m_size = 123;  // <- |m_size| matched by
+  //    }                //    |unresolved_dependent_using_matcher|.
+  //  };
+  auto unresolved_dependent_using_matcher =
+      expr(id("expr", unresolvedMemberExpr(allOverloadsMatch(allOf(
+                          in_blink_namespace, unresolvedUsingValueDecl())))));
+  UnresolvedDependentMemberRewriter unresolved_dependent_member_rewriter(
+      &replacements);
+  match_finder.addMatcher(unresolved_dependent_using_matcher,
+                          &unresolved_dependent_member_rewriter);
+  auto unresolved_using_value_decl_matcher =
+      decl(id("decl", unresolvedUsingValueDecl(in_blink_namespace)));
+  UnresolvedUsingValueDeclRewriter unresolved_using_value_decl_rewriter(
+      &replacements);
+  match_finder.addMatcher(unresolved_using_value_decl_matcher,
+                          &unresolved_using_value_decl_rewriter);
+
   // Using declarations ========
   // Given
   //   using blink::X;
   // matches |using blink::X|.
   auto using_decl_matcher = id(
       "decl", usingDecl(hasAnyUsingShadowDecl(hasTargetDecl(anyOf(
                   var_decl_matcher, field_decl_matcher, function_decl_matcher,
                   method_decl_matcher, function_template_decl_matcher,
                   method_template_decl_matcher, enum_member_decl_matcher)))));
   UsingDeclRewriter using_decl_rewriter(&replacements);
@@ skipping 43 matching lines @@
   for (const auto& r : replacements) {
     std::string replacement_text = r.getReplacementText().str();
     std::replace(replacement_text.begin(), replacement_text.end(), '\n', '\0');
     llvm::outs() << "r:::" << r.getFilePath() << ":::" << r.getOffset()
                  << ":::" << r.getLength() << ":::" << replacement_text << "\n";
   }
   llvm::outs() << "==== END EDITS ====\n";
 
   return 0;
 }