PRESUBMIT: Warn against adding new dangling pointers.

The DanglingPointerDetector is enabled on the CQ. It means we are now blocking new dangling pointers and can detect regressions. This blocks ~10 patches per day. The error message contains links toward the documentation and a guide to fix dangling pointers: - docs/dangling_ptr.md - docs/dangling_ptr_guide.md This is enough for most developers to fix them. However, it happens some are simply adding new `DanglingUntriaged` occurrences. Most are copy-pasting nearby code thinking this is the correct way to do. This patch adds a PRESUBMIT warning developers. Change-Id: I05d09e3f988f65b461a040c76a6ba2904073864b Bug: chromium:1395297 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4853628 Reviewed-by: Daniel Cheng <[email protected]> Commit-Queue: Arthur Sonzogni <[email protected]> Cr-Commit-Position: refs/heads/main@{#1200144}
junyujian · Sep 22, 2023 · 8228ad6 · 8228ad6
1 parent b6fc0a4
commit 8228ad6
Show file tree

Hide file tree

Showing 2 changed files with 202 additions and 0 deletions.
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
@@ -7091,3 +7091,58 @@ def LibcxxRevision(file):
     return [output_api.PresubmitError(
         'libcxx_revision not equal across %s' % ', '.join(DEPS_FILES),
         changed_deps_files)]
+
+
+def CheckDanglingUntriaged(input_api, output_api):
+    """Warn developers adding DanglingUntriaged raw_ptr."""
+    def FilterFile(file):
+        return input_api.FilterSourceFile(
+            file,
+            files_to_check=[r".*\.(h|cc|cpp|cxx|m|mm)$"],
+            files_to_skip=[r"^base/allocator.*"],
+        )
+
+    count = 0
+    for f in input_api.AffectedSourceFiles(FilterFile):
+        count -= f.OldContents().count("DanglingUntriaged")
+        count += f.NewContents().count("DanglingUntriaged")
+
+    # Most likely, nothing changed:
+    if count == 0:
+        return []
+
+    # Congrats developers for improving it:
+    if count < 0:
+        message = (
+            f"DanglingUntriaged pointers removed: {-count}",
+            f"Thank you!",
+        )
+        return [output_api.PresubmitNotifyResult(message)]
+
+    # Check for 'DanglingUntriaged-notes' in the description:
+    notes_regex = input_api.re.compile("DanglingUntriaged-notes[:=]")
+    if any(
+            notes_regex.match(line)
+            for line in input_api.change.DescriptionText().splitlines()):
+        return []
+
+    # Check for DanglingUntriaged-notes in the git footer:
+    if input_api.change.GitFootersFromDescription().get(
+            "DanglingUntriaged-notes", []):
+        return []
+
+    message = (
+        "Unexpected new occurrences of `DanglingUntriaged` detected. Please",
+        "avoid adding new ones",
+        "",
+        "See documentation:",
+        "https://chromium.googlesource.com/chromium/src/+/main/docs/dangling_ptr.md",
+        "",
+        "See also the guide to fix dangling pointers:",
+        "https://chromium.googlesource.com/chromium/src/+/main/docs/dangling_ptr_guide.md",
+        "",
+        "To disable this warning, please add in the commit description:",
+        "DanglingUntriaged-notes: <rational for new untriaged dangling "
+        "pointers>",
+    )
+    return [output_api.PresubmitPromptWarning(message)]
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
@@ -5079,5 +5079,152 @@ def testNoAbbreviation(self):
     results = PRESUBMIT.CheckNoAbbreviationInPngFileName(input_api, MockOutputApi())
     self.assertEqual(0, len(results))
 
+class CheckDanglingUntriagedTest(unittest.TestCase):
+  def testError(self):
+    """Test patch adding dangling pointers are reported"""
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.change.DescriptionText = lambda: "description"
+    mock_input_api.files = [
+      MockAffectedFile(
+        local_path="foo/foo.cc",
+        old_contents="raw_ptr<T>",
+        new_contents="raw_ptr<T, DanglingUntriaged>",
+      )
+    ]
+    msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api, mock_output_api)
+    self.assertEqual(len(msgs), 1)
+    self.assertEqual(len(msgs[0].message), 10)
+    self.assertEqual(
+      msgs[0].message[0],
+      "Unexpected new occurrences of `DanglingUntriaged` detected. Please",
+    )
+
+class CheckDanglingUntriagedTest(unittest.TestCase):
+  def testError(self):
+    """Test patch adding dangling pointers are reported"""
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.change.DescriptionText = lambda: "description"
+    mock_input_api.files = [
+      MockAffectedFile(
+        local_path="foo/foo.cc",
+        old_contents="raw_ptr<T>",
+        new_contents="raw_ptr<T, DanglingUntriaged>",
+      )
+    ]
+    msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
+                        mock_output_api)
+    self.assertEqual(len(msgs), 1)
+    self.assertEqual(len(msgs[0].message), 11)
+    self.assertEqual(
+      msgs[0].message[0],
+      "Unexpected new occurrences of `DanglingUntriaged` detected. Please",
+    )
+
+  def testNonCppFile(self):
+    """Test patch adding dangling pointers are not reported in non C++ files"""
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.change.DescriptionText = lambda: "description"
+    mock_input_api.files = [
+      MockAffectedFile(
+        local_path="foo/README.md",
+        old_contents="",
+        new_contents="The DanglingUntriaged annotation means",
+      )
+    ]
+    msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
+                        mock_output_api)
+    self.assertEqual(len(msgs), 0)
+
+  def testDeveloperAcknowledgeInCommitDescription(self):
+    """Test patch adding dangling pointers, but acknowledged by the developers
+    aren't reported"""
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.files = [
+      MockAffectedFile(
+        local_path="foo/foo.cc",
+        old_contents="raw_ptr<T>",
+        new_contents="raw_ptr<T, DanglingUntriaged>",
+      )
+    ]
+    mock_input_api.change.DescriptionText = lambda: (
+      "DanglingUntriaged-notes: Sorry about this!")
+    msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
+                        mock_output_api)
+    self.assertEqual(len(msgs), 0)
+
+  def testDeveloperAcknowledgeInCommitFooter(self):
+    """Test patch adding dangling pointers, but acknowledged by the developers
+    aren't reported"""
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.files = [
+      MockAffectedFile(
+        local_path="foo/foo.cc",
+        old_contents="raw_ptr<T>",
+        new_contents="raw_ptr<T, DanglingUntriaged>",
+      )
+    ]
+    mock_input_api.change.DescriptionText = lambda: "description"
+    mock_input_api.change.footers["DanglingUntriaged-notes"] = ["Sorry!"]
+    msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
+                        mock_output_api)
+    self.assertEqual(len(msgs), 0)
+
+  def testCongrats(self):
+    """Test the presubmit congrats users removing dangling pointers"""
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.files = [
+      MockAffectedFile(
+        local_path="foo/foo.cc",
+        old_contents="raw_ptr<T, DanglingUntriaged>",
+        new_contents="raw_ptr<T>",
+      )
+    ]
+    mock_input_api.change.DescriptionText = lambda: (
+      "This patch fixes some DanglingUntriaged pointers!")
+    msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
+                        mock_output_api)
+    self.assertEqual(len(msgs), 1)
+    self.assertTrue(
+      "DanglingUntriaged pointers removed: 1" in msgs[0].message)
+    self.assertTrue("Thank you!" in msgs[0].message)
+
+  def testRenameFile(self):
+    """Patch that we do not warn about DanglingUntriaged when moving files"""
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.files = [
+      MockAffectedFile(
+        local_path="foo/foo.cc",
+        old_contents="raw_ptr<T, DanglingUntriaged>",
+        new_contents="",
+        action="D",
+      ),
+      MockAffectedFile(
+        local_path="foo/foo.cc",
+        old_contents="",
+        new_contents="raw_ptr<T, DanglingUntriaged>",
+        action="A",
+      ),
+    ]
+    mock_input_api.change.DescriptionText = lambda: (
+      "This patch moves files")
+    msgs = PRESUBMIT.CheckDanglingUntriaged(mock_input_api,
+                        mock_output_api)
+    self.assertEqual(len(msgs), 0)
+
+
 if __name__ == '__main__':
   unittest.main()