feat: Add extensible CheckBannedPatterns check This change adds a generic, extensible presubmit check CheckBannedPatterns that checks for banned patterns based on a configuration list. Each pattern can specify regex, message, severity (error or warning), and file_filter. We use this to ban DAWN_UNSAFE_TODO and #pragma allow_unsafe_buffers (as warnings) in C++ files. We also add the corresponding unit tests to PRESUBMIT_test.py. Fixed: 517626950 Change-Id: Ia2438763d8b15dc33d667d079c254f14573e0974 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/312322 Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Kai Ninomiya <kainino@chromium.org> Commit-Queue: Kai Ninomiya <kainino@chromium.org>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py index b0912b4..f6e2edb 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py
@@ -26,8 +26,12 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. import hashlib +import dataclasses import re import sys +from typing import Optional +from typing import Sequence +from typing import Tuple PRESUBMIT_VERSION = '2.0.0' @@ -86,6 +90,55 @@ LINT_FILTERS = [] + +# Copied from Chrome's BanRule. +@dataclasses.dataclass +class BanRule: + # String pattern. If the pattern begins with a slash, the pattern will be + # treated as a regular expression instead. + pattern: str + + # Explanation as a sequence of strings. Each string in the sequence will be + # printed on its own line. + explanation: Tuple[str, ...] + + # Whether or not to treat this ban as a fatal error. + treat_as_error: bool = False + + # Paths that should be excluded from the ban check. Each string is a regular + # expression that will be matched against the path of the file being checked + # relative to the root of the source tree. + excluded_paths: Optional[Sequence[str]] = None + + # If True, surfaces any violation as a Gerrit comment on the CL after + # running the CQ. + surface_as_gerrit_lint: Optional[bool] = None + + +# Configuration for banned patterns checks. +_BANNED_CPP_PATTERNS: Sequence[BanRule] = ( + BanRule( + pattern=r'/\bDAWN_UNSAFE_TODO\b', + explanation=( + 'Do not introduce new instances of DAWN_UNSAFE_TODO. ', + 'Use DAWN_UNSAFE_BUFFERS with a // SAFETY: comment instead, ', + 'or rewrite to be safe.', + ), + treat_as_error=False, + surface_as_gerrit_lint=True, + ), + BanRule( + pattern=r'/#pragma\s+allow_unsafe_buffers\b', + explanation=( + '#pragma allow_unsafe_buffers is discouraged. Prefer using ', + 'DAWN_UNSAFE_BUFFERS with a // SAFETY: comment for ', + 'specific blocks, or rewrite to be safe.', + ), + treat_as_error=False, + surface_as_gerrit_lint=True, + ), +) + EXPECTED_LICENSE_TEXT = { "//": [ "//", @@ -589,6 +642,94 @@ return [] +# Copied from Chrome's _GetMessageForMatchingType. +def _GetMessageForMatchingType(input_api, affected_file, line_number, line, + ban_rule): + """ + Helper method for checking for banned constructs. + + Returns an string composed of the name of the file, the line number + where the match has been found and the additional text passed as + |message| in case the target type name matches the text inside the + line passed as parameter. + """ + result = [] + + # Ignore comments about banned types. + if input_api.re.search(r'^ *//', line): + return result + # A // nocheck comment will bypass this error. + if line.endswith(' nocheck'): + return result + + matched = False + if ban_rule.pattern[0:1] == '/': + regex = ban_rule.pattern[1:] + if input_api.re.search(regex, line): + matched = True + elif ban_rule.pattern in line: + matched = True + + if matched: + result.append(' %s:%d:' % (affected_file.LocalPath(), line_number)) + for line in ban_rule.explanation: + result.append(' %s' % line) + + return result + + +# Copied from Chrome's CheckNoBannedPatterns with modifications. +def CheckNoBannedPatterns(input_api, output_api): + """Make sure that banned patterns are not used.""" + results = [] + + def IsExcludedFile(affected_file, excluded_paths): + if not excluded_paths: + return False + + local_path = affected_file.UnixLocalPath() + for item in excluded_paths: + if input_api.re.match(item, local_path): + return True + return False + + def CheckForMatch(affected_file, line_num: int, line: str, + ban_rule: BanRule): + if IsExcludedFile(affected_file, ban_rule.excluded_paths): + return + + message = _GetMessageForMatchingType(input_api, affected_file, + line_num, line, ban_rule) + if message: + result_loc = [] + if ban_rule.surface_as_gerrit_lint: + result_loc.append( + output_api.PresubmitResultLocation( + file_path=affected_file.LocalPath(), + start_line=line_num, + end_line=line_num, + )) + if ban_rule.treat_as_error: + results.append( + output_api.PresubmitError('A banned pattern was used.\n' + + '\n'.join(message), + locations=result_loc)) + else: + results.append( + output_api.PresubmitPromptWarning( + 'A banned pattern was used.\n' + '\n'.join(message), + locations=result_loc)) + + file_filter = lambda f: f.LocalPath().endswith( + ('.cc', '.mm', '.cpp', '.h')) + for f in input_api.AffectedFiles(file_filter=file_filter): + for line_num, line in f.ChangedContents(): + for ban_rule in _BANNED_CPP_PATTERNS: + CheckForMatch(f, line_num, line, ban_rule) + + return results + + def CheckChange(input_api, output_api): results = [] results.extend(
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index 4975884..7468efd 100644 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py
@@ -41,6 +41,39 @@ MockAffectedFile, MockInputApi, MockOutputApi) +class MockOutputApiWithLocations(MockOutputApi): + + class PresubmitResultLocation(object): + + def __init__(self, file_path, start_line, end_line): + self.file_path = file_path + self.start_line = start_line + self.end_line = end_line + + class PresubmitResult(object): + + def __init__(self, message, items=None, long_text='', locations=None): + self.message = message + self.items = items + self.long_text = long_text + self.locations = locations or [] + + def __repr__(self): + return self.message + + class PresubmitError(PresubmitResult): + + def __init__(self, message, items=None, long_text='', locations=None): + super().__init__(message, items, long_text, locations) + self.type = 'error' + + class PresubmitPromptWarning(PresubmitResult): + + def __init__(self, message, items=None, long_text='', locations=None): + super().__init__(message, items, long_text, locations) + self.type = 'warning' + + class CheckChangeTodoHasOwnerTest(unittest.TestCase): def _create_mock_input_api(self): @@ -274,5 +307,87 @@ self.assertEqual(0, len(errors)) +class CheckBannedPatternsTest(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.CheckNoBannedPatterns(mock_input_api, + MockOutputApiWithLocations()) + self.assertEqual(0, len(errors)) + + def testBannedUnsafeTodo(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + 'void Foo() {', + ' DAWN_UNSAFE_TODO(ptr[0] = 0);', + '}', + ]) + ] + errors = PRESUBMIT.CheckNoBannedPatterns(mock_input_api, + MockOutputApiWithLocations()) + self.assertEqual(1, len(errors)) + self.assertIn('A banned pattern was used.', errors[0].message) + self.assertIn('src/dawn/Foo.cpp:2:', errors[0].message) + self.assertIn('Do not introduce new instances of DAWN_UNSAFE_TODO', + errors[0].message) + self.assertEqual(1, len(errors[0].locations)) + loc = errors[0].locations[0] + self.assertEqual('src/dawn/Foo.cpp', loc.file_path) + self.assertEqual(2, loc.start_line) + self.assertEqual(2, loc.end_line) + + def testBannedPragma(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + '#pragma allow_unsafe_buffers', + ]) + ] + errors = PRESUBMIT.CheckNoBannedPatterns(mock_input_api, + MockOutputApiWithLocations()) + self.assertEqual(1, len(errors)) + self.assertIn('A banned pattern was used.', errors[0].message) + self.assertIn('src/dawn/Foo.cpp:1:', errors[0].message) + self.assertIn('#pragma allow_unsafe_buffers is discouraged', + errors[0].message) + self.assertEqual(1, len(errors[0].locations)) + loc = errors[0].locations[0] + self.assertEqual('src/dawn/Foo.cpp', loc.file_path) + self.assertEqual(1, loc.start_line) + self.assertEqual(1, loc.end_line) + + def testCommentedUsageIgnored(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.cpp', [ + '// DAWN_UNSAFE_TODO(ptr[0] = 0);', + '// #pragma allow_unsafe_buffers', + ]) + ] + errors = PRESUBMIT.CheckNoBannedPatterns(mock_input_api, + MockOutputApiWithLocations()) + self.assertEqual(0, len(errors)) + + def testNonCppFilesIgnored(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockAffectedFile('src/dawn/Foo.txt', [ + 'DAWN_UNSAFE_TODO(ptr[0] = 0);', + '#pragma allow_unsafe_buffers', + ]) + ] + errors = PRESUBMIT.CheckNoBannedPatterns(mock_input_api, + MockOutputApiWithLocations()) + self.assertEqual(0, len(errors)) + + if __name__ == '__main__': unittest.main()