Ensure that |SomeTemplate<SomeClass>* ptr_field| gets rewritten.

It turns out that before this CL |SomeTemplate<SomeClass>* ptr_field|
would match the |non_free_standing_tag_type| matcher and therefore would
not get rewritten. :-(

Both the old |non_free_standing_tag_type| matcher, and the old
|hasUniqueTypeLoc| matcher were trying to prevent generating conflicting
replacements (i.e. separate edits of overlapping sections of a source
file).  This CL fixes this problem in a more direct way - by introducing
|overlapsOtherDeclsWithinRecordDecl| matcher for FieldDecls.  The new
matcher replaces both of the old matchers.

The new matcher is a more generic version of the old, removed
|hasUniqueTypeLoc| matcher.  The new matcher:
1) considers all Decls under a RecordDecl (not just FieldDecls)
2) looks for any SourceRange overlap (rather than just considering
   the starting SourceLocation of a field type)

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

tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp

@@ -22,6 +22,7 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersMacros.h"
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
@@ -216,11 +217,6 @@ class OutputHelper : public clang::tooling::SourceFileCallbacks {
   clang::Language current_language_ = clang::Language::Unknown;
 };
 
-AST_MATCHER(clang::TagDecl, isNotFreeStandingTagDecl) {
-  const clang::TagDecl* tag_decl = Node.getCanonicalDecl();
-  return !tag_decl->isFreeStanding();
-}
-
 llvm::StringRef GetFilePath(const clang::SourceManager& source_manager,
                             const clang::FieldDecl& field_decl) {
   clang::SourceLocation loc = field_decl.getSourceRange().getBegin();
@@ -438,34 +434,86 @@ AST_MATCHER_P(clang::FieldDecl,
   return InnerMatcher.matches(*explicit_field_decl, Finder, Builder);
 }
 
-// Matcher for FieldDecl that has a TypeLoc with a unique start location
-// (i.e. has a TypeLoc that is not shared with any other FieldDecl).
+// Returns |true| if and only if:
+// 1. |a| and |b| are in the same file (e.g. |false| is returned if any location
+//    is within macro scratch space or a similar location;  similarly |false| is
+//    returned if |a| and |b| are in different files).
+// 2. |a| and |b| overlap.
+bool IsOverlapping(const clang::SourceManager& source_manager,
+                   const clang::SourceRange& a,
+                   const clang::SourceRange& b) {
+  clang::FullSourceLoc a1(a.getBegin(), source_manager);
+  clang::FullSourceLoc a2(a.getEnd(), source_manager);
+  clang::FullSourceLoc b1(b.getBegin(), source_manager);
+  clang::FullSourceLoc b2(b.getEnd(), source_manager);
+
+  // Are all locations in a file?
+  if (!a1.isFileID() || !a2.isFileID() || !b1.isFileID() || !b2.isFileID())
+    return false;
+
+  // Are all locations in the same file?
+  if (a1.getFileID() != a2.getFileID() || a2.getFileID() != b1.getFileID() ||
+      b1.getFileID() != b2.getFileID()) {
+    return false;
+  }
+
+  // Check the 2 cases below:
+  // 1. A: |============|
+  //    B:      |===============|
+  //       a1   b1      a2      b2
+  // or
+  // 2. A: |====================|
+  //    B:      |=======|
+  //       a1   b1      b2      a2
+  bool b1_is_inside_a_range = a1.getFileOffset() <= b1.getFileOffset() &&
+                              b1.getFileOffset() <= a2.getFileOffset();
+
+  // Check the 2 cases below:
+  // 1. B: |============|
+  //    A:      |===============|
+  //       b1   a1      b2      a2
+  // or
+  // 2. B: |====================|
+  //    A:      |=======|
+  //       b1   a1      a2      b2
+  bool a1_is_inside_b_range = b1.getFileOffset() <= a1.getFileOffset() &&
+                              a1.getFileOffset() <= b2.getFileOffset();
+
+  return b1_is_inside_a_range || a1_is_inside_b_range;
+}
+
+// Matcher for FieldDecl that has a SourceRange that overlaps other declarations
+// within the parent RecordDecl.
 //
 // Given
-//   struct MyStrict {
+//   struct MyStruct {
 //     int f;
 //     int f2, f3;
+//     struct S { int x } f4;
 //   };
-// matches |int f|, but does not match declarations of |f2| and |f3|.
-AST_MATCHER(clang::FieldDecl, hasUniqueTypeLoc) {
+// - doesn't match |f|
+// - matches |f2| and |f3| (which overlap each other's location)
+// - matches |f4| (which overlaps the location of |S|)
+AST_MATCHER(clang::FieldDecl, overlapsOtherDeclsWithinRecordDecl) {
   const clang::FieldDecl& self = Node;
+  const clang::SourceManager& source_manager =
+      Finder->getASTContext().getSourceManager();
+
   const clang::RecordDecl* record_decl = self.getParent();
-  clang::SourceLocation self_type_loc =
-      self.getTypeSourceInfo()->getTypeLoc().getBeginLoc();
-
-  bool has_sibling_with_same_type_loc =
-      std::any_of(record_decl->field_begin(), record_decl->field_end(),
-                  [&](const clang::FieldDecl* f) {
-                    // Is |f| a real sibling?
-                    if (f == &self)
-                      return false;  // Not a sibling.
-
-                    clang::SourceLocation sibling_type_loc =
-                        f->getTypeSourceInfo()->getTypeLoc().getBeginLoc();
-                    return self_type_loc == sibling_type_loc;
-                  });
-
-  return !has_sibling_with_same_type_loc;
+  clang::SourceRange self_range(self.getBeginLoc(), self.getEndLoc());
+
+  auto is_overlapping_sibling = [&](const clang::Decl* other_decl) {
+    if (other_decl == &self)
+      return false;
+
+    clang::SourceRange other_range(other_decl->getBeginLoc(),
+                                   other_decl->getEndLoc());
+    return IsOverlapping(source_manager, self_range, other_range);
+  };
+  bool has_sibling_with_overlapping_location =
+      std::any_of(record_decl->decls_begin(), record_decl->decls_end(),
+                  is_overlapping_sibling);
+  return has_sibling_with_overlapping_location;
 }
 
 // Rewrites |SomeClass* field| (matched as "affectedFieldDecl") into
@@ -631,13 +679,10 @@ int main(int argc, const char* argv[]) {
   auto record_with_deleted_allocation_operator_type_matcher =
       recordType(hasDeclaration(cxxRecordDecl(
           hasMethod(allOf(hasOverloadedOperatorName("new"), isDeleted())))));
-  auto non_free_standing_tag_type =
-      tagType(hasDeclaration(tagDecl(isNotFreeStandingTagDecl())));
   auto supported_pointer_types_matcher =
-      pointerType(unless(pointee(hasUnqualifiedDesugaredType(
-          anyOf(record_with_deleted_allocation_operator_type_matcher,
-                non_free_standing_tag_type, functionType(), memberPointerType(),
-                anyCharType(), arrayType())))));
+      pointerType(unless(pointee(hasUnqualifiedDesugaredType(anyOf(
+          record_with_deleted_allocation_operator_type_matcher, functionType(),
+          memberPointerType(), anyCharType(), arrayType())))));
 
   // Implicit field declarations =========
   // Matches field declarations that do not explicitly appear in the source
@@ -669,8 +714,9 @@ int main(int argc, const char* argv[]) {
   FieldDeclFilterFile fields_to_exclude(exclude_fields_param);
   auto field_decl_matcher =
       fieldDecl(
-          allOf(hasType(supported_pointer_types_matcher), hasUniqueTypeLoc(),
-                unless(anyOf(isInThirdPartyLocation(), isInGeneratedLocation(),
+          allOf(hasType(supported_pointer_types_matcher),
+                unless(anyOf(overlapsOtherDeclsWithinRecordDecl(),
+                             isInThirdPartyLocation(), isInGeneratedLocation(),
                              isExpansionInSystemHeader(), isInMacroLocation(),
                              isInExternCContext(),
                              isListedInFilterFile(fields_to_exclude),

tools/clang/rewrite_raw_ptr_fields/tests/various-types-expected.cc

@@ -5,6 +5,9 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <string>
+#include <vector>
+
 #include "base/memory/checked_ptr.h"
 
 namespace my_namespace {
@@ -15,6 +18,11 @@ class SomeClass {
   int data_member;
 };
 
+template <typename T>
+struct SomeTemplate {
+  T t;
+};
+
 // The class below deletes the |operator new| - this simulate's Blink's
 // STACK_ALLOCATED macro and/or OilPan / GarbageCollected<T> classes.
 class NoNewOperator {
@@ -40,6 +48,14 @@ struct MyStruct {
   // Expected rewrite: CheckedPtr<const bool> bool_ptr;
   CheckedPtr<const bool> const_bool_ptr;
 
+  // Pointers to templates.
+  // Expected rewrite: CheckedPtr<std::string> string_ptr;
+  CheckedPtr<std::string> string_ptr;
+  // Expected rewrite: CheckedPtr<std::vector<char>> vector_ptr;
+  CheckedPtr<std::vector<char>> vector_ptr;
+  // Expected rewrite: CheckedPtr<SomeTemplate<char>> template_ptr;
+  CheckedPtr<SomeTemplate<char>> template_ptr;
+
   // Some types may be spelled in various, alternative ways.  If possible, the
   // rewriter should preserve the original spelling.
   //

tools/clang/rewrite_raw_ptr_fields/tests/various-types-original.cc

@@ -5,6 +5,9 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <string>
+#include <vector>
+
 namespace my_namespace {
 
 class SomeClass {
@@ -13,6 +16,11 @@ class SomeClass {
   int data_member;
 };
 
+template <typename T>
+struct SomeTemplate {
+  T t;
+};
+
 // The class below deletes the |operator new| - this simulate's Blink's
 // STACK_ALLOCATED macro and/or OilPan / GarbageCollected<T> classes.
 class NoNewOperator {
@@ -38,6 +46,14 @@ struct MyStruct {
   // Expected rewrite: CheckedPtr<const bool> bool_ptr;
   const bool* const_bool_ptr;
 
+  // Pointers to templates.
+  // Expected rewrite: CheckedPtr<std::string> string_ptr;
+  std::string* string_ptr;
+  // Expected rewrite: CheckedPtr<std::vector<char>> vector_ptr;
+  std::vector<char>* vector_ptr;
+  // Expected rewrite: CheckedPtr<SomeTemplate<char>> template_ptr;
+  SomeTemplate<char>* template_ptr;
+
   // Some types may be spelled in various, alternative ways.  If possible, the
   // rewriter should preserve the original spelling.
   //