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 157 matching lines @@
       return true;
   }
   return false;
 }
 
 bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
   // swap() functions should match the signature of std::swap for ADL tricks.
   return decl.getName() == "swap";
 }
 
-bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) {
-  if (decl.isStatic())
+bool IsBlacklistedMethod(const std::string& name,

[dcheng, 2016/09/10 04:00:55]

Pass as llvm::StringRef here, so we don't need to coerce to string.

[Łukasz Anforowicz, 2016/09/12 19:52:56]

Done.

This is how I started actually, but then got confused because it turns out that
llvm::StringRef is not a drop-in replacement for std::string (i.e. some
llvm::StringRef methods mutate their argument when the equivalent std::string
methods wouldn't;  I think this might be substr, but I forgot).  OTOH, I see
that it works fine now.

+                         const clang::CXXMethodDecl* decl) {
+  if (decl && decl->isStatic())

[dcheng, 2016/09/10 04:00:55]

It feels like we should just have two versions of this: one for CXXMethodDecl
and one for UnresolvedUsingValueDecl. We can pull out some of the common checks,
but some of the checks here will simply always be skipped for
UnresolvedUsingValueDecl.

[Łukasz Anforowicz, 2016/09/12 19:52:56]

In the latest patchset I have 1) reverted changes to IsBlacklistedMethod (so
that it again always works against CXXMethodDecl) and 2) introduced a separate
IsBlacklistedFunctionOrMethodName.  I've thought a little bit on how to avoid
duplication, but everything I thought about seems too heavy so I didn't
deduplicate and I think this is okay.  The best deduplication I could think of
was to introduce GetSetOfBlacklistedIteratorNames,
GetSetOfBlacklistedFunctionNames, GetSetOfUnconditionallyBlacklistedMethodNames,
etc. (with the set(s) being constructed lazily in the functions + reused from
both IsBlacklistedMethod and from IsBlacklistedFunctionOrMethodName).

     return false;
 
-  clang::StringRef name = decl.getName();
+  if (decl)
+    assert(decl->getName().str() == name);
 
   // These methods should never be renamed.
   static const char* kBlacklistMethods[] = {"trace", "traceImpl", "lock",
                                             "unlock", "try_lock"};
   for (const auto& b : kBlacklistMethods) {
     if (name == b)
       return true;
   }
 
   // Iterator methods shouldn't be renamed to work with stl and range-for
   // loops.
-  std::string ret_type = decl.getReturnType().getAsString();
-  if (ret_type.find("iterator") != std::string::npos ||
-      ret_type.find("Iterator") != std::string::npos) {
+  bool is_iterator = false;
+  if (decl) {
+    std::string ret_type = decl->getReturnType().getAsString();
+    is_iterator = ret_type.find("iterator") != std::string::npos ||
+                  ret_type.find("Iterator") != std::string::npos;

[dcheng, 2016/09/10 04:00:55]

For this, we can just have a simple shared method to determine if the thing is
in an iterator, with the return type checks only in the CXXMethodDecl version.

[Łukasz Anforowicz, 2016/09/12 19:52:56]

I think this comment is not applicable to the most recent patchset.

+  } else {
+    is_iterator = true;
+  }
+  if (is_iterator) {
     static const char* kIteratorBlacklist[] = {"begin", "end", "rbegin",
                                                "rend"};
     for (const auto& b : kIteratorBlacklist) {
       if (name == b)
         return true;
     }
   }
 
   // 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"))
+  if (name == "disable" &&
+      (!decl || IsMethodOverrideOf(*decl, "blink::InspectorAgent")))
     return true;
 
   return false;
 }
 
 AST_MATCHER(clang::FunctionDecl, isBlacklistedFunction) {
   return IsBlacklistedFunction(Node);
 }
 
 AST_MATCHER(clang::CXXMethodDecl, isBlacklistedMethod) {
-  return IsBlacklistedMethod(Node);
+  return IsBlacklistedMethod(Node.getName().str(), &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.
 std::string CamelCaseToUnderscoreCase(StringRef input) {
   std::string output;
   bool needs_underscore = false;
   bool was_lowercase = false;
   bool was_uppercase = false;
   bool first_char = true;
@@ skipping 226 matching lines @@
 
 template <typename Type>
 struct TargetNodeTraits;
 
 template <>
 struct TargetNodeTraits<clang::NamedDecl> {
   static clang::SourceLocation GetLoc(const clang::NamedDecl& decl) {
     return decl.getLocation();
   }
   static const char* GetName() { return "decl"; }
-  static const char* GetType() { return "NamedDecl"; }

[dcheng, 2016/09/10 04:00:55]

While unused, these were helpful for debugging in the past. Let's leave them?

[Łukasz Anforowicz, 2016/09/12 19:52:56]

Done.

 };
 
 template <>
 struct TargetNodeTraits<clang::MemberExpr> {
   static clang::SourceLocation GetLoc(const clang::MemberExpr& expr) {
     return expr.getMemberLoc();
   }
   static const char* GetName() { return "expr"; }
-  static const char* GetType() { return "MemberExpr"; }
 };
 
 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::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 <>
 struct TargetNodeTraits<clang::UnresolvedLookupExpr> {
   static clang::SourceLocation GetLoc(const clang::UnresolvedLookupExpr& expr) {
     return expr.getNameLoc();
   }
   static const char* GetName() { return "expr"; }
-  static const char* GetType() { return "UnresolvedLookupExpr"; }
 };
 
 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"; }
+};
+
+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));
   }
