Handling of DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr 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 44 matching lines @@
 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;
 
+const clang::ast_matchers::internal::
+    VariadicDynCastAllOfMatcher<clang::Expr, clang::DependentScopeDeclRefExpr>
+        dependentScopeDeclRefExpr;
+
+const clang::ast_matchers::internal::
+    VariadicDynCastAllOfMatcher<clang::Expr, clang::CXXDependentScopeMemberExpr>
+        cxxDependentScopeMemberExpr;
+
 AST_MATCHER(clang::FunctionDecl, isOverloadedOperator) {
   return Node.isOverloadedOperator();
 }
 
 AST_MATCHER(clang::CXXMethodDecl, isInstanceMethod) {
   return Node.isInstance();
 }
 
 AST_MATCHER_P(clang::FunctionTemplateDecl,
               templatedDecl,
@@ skipping 83 matching lines @@
   return inner_matcher.matches(decl, finder, builder);
 }
 
 AST_MATCHER_P(clang::CXXMethodDecl,
               includeAllOverriddenMethods,
               clang::ast_matchers::internal::Matcher<clang::CXXMethodDecl>,
               InnerMatcher) {
   return MatchAllOverriddenMethods(Node, InnerMatcher, Finder, Builder);
 }
 
+// Matches |x.m| CXXDependentScopeMemberExpr if InnerMatcher matches |x|.
+AST_MATCHER_P(clang::CXXDependentScopeMemberExpr,
+              hasBaseExprMissingOrMatching,
+              clang::ast_matchers::internal::Matcher<clang::Expr>,
+              InnerMatcher) {
+  clang::Expr* base_expr = Node.isImplicitAccess() ? nullptr : Node.getBase();
+  return !base_expr || InnerMatcher.matches(*base_expr, Finder, Builder);

[dcheng, 2016/11/30 05:55:52]

What's an implicit access look like? Why is it OK to always return a match in
that case?

[Łukasz Anforowicz, 2016/11/30 23:26:53]

1. You are right that we should not return a match if Node.isImplicitAccess() is
true.


2. Consider the following case, which illustrates why combining hasBaseExpr...
and hasQualifier... via *anyOf* is undesirable:

    blinkObject->NonBlinkBaseClass::virtualMethod()  

In this case we *cannot* rename even though hasBaseExpr(blinkType(...)) matches
here, because the call is to a method from outside of blink - this will be
rejected because NonBlinkBaseClass won't match via
hasQualifierMissingOrMatching.


3. I hope I made things better by combining the 2 matchers into a single
matcher.  The new matcher is also more correct as well (the old code would
refuse to rename nonBlinkObject->BlinkClass::method()).  I've added a regression
test for this.

[dcheng, 2016/12/01 07:15:06]

I see. I think the usual preference is to have smaller, more composable matchers
(we could still compose with anyOf, we just have to be insert an additional
unless() matcher to fix #2), but I can see the argument either way here.

[Łukasz Anforowicz, 2016/12/01 18:26:56]

Acknowledged.

+}
+
+// Matches |T::m| CXXDependentScopeMemberExpr if InnerMatcher matches |T|.
+AST_MATCHER_P(
+    clang::CXXDependentScopeMemberExpr,
+    hasQualifierMissingOrMatching,
+    clang::ast_matchers::internal::Matcher<clang::NestedNameSpecifier>,
+    InnerMatcher) {
+  clang::NestedNameSpecifier* qual = Node.getQualifier();
+  return !qual || InnerMatcher.matches(*qual, Finder, Builder);

[dcheng, 2016/11/30 05:55:52]

Similar question here: why is it OK to return a match if there is no qualifier?

Shouldn't the matcher expression that composes these just use anyOf() to select
as appropriate?

[Łukasz Anforowicz, 2016/11/30 23:26:53]

See above.

+}
+
+// Matches |const Class<T>&| QualType if InnerMatcher matches |Class<T>|.
+AST_MATCHER_P(clang::QualType,
+              hasBaseType,
+              clang::ast_matchers::internal::Matcher<clang::Type>,
+              InnerMatcher) {
+  const clang::Type* type = Node.getTypePtrOrNull();
+  return type && InnerMatcher.matches(*type, Finder, Builder);
+}
+
 bool IsMethodOverrideOf(const clang::CXXMethodDecl& decl,
                         const char* class_name) {
   if (decl.getParent()->getQualifiedNameAsString() == class_name)
     return true;
   for (auto it = decl.begin_overridden_methods();
        it != decl.end_overridden_methods(); ++it) {
     if (IsMethodOverrideOf(**it, class_name))
       return true;
   }
   return false;
@@ skipping 346 matching lines @@
 template <>
 struct TargetNodeTraits<clang::DeclRefExpr> {
   static clang::SourceLocation GetLoc(const clang::DeclRefExpr& expr) {
     return expr.getLocation();
   }
   static const char* GetName() { return "expr"; }
   static const char* GetType() { return "DeclRefExpr"; }
 };
 
 template <>
+struct TargetNodeTraits<clang::DependentScopeDeclRefExpr> {
+  static clang::SourceLocation GetLoc(
+      const clang::DependentScopeDeclRefExpr& expr) {
+    return expr.getLocation();
+  }
+  static const char* GetName() { return "expr"; }
+};
+
+template <>
+struct TargetNodeTraits<clang::CXXDependentScopeMemberExpr> {
+  static clang::SourceLocation GetLoc(
+      const clang::CXXDependentScopeMemberExpr& expr) {
+    return expr.getMemberLoc();
+  }
+  static const char* GetName() { return "expr"; }
+};
+
+template <>
 struct TargetNodeTraits<clang::CXXCtorInitializer> {
   static clang::SourceLocation GetLoc(const clang::CXXCtorInitializer& init) {
     assert(init.isWritten());
     return init.getSourceLocation();
   }
   static const char* GetName() { return "initializer"; }
   static const char* GetType() { return "CXXCtorInitializer"; }
 };
 
 template <>
@@ skipping 126 matching lines @@
     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::DependentScopeDeclRefExpr& expr) {
+  return expr.getDeclName();
+}
+
+clang::DeclarationName GetUnresolvedName(
+    const clang::CXXDependentScopeMemberExpr& expr) {
+  return expr.getMember();
+}
+
+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)))));
+  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)) {
+
+    clang::DeclarationName unresolved_name = GetUnresolvedName(expr);
+    switch (unresolved_name.getNameKind()) {
+      // Do not rewrite this:
+      //   return operator T*();
+      // into this:
+      //   return Operator type - parameter - 0 - 0 * T * ();
+      case clang::DeclarationName::NameKind::CXXConversionFunctionName:
+      case clang::DeclarationName::NameKind::CXXOperatorName:
+      case clang::DeclarationName::NameKind::CXXLiteralOperatorName:
+        return;
+      default:
+        break;
+    }
+
+    std::string old_name = unresolved_name.getAsString();
+    std::string new_name = GuessNameForUnresolvedDependentNode(
+        expr, *result.Context, old_name);
+    if (!new_name.empty())
       Base::AddReplacement(result, old_name, std::move(new_name));
-    }
   }
 
  private:
   // 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,
