Ignoring |SomeClass *ptr_field1, *ptr_field2|.

Consider the example fixed by this CL in tests/basics-original.cc:

    struct MyStruct {
      SomeClass *ptr_field1, *ptr_field2;
    };

Before this CL, the rewriter would generate a separate replacement for
|ptr_field1| and another one for |ptr_field2|.  Such replacements would
cover overlapping ranges of the source file, leading to butchered,
incorrect edits.

Note that in the example above |git cl format| places the "*" character
above next to the field names (e.g. |*ptr_field1|) rather than next to
the type name (e.g.  |SomeClass*|).  For a single field |git cl format|
places the "*" character next to the type name, but the CL adds a
single-field test to also cover the non-standard placement.

After this CL:
*) apply_edits.py detects overlapping replacements and reports them as
   an error.  Simple unit tests have been added to apply_edits_test.py
*) RewriteRawPtrFields.cpp defines and uses a custom |hasUniqueTypeLoc|
   matcher that can be used to ignore fields with a non-unique type
   location
*) RewriteRawPtrFields.cpp tweaks slightly the |replacement_range| and
   |replacement_text| so that non-standard placement of the "*"
   character is handled correctly.

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

tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp

@@ -92,6 +92,36 @@ AST_MATCHER(clang::Type, anyCharType) {
   return Node.isAnyCharacterType();
 }
 
