Handling of UnresolvedUsingValueDecl nodes.

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Index: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index 6567f77918d8e70960af1cadaeb32abaeafedc7f..85a9661d70f190f8cc0f4857067739b9de772cf6 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -175,11 +175,13 @@ bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
   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",
@@ -191,9 +193,15 @@ bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) {
 
   // 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) {
@@ -205,8 +213,8 @@ bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) {
   // 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;
@@ -217,7 +225,7 @@ AST_MATCHER(clang::FunctionDecl, isBlacklistedFunction) {
 }
 
 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
@@ -464,7 +472,6 @@ struct TargetNodeTraits<clang::NamedDecl> {
     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 <>
@@ -473,7 +480,6 @@ struct TargetNodeTraits<clang::MemberExpr> {
     return expr.getMemberLoc();
   }
   static const char* GetName() { return "expr"; }
-  static const char* GetType() { return "MemberExpr"; }
 };
 
 template <>
@@ -482,7 +488,6 @@ struct TargetNodeTraits<clang::DeclRefExpr> {
     return expr.getLocation();
   }
   static const char* GetName() { return "expr"; }
-  static const char* GetType() { return "DeclRefExpr"; }
 };
 
 template <>
@@ -492,7 +497,6 @@ struct TargetNodeTraits<clang::CXXCtorInitializer> {
     return init.getSourceLocation();
   }
   static const char* GetName() { return "initializer"; }
-  static const char* GetType() { return "CXXCtorInitializer"; }
 };
 
 template <>
@@ -501,7 +505,6 @@ struct TargetNodeTraits<clang::UnresolvedLookupExpr> {
     return expr.getNameLoc();
   }
   static const char* GetName() { return "expr"; }
-  static const char* GetType() { return "UnresolvedLookupExpr"; }
 };
 
 template <>
@@ -510,17 +513,64 @@ struct TargetNodeTraits<clang::UnresolvedMemberExpr> {
     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;
@@ -542,55 +592,129 @@ class RewriterBase : public MatchFinder::MatchCallback {
     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 UnresolvedDependentMemberRewriter =
+    UnresolvedRewriterBase<clang::UnresolvedMemberExpr>;
 
-using UsingDeclRewriter = RewriterBase<clang::UsingDecl, clang::NamedDecl>;
+using UnresolvedUsingValueDeclRewriter =
+    UnresolvedRewriterBase<clang::UnresolvedUsingValueDecl>;
+
+using UsingDeclRewriter = DeclRewriterBase<clang::UsingDecl, clang::NamedDecl>;
 
 }  // namespace
 
@@ -862,7 +986,7 @@ int main(int argc, const char* argv[]) {
   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(
@@ -876,6 +1000,36 @@ int main(int argc, const char* argv[]) {
   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;