Add banned functions PRESUBMIT check
Based heavily off Chromium's similar PRESUBMIT check.
Checks against:
1) `using namespace`, as it is banned by the C++ style guide.
2) `v8::Isolate::GetCurrent()`, as it is bug prone and there are better
alternatives.
Bug: pdfium:1737
Change-Id: Ia69d22d11a2383005db77ceaba952e94d21cf6fa
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/86310
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 955839c..55d417c 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -40,6 +40,106 @@
_KNOWN_ROBOTS = set() | set(
'%s@skia-public.iam.gserviceaccount.com' % s for s in ('pdfium-autoroll',))
+_THIRD_PARTY = 'third_party/'
+
+# Format: Sequence of tuples containing:
+# * String pattern or, if starting with a slash, a regular expression.
+# * Sequence of strings to show when the pattern matches.
+# * Error flag. True if a match is a presubmit error, otherwise it's a warning.
+# * Sequence of paths to *not* check (regexps).
+_BANNED_CPP_FUNCTIONS = (
+ (
+ r'/\busing namespace ',
+ (
+ 'Using directives ("using namespace x") are banned by the Google Style',
+ 'Guide ( https://google.github.io/styleguide/cppguide.html#Namespaces ).',
+ 'Explicitly qualify symbols or use using declarations ("using x::foo").',
+ ),
+ True,
+ [_THIRD_PARTY],
+ ),
+ (
+ 'v8::Isolate::GetCurrent()',
+ (
+ 'Avoid uses of v8::Isolate::GetCurrent(). Prefer holding a pointer to',
+ 'the v8::Isolate that was entered.',
+ ),
+ False,
+ (),
+ ),
+)
+
+
+def _CheckNoBannedFunctions(input_api, output_api):
+ """Makes sure that banned functions are not used."""
+ warnings = []
+ errors = []
+
+ def _GetMessageForMatchingType(input_api, affected_file, line_number, line,
+ type_name, message):
+ """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 = []
+
+ if input_api.re.search(r"^ *//",
+ line): # Ignore comments about banned types.
+ return result
+ if line.endswith(
+ " nocheck"): # A // nocheck comment will bypass this error.
+ return result
+
+ matched = False
+ if type_name[0:1] == '/':
+ regex = type_name[1:]
+ if input_api.re.search(regex, line):
+ matched = True
+ elif type_name in line:
+ matched = True
+
+ if matched:
+ result.append(' %s:%d:' % (affected_file.LocalPath(), line_number))
+ for message_line in message:
+ result.append(' %s' % message_line)
+
+ return result
+
+ def IsExcludedFile(affected_file, excluded_paths):
+ local_path = affected_file.LocalPath()
+ for item in excluded_paths:
+ if input_api.re.match(item, local_path):
+ return True
+ return False
+
+ def CheckForMatch(affected_file, line_num, line, func_name, message, error):
+ problems = _GetMessageForMatchingType(input_api, f, line_num, line,
+ func_name, message)
+ if problems:
+ if error:
+ errors.extend(problems)
+ else:
+ warnings.extend(problems)
+
+ file_filter = lambda f: f.LocalPath().endswith(('.cc', '.cpp', '.h'))
+ for f in input_api.AffectedFiles(file_filter=file_filter):
+ for line_num, line in f.ChangedContents():
+ for func_name, message, error, excluded_paths in _BANNED_CPP_FUNCTIONS:
+ if IsExcludedFile(f, excluded_paths):
+ continue
+ CheckForMatch(f, line_num, line, func_name, message, error)
+
+ result = []
+ if (warnings):
+ result.append(
+ output_api.PresubmitPromptWarning('Banned functions were used.\n' +
+ '\n'.join(warnings)))
+ if (errors):
+ result.append(
+ output_api.PresubmitError('Banned functions were used.\n' +
+ '\n'.join(errors)))
+ return result
+
def _CheckUnwantedDependencies(input_api, output_api):
"""Runs checkdeps on #include statements added in this
@@ -365,6 +465,7 @@
def CheckChangeOnUpload(input_api, output_api):
results = []
+ results.extend(_CheckNoBannedFunctions(input_api, output_api))
results.extend(_CheckUnwantedDependencies(input_api, output_api))
results.extend(
input_api.canned_checks.CheckPatchFormatted(input_api, output_api))
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
index cbe87d7..72723ed 100755
--- a/PRESUBMIT_test.py
+++ b/PRESUBMIT_test.py
@@ -9,6 +9,37 @@
from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi, MockFile
+class BannedTypeCheckTest(unittest.TestCase):
+
+ def testBannedCppFunctions(self):
+ input_api = MockInputApi()
+ input_api.files = [
+ MockFile('some/cpp/problematic/file.cc', ['using namespace std;']),
+ MockFile('third_party/some/cpp/problematic/file.cc',
+ ['using namespace std;']),
+ MockFile('some/cpp/ok/file.cc', ['using std::string;']),
+ MockFile('some/cpp/nocheck/file.cc',
+ ['using namespace std; // nocheck']),
+ MockFile('some/cpp/comment/file.cc',
+ [' // A comment about `using namespace std;`']),
+ MockFile('some/cpp/v8/file.cc', ['v8::Isolate::GetCurrent()']),
+ ]
+
+ results = PRESUBMIT._CheckNoBannedFunctions(input_api, MockOutputApi())
+
+ # warnings are results[0], errors are results[1]
+ self.assertEqual(2, len(results))
+ self.assertTrue('some/cpp/problematic/file.cc' in results[1].message)
+ self.assertFalse(
+ 'third_party/some/cpp/problematic/file.cc' in results[1].message)
+ self.assertFalse('some/cpp/ok/file.cc' in results[1].message)
+ self.assertFalse('some/cpp/nocheck/file.cc' in results[0].message)
+ self.assertFalse('some/cpp/nocheck/file.cc' in results[1].message)
+ self.assertFalse('some/cpp/comment/file.cc' in results[0].message)
+ self.assertFalse('some/cpp/comment/file.cc' in results[1].message)
+ self.assertTrue('some/cpp/v8/file.cc' in results[0].message)
+
+
class CheckChangeOnUploadTest(unittest.TestCase):
def testCheckPNGFormat(self):
diff --git a/PRESUBMIT_test_mocks.py b/PRESUBMIT_test_mocks.py
index ce9fcb0..e0e484f 100644
--- a/PRESUBMIT_test_mocks.py
+++ b/PRESUBMIT_test_mocks.py
@@ -44,6 +44,12 @@
MockOutputApi.PresubmitResult.__init__(self, message, items, long_text)
self.type = 'error'
+ class PresubmitPromptWarning(PresubmitResult):
+
+ def __init__(self, message, items=None, long_text=''):
+ MockOutputApi.PresubmitResult.__init__(self, message, items, long_text)
+ self.type = 'warning'
+
class MockFile(object):
"""Mock class for the File class.
@@ -65,5 +71,8 @@
self._action = action
self._old_contents = old_contents
+ def ChangedContents(self):
+ return self._changed_contents
+
def LocalPath(self):
return self._local_path
