Ignoring fields declared within implicit function specializations.

Prior to this CL, field decls within implicit *class* template
specializations have been ignored, to avoid generating conflicting
replacements (e.g. can't rewrite |T*| both to |CheckedPtr<T>| and to
|CheckedPtr<SomeClass>| just because there exists an implicit
specialization where T=SomeClass).

It turns out that function specializations need a similar treatment
(otherwise, a conflicting edit/replacement may get generated).

Bug: 1069567
Change-Id: I18a004f6268ecc926f5b61311fd3a13a88c546e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2223716
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776996}

tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp

@@ -334,10 +334,58 @@ AST_MATCHER(clang::Decl, isInExternCContext) {
   return Node.getLexicalDeclContext()->isExternCContext();
 }
 
-AST_MATCHER(clang::ClassTemplateSpecializationDecl, isImplicitSpecialization) {
+// Given:
+//   template <typename T, typename T2> class MyTemplate {};  // Node1 and Node4
+//   template <typename T2> class MyTemplate<int, T2> {};     // Node2
+//   template <> class MyTemplate<int, char> {};              // Node3
+//   void foo() {
+//     // This creates implicit template specialization (Node4) out of the
+//     // explicit template definition (Node1).
+//     MyTemplate<bool, double> v;
+//   }
+// with the following AST nodes:
+//   ClassTemplateDecl MyTemplate                                       - Node1
+//   | |-CXXRecordDecl class MyTemplate definition
+//   | `-ClassTemplateSpecializationDecl class MyTemplate definition    - Node4
+//   ClassTemplatePartialSpecializationDecl class MyTemplate definition - Node2
+//   ClassTemplateSpecializationDecl class MyTemplate definition        - Node3
+//
+// Matches AST node 4, but not AST node2 nor node3.
+AST_MATCHER(clang::ClassTemplateSpecializationDecl,
+            isImplicitClassTemplateSpecialization) {
   return !Node.isExplicitSpecialization();
 }
 
+// Given:
+//   template <typename T, typename T2> void foo(T t, T2 t2) {};  // N1 and N4
+//   template <typename T2> void foo<int, T2>(int t, T2 t) {};    // N2
+//   template <> void foo<int, char>(int t, char t2) {};          // N3
+//   void foo() {
+//     // This creates implicit template specialization (N4) out of the
+//     // explicit template definition (N1).
+//     foo<bool, double>(true, 1.23);
+//   }
+// with the following AST nodes:
+//   FunctionTemplateDecl foo
+//   |-FunctionDecl 0x191da68 foo 'void (T, T2)'         // N1
+//   `-FunctionDecl 0x194bf08 foo 'void (bool, double)'  // N4
+//   FunctionTemplateDecl foo
+//   `-FunctionDecl foo 'void (int, T2)'                 // N2
+//   FunctionDecl foo 'void (int, char)'                 // N3
+//
+// Matches AST node N4, but not AST nodes N1, N2 nor N3.
+AST_MATCHER(clang::FunctionDecl, isImplicitFunctionTemplateSpecialization) {
+  switch (Node.getTemplateSpecializationKind()) {
+    case clang::TSK_ImplicitInstantiation:
+      return true;
+    case clang::TSK_Undeclared:
+    case clang::TSK_ExplicitSpecialization:
+    case clang::TSK_ExplicitInstantiationDeclaration:
+    case clang::TSK_ExplicitInstantiationDefinition:
+      return false;
+  }
+}
+
 AST_MATCHER(clang::Type, anyCharType) {
   return Node.isAnyCharacterType();
 }
@@ -595,13 +643,17 @@ int main(int argc, const char* argv[]) {
   // Matches field declarations that do not explicitly appear in the source
   // code:
   // 1. fields of classes generated by the compiler to back capturing lambdas,
-  // 2. fields within an implicit class template specialization (e.g. when a
-  //    template is instantiated by a bit of code and there's no explicit
-  //    specialization for it).
+  // 2. fields within an implicit class or function template specialization
+  //    (e.g. when a template is instantiated by a bit of code and there's no
+  //    explicit specialization for it).
+  auto implicit_class_specialization_matcher =
+      classTemplateSpecializationDecl(isImplicitClassTemplateSpecialization());
+  auto implicit_function_specialization_matcher =
+      functionDecl(isImplicitFunctionTemplateSpecialization());
   auto implicit_field_decl_matcher = fieldDecl(hasParent(cxxRecordDecl(anyOf(
-      isLambda(), classTemplateSpecializationDecl(isImplicitSpecialization()),
-      hasAncestor(
-          classTemplateSpecializationDecl(isImplicitSpecialization()))))));
+      isLambda(), implicit_class_specialization_matcher,
+      hasAncestor(decl(anyOf(implicit_class_specialization_matcher,
+                             implicit_function_specialization_matcher)))))));
 
   // Field declarations =========
   // Given