+};

[dcheng, 2016/09/10 04:00:55]

Nit: let's move the DeclRewriterBase using decls up here to group things and
give them a logical divider.

[Łukasz Anforowicz, 2016/09/12 19:52:56]

Done.

 
-  const std::unordered_map<std::string, std::string>& replacement_names()
-      const {
-    return replacement_names_;
+clang::DeclarationName GetUnresolvedName(
+    const clang::UnresolvedMemberExpr& expr) {
+  return expr.getMemberName();
+}
+
+clang::DeclarationName GetUnresolvedName(
+    const clang::UnresolvedUsingValueDecl& decl) {
+  return decl.getDeclName();
+}
+
+// Returns whether |expr| is a callee of a method or function call.
+bool IsCallee(const clang::Expr& expr, clang::ASTContext& context) {

[dcheng, 2016/09/10 04:00:55]

Can this logic be encapsulated into a matcher?

[Łukasz Anforowicz, 2016/09/12 19:52:56]

Done.  Thanks for helping figure out that I need |callExpr| in there :-)

+  auto parents = context.getParents(expr);
+  return std::all_of(
+      parents.begin(), parents.end(),
+      [&expr](const clang::ast_type_traits::DynTypedNode& parent) {
+        const clang::CallExpr* call = parent.get<clang::CallExpr>();
+        return call && call->getCallee() == &expr;
+      });
+}
+
+bool IsCallee(const clang::UnresolvedUsingValueDecl& /* decl */,
+              clang::ASTContext& /* context */) {
+  // Heuristic - assuming that |using Base::foo| refers to a method.

[dcheng, 2016/09/10 04:00:55]

We can guess from the name though, right?

[Łukasz Anforowicz, 2016/09/12 19:52:56]

Yes, we can guess from the name - we do it in
GuessNameForUnresolvedDependentNode and we use IsCallee as a secondary
verification that we are really dealing with a function/method.

In theory I could duplicate name checks from GuessNameForUnresolvedDependentNode
into IsCallee, but I am not sure if this would be desirable.

FWIW, I've added a comment saying that name checks will be done elsewhere.

+  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,
+                                           const std::string& old_name,

[dcheng, 2016/09/10 04:00:56]

Pass this as a llvm::StringRef.

[Łukasz Anforowicz, 2016/09/12 19:52:56]

Done.

+                                           std::string& new_name) {
+    // |m_fieldName| -> |field_name_|.
+    if (old_name.substr(0, strlen(kBlinkFieldPrefix)) == kBlinkFieldPrefix) {

[dcheng, 2016/09/10 04:00:55]

Then this can just use llvm::StringRef::startswith

[Łukasz Anforowicz, 2016/09/12 19:52:56]

Done.

+      std::string field_name = old_name.substr(strlen(kBlinkFieldPrefix));

[Łukasz Anforowicz, 2016/09/12 19:52:56]

This is the reason why in the end I've used std::string instead of
llvm::StringRef.  If |old_name| is a llvm::StringRef, then I get results which I
don't understand:

--- tools/clang/rewrite_to_chrome_style/tests/template-expected.cc
+++ tools/clang/rewrite_to_chrome_style/tests/template-actual.cc
@@ -132,8 +132,8 @@
  private:
   using Base = BaseClass<T>;
   // https://crbug.com/640016: Need to rewrite |m_size| into |size_|.
-  using Base::size_;
-  void Method() { size_ = 123; }
+  using Base::zeze_;
+  void Method() { zeze_ = 123; }
 };
 
 }  // namespace test_inherited_field