+  //
+  // The guessed new name is returned (or an empty string if no name was
+  // guessed).
+  std::string GuessNameForUnresolvedDependentNode(const TargetNode& node,

[dcheng, 2016/11/30 05:55:52]

Can we keep this consistent with GetNameForDecl()?

[Łukasz Anforowicz, 2016/11/30 23:26:53]

Done (I think).

There is some duplication for calculating the |old_name| both in |run| and in
GuessNameForUnresolvedDependentNode methods, but I guess this is okay (and I
think I like how GuessNameForUnresolvedDependentNode doesn't need to take
|old_name| as a parameter).

[dcheng, 2016/12/01 07:15:06]

I think I'm OK with passing old_name in; just having two different return styles
(one returns false, one returns empty string) is a bit confusing.

[Łukasz Anforowicz, 2016/12/01 18:26:57]

Done.

                                            clang::ASTContext& context,
-                                           llvm::StringRef old_name,
-                                           std::string& new_name) {
+                                           llvm::StringRef old_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;
-      }
+      std::string field_name = old_name.substr(strlen(kBlinkFieldPrefix));
+      if (field_name.find('_') == std::string::npos)
+        return CamelCaseToUnderscoreCase(field_name) + "_";
     }
 
     // |T::myMethod(...)| -> |T::MyMethod(...)|.
-    if ((old_name.find('_') == std::string::npos) &&
+    if ((old_name.find('_') == std::string::npos) && IsCallee(node, context) &&
         !IsBlacklistedFunctionOrMethodName(old_name)) {
-      new_name = old_name;
+      std::string new_name = old_name;
       new_name[0] = clang::toUppercase(new_name[0]);
-      return true;
+      return new_name;
     }
 
     // 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;
+    return std::string();
   }
 };
 
 using UnresolvedDependentMemberRewriter =
     UnresolvedRewriterBase<clang::UnresolvedMemberExpr>;
 
 using UnresolvedUsingValueDeclRewriter =
     UnresolvedRewriterBase<clang::UnresolvedUsingValueDecl>;
 
