feat: Add presubmit check for DAWN_UNSAFE_BUFFERS safety comments This change adds a presubmit check CheckUnsafeBuffersSafetyComments that enforces C++ files using DAWN_UNSAFE_BUFFERS to have a preceding // SAFETY: comment explaining why it is safe. We also add the corresponding unit tests to PRESUBMIT_test.py. Bug: 517626950 Change-Id: I249b11ff87b40b034a2c002d67f1dd2c33c123ab Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/312321 Reviewed-by: Kai Ninomiya <kainino@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 7ebcaa3..b0912b4 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py
@@ -503,6 +503,57 @@ files_to_check=[r'^PRESUBMIT_test\.py$'])) +def CheckUnsafeBuffersSafetyComments(input_api, output_api): + """Checks that DAWN_UNSAFE_BUFFERS is accompanied by a + // SAFETY: comment. + """ + # We only check C++ source files. + exts = ('.h', '.cc', '.cpp', '.mm') + file_filter = lambda f: f.LocalPath().endswith(exts) + + unsafe_buffers_regex = re.compile(r'\bDAWN_UNSAFE_BUFFERS\b') + safety_comment_regex = re.compile(r'//.*\bSAFETY\b') + + problems = [] + + for f in input_api.AffectedFiles(include_deletes=False, + file_filter=file_filter): + lines = f.NewContents() + for line_num, line in enumerate(lines, start=1): + if line.strip().startswith('//'): + continue + if unsafe_buffers_regex.search(line): + # Check if safety comment is on the same line. + if safety_comment_regex.search(line): + continue + + # Check preceding lines for a SAFETY comment. + has_safety = False + for check_line_num in range(line_num - 1, 0, -1): + check_line = lines[check_line_num - 1].strip() + if not check_line.startswith('//'): + # Not a comment line, stop searching. + break + if safety_comment_regex.search(check_line): + has_safety = True + break + + if not has_safety: + problems.append( + f"{f.LocalPath()}:{line_num}: " + "DAWN_UNSAFE_BUFFERS usage must be accompanied by a " + "// SAFETY: comment.") + + if problems: + return [ + output_api.PresubmitError( + "DAWN_UNSAFE_BUFFERS usages must be accompanied by a " + "// SAFETY: comment explaining why they are safe.", + items=problems) + ] + return [] + + def CheckChangeTodoHasOwner(input_api, output_api): """ Checks that TODO comments have the issue number.
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index 45caafb..4975884 100644 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py
@@ -148,5 +148,131 @@ self.assertEqual(0, len(errors)) +class CheckUnsafeBuffersSafetyCommentsTest(unittest.TestCase): + + def testNoUsage(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' int x = 0;', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + def testValidUsageSameLine(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' DAWN_UNSAFE_BUFFERS(ptr[0] = 0); // SAFETY: safe', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + def testValidUsagePrecedingLine(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' // SAFETY: safe', + ' DAWN_UNSAFE_BUFFERS(ptr[0] = 0);', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + def testValidUsagePrecedingLineWithOtherComments(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' // SAFETY: safe', + ' // some other info', + ' DAWN_UNSAFE_BUFFERS(ptr[0] = 0);', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + def testInvalidUsageNoComment(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' DAWN_UNSAFE_BUFFERS(ptr[0] = 0);', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertIn('DAWN_UNSAFE_BUFFERS usage must be accompanied', + errors[0].items[0]) + + def testInvalidUsageCommentNotSafety(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' // this is a comment but not safety', + ' DAWN_UNSAFE_BUFFERS(ptr[0] = 0);', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + + def testInvalidUsageCommentSeparatedByCode(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' // SAFETY: safe', + ' int x = 0;', + ' DAWN_UNSAFE_BUFFERS(ptr[0] = 0);', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + + def testIgnoreCommentedUsage(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' // DAWN_UNSAFE_BUFFERS(ptr[0] = 0);', + '}', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + def testNonCppFilesIgnored(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.txt', [ + 'DAWN_UNSAFE_BUFFERS(ptr[0] = 0);', + ]) + ] + errors = PRESUBMIT.CheckUnsafeBuffersSafetyComments( + mock_input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + if __name__ == '__main__': unittest.main()