if |old_name| is llvm::StringRef and the code says:

      std::string field_name = old_name.substr(strlen(kBlinkFieldPrefix));

instead of

      std::string field_name = old_name.str().substr(strlen(kBlinkFieldPrefix));


Things work in the latest patchset, but the |.str()| is a bit of an eye-sore to
me... :-/

+      if (field_name.find('_') == std::string::npos) {
+        new_name = CamelCaseToUnderscoreCase(field_name) + "_";
+        return true;
+      }
+    }
+
+    if ((old_name.find('_') == std::string::npos) && IsCallee(node, context) &&
+        !IsBlacklistedMethod(old_name, nullptr)) {
+      // |T::myMethod(...)| -> |T::MyMethod(...)|.
+      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 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 =
-    RewriterBase<clang::FunctionDecl, clang::NamedDecl>;
+    DeclRewriterBase<clang::FunctionDecl, clang::NamedDecl>;
 using FunctionRefRewriter =
-    RewriterBase<clang::FunctionDecl, clang::DeclRefExpr>;
+    DeclRewriterBase<clang::FunctionDecl, clang::DeclRefExpr>;
 using ConstructorInitializerRewriter =
-    RewriterBase<clang::FieldDecl, clang::CXXCtorInitializer>;
+    DeclRewriterBase<clang::FieldDecl, clang::CXXCtorInitializer>;
 
-using MethodDeclRewriter = RewriterBase<clang::CXXMethodDecl, clang::NamedDecl>;
+using MethodDeclRewriter =
+    DeclRewriterBase<clang::CXXMethodDecl, clang::NamedDecl>;
 using MethodRefRewriter =
-    RewriterBase<clang::CXXMethodDecl, clang::DeclRefExpr>;
+    DeclRewriterBase<clang::CXXMethodDecl, clang::DeclRefExpr>;
 using MethodMemberRewriter =
-    RewriterBase<clang::CXXMethodDecl, clang::MemberExpr>;
+    DeclRewriterBase<clang::CXXMethodDecl, clang::MemberExpr>;
 
 using EnumConstantDeclRewriter =
-    RewriterBase<clang::EnumConstantDecl, clang::NamedDecl>;
+    DeclRewriterBase<clang::EnumConstantDecl, clang::NamedDecl>;
 using EnumConstantDeclRefRewriter =
-    RewriterBase<clang::EnumConstantDecl, clang::DeclRefExpr>;
+    DeclRewriterBase<clang::EnumConstantDecl, clang::DeclRefExpr>;
 
 using UnresolvedLookupRewriter =
-    RewriterBase<clang::NamedDecl, clang::UnresolvedLookupExpr>;
+    DeclRewriterBase<clang::NamedDecl, clang::UnresolvedLookupExpr>;
 using UnresolvedMemberRewriter =
-    RewriterBase<clang::NamedDecl, clang::UnresolvedMemberExpr>;
+    DeclRewriterBase<clang::NamedDecl, clang::UnresolvedMemberExpr>;
 
-using UsingDeclRewriter = RewriterBase<clang::UsingDecl, clang::NamedDecl>;
+using UnresolvedDependentMemberRewriter =
+    UnresolvedRewriterBase<clang::UnresolvedMemberExpr>;
+
+using UnresolvedUsingValueDeclRewriter =
+    UnresolvedRewriterBase<clang::UnresolvedUsingValueDecl>;
+
+using UsingDeclRewriter = DeclRewriterBase<clang::UsingDecl, clang::NamedDecl>;
 
 }  // 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|.
+  //    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;
 }