+using DependentScopeDeclRefExprRewriter =
+    UnresolvedRewriterBase<clang::DependentScopeDeclRefExpr>;
+
+using CXXDependentScopeMemberExprRewriter =
+    UnresolvedRewriterBase<clang::CXXDependentScopeMemberExpr>;
+
 }  // 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();
   llvm::cl::OptionCategory category(
@@ skipping 306 matching lines @@
   //   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);
   match_finder.addMatcher(using_decl_matcher, &using_decl_rewriter);
 
+  // Matches any QualType that refers to a blink type:
+  // - const blink::Foo&
+  // - blink::Foo*
+  // - blink::Foo<T>
+  // - ...
+  // TODO(lukasza): The matchers below can be simplified after
+  // https://llvm.org/bugs/show_bug.cgi?id=30331 is fixed.
+  // Simplified matchers:
+  //    auto blink_qual_type_base_matcher =
+  //        qualType(hasDeclaration(in_blink_namespace));
+  //    auto blink_qual_type_matcher = qualType(anyOf(
+  //        blink_qual_type_base_matcher,
+  //        pointsTo(blink_qual_type_base_matcher),
+  //        references(blink_qual_type_base_matcher)));
+  auto blink_qual_type_bug_workaround_matcher1 = hasBaseType(
+      anyOf(enumType(hasDeclaration(in_blink_namespace)),
+            recordType(hasDeclaration(in_blink_namespace)),
+            templateSpecializationType(hasDeclaration(in_blink_namespace)),
+            templateTypeParmType(hasDeclaration(in_blink_namespace)),
+            typedefType(hasDeclaration(in_blink_namespace))));
+  auto blink_qual_type_base_matcher =
+      qualType(anyOf(blink_qual_type_bug_workaround_matcher1,
+                     hasBaseType(elaboratedType(
+                         namesType(blink_qual_type_bug_workaround_matcher1)))));
+  auto blink_qual_type_matcher =
+      qualType(anyOf(blink_qual_type_base_matcher, pointsTo(in_blink_namespace),
+                     references(in_blink_namespace)));
+
+  // Template-dependent decl lookup ========
+  // Given
+  //   template <typename T> void f() { T::foo(); }
+  // matches |T::foo|.
+  auto dependent_scope_decl_ref_expr_matcher =
+      expr(id("expr", dependentScopeDeclRefExpr(has(nestedNameSpecifier(
+                          specifiesType(blink_qual_type_matcher))))));
+  DependentScopeDeclRefExprRewriter dependent_scope_decl_ref_expr_rewriter(
+      &replacements);
+  match_finder.addMatcher(dependent_scope_decl_ref_expr_matcher,
+                          &dependent_scope_decl_ref_expr_rewriter);
+
+  // Template-dependent member lookup ========
+  // Given
+  //   template <typename T>
+  //   class Foo {
+  //     void f() { T::foo(); }
+  //     void g(T x) { x.bar(); }
+  //   };
+  // matches |T::foo| and |x.bar|.
+  auto cxx_dependent_scope_member_expr_matcher = expr(
+      id("expr",
+         cxxDependentScopeMemberExpr(allOf(
+             hasBaseExprMissingOrMatching(hasType(blink_qual_type_matcher)),
+             hasQualifierMissingOrMatching(
+                 specifiesType(blink_qual_type_matcher))))));
+  CXXDependentScopeMemberExprRewriter cxx_dependent_scope_member_expr_rewriter(
+      &replacements);
+  match_finder.addMatcher(cxx_dependent_scope_member_expr_matcher,
+                          &cxx_dependent_scope_member_expr_rewriter);
+
   std::unique_ptr<clang::tooling::FrontendActionFactory> factory =
       clang::tooling::newFrontendActionFactory(&match_finder);
   int result = tool.run(factory.get());
   if (result != 0)
     return result;
 
 #if defined(_WIN32)
   HANDLE lockfd = CreateFile("rewrite-sym.lock", GENERIC_READ, FILE_SHARE_READ,
                              NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
   OVERLAPPED overlapped = {};
@@ skipping 31 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;
 }