tools/clang/rewrite_raw_ptr_fields/tests/lambdas-expected.cc

@@ -2,15 +2,30 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/memory/checked_ptr.h"
+
 class MyClass {
   // Lambdas are backed by a class that may have (depending on what the lambda
   // captures) pointer fields.  The rewriter should ignore such fields (since
   // they don't have an equivalent in source code).
-  //
-  // No rewrite expected anywhere below.
   void foo() {
     int x = 123;
+
+    // No rewrite expected in the two lamdas below.
     auto lambda1 = [this]() -> int { return 123; };
     auto lambda2 = [&]() -> int { return x; };
+
+    // Nested structs defined within a lambda should be rewritten.  This test
+    // helps ensure that |implicit_field_decl_matcher| uses |hasParent| to match
+    // |isLambda|, rather than |hasAncestor|.
+    auto lambda = [&]() -> int {
+      struct NestedStruct {
+        // Expected rewrite: CheckedPtr<int> ptr_field;
+        CheckedPtr<int> ptr_field;
+      } var;
+      var.ptr_field = &x;
+
+      return x;
+    };
   }
 };

tools/clang/rewrite_raw_ptr_fields/tests/lambdas-original.cc

@@ -6,11 +6,24 @@ class MyClass {
   // Lambdas are backed by a class that may have (depending on what the lambda
   // captures) pointer fields.  The rewriter should ignore such fields (since
   // they don't have an equivalent in source code).
-  //
-  // No rewrite expected anywhere below.
   void foo() {
     int x = 123;
+
+    // No rewrite expected in the two lamdas below.
     auto lambda1 = [this]() -> int { return 123; };
     auto lambda2 = [&]() -> int { return x; };
+
+    // Nested structs defined within a lambda should be rewritten.  This test
+    // helps ensure that |implicit_field_decl_matcher| uses |hasParent| to match
+    // |isLambda|, rather than |hasAncestor|.
+    auto lambda = [&]() -> int {
+      struct NestedStruct {
+        // Expected rewrite: CheckedPtr<int> ptr_field;
+        int* ptr_field;
+      } var;
+      var.ptr_field = &x;
+
+      return x;
+    };
   }
 };

tools/clang/rewrite_raw_ptr_fields/tests/template-specializations-expected.cc

@@ -153,3 +153,32 @@ void foo2() {
 }
 
 }  // namespace nested_iterator_test
+
+// Example based on base/trace_event/memory_usage_estimator.h where a function
+// template |EstimateMemoryUsage| had a nested struct |SharedPointer| definition
+// with a pointer field |value| that was leading to conflicting replacements.
+namespace template_function {
+
+template <typename T>
+void foo(T* arg) {
+  struct NestedStruct {
+    // Expected rewrite: CheckedPtr<T> ptr_field;
+    CheckedPtr<T> ptr_field;
+
+    // Expected rewrite: CheckedPtr<MyTemplate<T, T>> ptr_field2;
+    CheckedPtr<MyTemplate<T, T>> ptr_field2;
+  } var;
+
+  var.ptr_field = nullptr;
+}
+
+void bar() {
+  // Triggering implicit specializations of foo that in the past led the
+  // rewriter to generate conflicting replacements.
+  int i = 123;
+  SomeClass* p = nullptr;
+  foo(p);
+  foo(&i);
+}
+
+}  // namespace template_function

tools/clang/rewrite_raw_ptr_fields/tests/template-specializations-original.cc

@@ -151,3 +151,32 @@ void foo2() {
 }
 
 }  // namespace nested_iterator_test
+
+// Example based on base/trace_event/memory_usage_estimator.h where a function
+// template |EstimateMemoryUsage| had a nested struct |SharedPointer| definition
+// with a pointer field |value| that was leading to conflicting replacements.
+namespace template_function {
+
+template <typename T>
+void foo(T* arg) {
+  struct NestedStruct {
+    // Expected rewrite: CheckedPtr<T> ptr_field;
+    T* ptr_field;
+
+    // Expected rewrite: CheckedPtr<MyTemplate<T, T>> ptr_field2;
+    MyTemplate<T, T>* ptr_field2;
+  } var;
+
+  var.ptr_field = nullptr;
+}
+
+void bar() {
+  // Triggering implicit specializations of foo that in the past led the
+  // rewriter to generate conflicting replacements.
+  int i = 123;
+  SomeClass* p = nullptr;
+  foo(p);
+  foo(&i);
+}
+
+}  // namespace template_function