Reland "PRESUBMIT: Warn against adding new dangling pointers."
This is a reland of commit 8228ad6aef
What happens in the reland:
---------------------------
I added an early return when `input_api.nodiff` is true. This disable
the check completely.
Performance evaluation:
-----------------------
Because the original presubmit check was reverted due to the performance
cost when running `git cl presubmit --all`, it comes the question of the
performance cost during normal `git cl presubmit`.
I am evaluating the cost to be in the range: [-0.178%, -1.04%].
This is the 95 confidence interval using a t-test.
This was tested on a very large patch modifying 535 files. The data was:
Without With
------- ----
91.1s 91.5s
91.6s 91.8s
91.6s 92.1s
90.9s 92.0s
91.3s 91.9s
Original change's description:
> 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 <dcheng@chromium.org>
> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1200144}
Bug: chromium:1395297
Change-Id: I0a8305383eda3c3b50c0f433bd6751cf19069584
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4907727
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204558}
This commit is contained in:
Arthur Sonzogni
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
f234769a2f
commit
7109bd363b
66
PRESUBMIT.py
66
PRESUBMIT.py
@ -7106,3 +7106,69 @@ def CheckLibcxxRevisionsMatch(input_api, output_api):
|
||||
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."""
|
||||
|
||||
# Ignore during git presubmit --all.
|
||||
#
|
||||
# This would be too costly, because this would check every lines of every
|
||||
# C++ files. Check from _BANNED_CPP_FUNCTIONS are also reading the whole
|
||||
# source code, but only once to apply every checks. It seems the bots like
|
||||
# `win-presubmit` are particularly sensitive to reading the files. Adding
|
||||
# this check caused the bot to run 2x longer. See https://crbug.com/1486612.
|
||||
if input_api.no_diffs:
|
||||
return []
|
||||
|
||||
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)]
|
||||
|
||||
@ -5086,5 +5086,152 @@ class CheckNoAbbreviationInPngFileNameTest(unittest.TestCase):
|
||||
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()
|
||||
|
||||
Reference in New Issue
Block a user