+// 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).
+//
+// Given
+//   struct MyStrict {
+//     int f;
+//     int f2, f3;
+//   };
+// matches |int f|, but does not match declarations of |f2| and |f3|.
+AST_MATCHER(clang::FieldDecl, hasUniqueTypeLoc) {
+  const clang::FieldDecl& self = Node;
+  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;
+}
+
 class FieldDeclRewriter : public MatchFinder::MatchCallback {
  public:
   explicit FieldDeclRewriter(ReplacementsPrinter* replacements_printer)
@@ -117,7 +147,7 @@ class FieldDeclRewriter : public MatchFinder::MatchCallback {
     //
     // Consider the following example:
     //      const Pointee* const field_name_;
-    //      ^-------------------^   = |replacement_range|
+    //      ^--------------------^  = |replacement_range|
     //                           ^  = |field_decl->getLocation()|
     //      ^                       = |field_decl->getBeginLoc()|
     //                   ^          = PointerTypeLoc::getStarLoc
@@ -126,9 +156,8 @@ class FieldDeclRewriter : public MatchFinder::MatchCallback {
     // We get the |replacement_range| in a bit clumsy way, because clang docs
     // for QualifiedTypeLoc explicitly say that these objects "intentionally do
     // not provide source location for type qualifiers".
-    clang::SourceRange replacement_range(
-        field_decl->getBeginLoc(),
-        field_decl->getLocation().getLocWithOffset(-1));
+    clang::SourceRange replacement_range(field_decl->getBeginLoc(),
+                                         field_decl->getLocation());
 
     // Calculate |replacement_text|.
     std::string replacement_text = GenerateNewText(ast_context, pointer_type);
@@ -161,7 +190,7 @@ class FieldDeclRewriter : public MatchFinder::MatchCallback {
     printing_policy.SuppressScope = 1;  // s/blink::Pointee/Pointee/
     std::string pointee_type_as_string =
         pointee_type.getAsString(printing_policy);
-    result += llvm::formatv("CheckedPtr<{0}>", pointee_type_as_string);
+    result += llvm::formatv("CheckedPtr<{0}> ", pointee_type_as_string);
 
     return result;
   }
@@ -231,7 +260,7 @@ int main(int argc, const char* argv[]) {
   // - fields of lambda-supporting classes
   auto field_decl_matcher =
       fieldDecl(allOf(hasType(supported_pointer_types_matcher),
-                      unless(implicit_field_decl_matcher)))
+                      hasUniqueTypeLoc(), unless(implicit_field_decl_matcher)))
           .bind("fieldDecl");
   FieldDeclRewriter field_decl_rewriter(&replacements_printer);
   match_finder.addMatcher(field_decl_matcher, &field_decl_rewriter);

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

@@ -20,6 +20,21 @@ struct MyStruct {
 
   // No rewrite expected.
   int int_field;
+
+  // "*" next to the field name.  This is non-standard formatting, so
+  // "clang-format off" is used to make sure |git cl format| won't change this
+  // testcase.
+  //
+  // Expected rewrite: CheckedPtr<SomeClass> raw_ptr_field;
+  // clang-format off
+  CheckedPtr<SomeClass> raw_ptr_field2;
+  // clang-format on
+
+  // No rewrite expected.  (The syntax below should be rare + it is difficult to
+  // generate correct, non-overlapping replacements that cover both fields.)
+  SomeClass *overlapping_fields_1a, *overlapping_fields_1b;
+  int overlapping_fields_2a, *overlapping_fields_2b;
+  int *overlapping_fields_3a, overlapping_fields_3b;
 };
 
 template <typename T>

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

@@ -18,6 +18,21 @@ struct MyStruct {
 
   // No rewrite expected.
   int int_field;
+
+  // "*" next to the field name.  This is non-standard formatting, so
+  // "clang-format off" is used to make sure |git cl format| won't change this
+  // testcase.
+  //
+  // Expected rewrite: CheckedPtr<SomeClass> raw_ptr_field;
+  // clang-format off
+  SomeClass *raw_ptr_field2;
+  // clang-format on
+
+  // No rewrite expected.  (The syntax below should be rare + it is difficult to
+  // generate correct, non-overlapping replacements that cover both fields.)
+  SomeClass *overlapping_fields_1a, *overlapping_fields_1b;
+  int overlapping_fields_2a, *overlapping_fields_2b;
+  int *overlapping_fields_3a, overlapping_fields_3b;
 };
 
 template <typename T>

tools/clang/scripts/apply_edits.py

@@ -197,12 +197,24 @@ def _InsertNonSystemIncludeHeader(filepath, header_line_to_add, contents):
 
 
 def _ApplyReplacement(filepath, contents, edit, last_edit):
-  if (last_edit is not None and edit.edit_type == last_edit.edit_type
-      and edit.offset == last_edit.offset and edit.length == last_edit.length):
-    raise ValueError(('Conflicting replacement text: ' +
-                      '%s at offset %d, length %d: "%s" != "%s"\n') %
-                     (filepath, edit.offset, edit.length, edit.replacement,
-                      last_edit.replacement))
+  assert (edit.edit_type == 'r')
+  assert ((last_edit is None) or (last_edit.edit_type == 'r'))
+
+  if last_edit is not None:
+    if edit.offset == last_edit.offset and edit.length == last_edit.length:
+      assert (edit.replacement != last_edit.replacement)
+      raise ValueError(('Conflicting replacement text: ' +
+                        '%s at offset %d, length %d: "%s" != "%s"\n') %
+                       (filepath, edit.offset, edit.length, edit.replacement,
+                        last_edit.replacement))
+
+    if edit.offset + edit.length > last_edit.offset:
+      raise ValueError(
+          ('Overlapping replacements: ' +
+           '%s at offset %d, length %d: "%s" and ' +
+           'offset %d, length %d: "%s"\n') %
+          (filepath, edit.offset, edit.length, edit.replacement,
+           last_edit.offset, last_edit.length, last_edit.replacement))
 
   contents[edit.offset:edit.offset + edit.length] = edit.replacement
   if not edit.replacement:

tools/clang/scripts/apply_edits_test.py

@@ -574,6 +574,17 @@ class ApplyReplacementTest(unittest.TestCase):
     with self.assertRaisesRegexp(ValueError, expected_msg_regex):
       _ApplyEdit(old_text, edit)
 
+  def testOverlappingReplacement(self):
+    old_text = "123 456 789"
+    last = _CreateReplacement(old_text, "456 789", "foo")
+    edit = _CreateReplacement(old_text, "123 456", "bar")
+    expected_msg_regex = 'Overlapping replacements'
+    expected_msg_regex += '.*some_file.cc'
+    expected_msg_regex += '.*offset 0, length 7.*"bar"'
+    expected_msg_regex += '.*offset 4, length 7.*"foo"'
+    with self.assertRaisesRegexp(ValueError, expected_msg_regex):
+      _ApplyEdit(old_text, edit, last_edit=last)
+
 
 if __name__ == '__main__':
   unittest.main()