[depot_tools, chromium/src] mv CheckInclusiveLanguage into canned checks

Also adds a new canned_presubmit_checks_test.py and supporting mocks,
based on the PRESUBMIT_*.py under chromium/src.

If this is OK, there are subsequent CLs for removing the original code
from chromuim/src and infra/infra, and having their PRESUBMIT scripts
just reference this canned check instead.

Change-Id: I67dfb7ac0b4cdc36bd62ec2dc062ca5c78c2244e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2805268
Commit-Queue: Sean McCullough <seanmccullough@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>

presubmit_canned_checks.py

@@ -1715,3 +1715,136 @@ def CheckJsonParses(input_api, output_api):
         warnings.append(output_api.PresubmitPromptWarning(
             '%s does not appear to be valid JSON.' % f.LocalPath()))
   return warnings
+
+# string pattern, sequence of strings to show when pattern matches,
+# error flag. True if match is a presubmit error, otherwise it's a warning.
+_NON_INCLUSIVE_TERMS = (
+    (
+        # Note that \b pattern in python re is pretty particular. In this
+        # regexp, 'class WhiteList ...' will match, but 'class FooWhiteList
+        # ...' will not. This may require some tweaking to catch these cases
+        # without triggering a lot of false positives. Leaving it naive and
+        # less matchy for now.
+        r'/\b(?i)((black|white)list|slave)\b',  # nocheck
+        (
+            'Please don\'t use blacklist, whitelist, '  # nocheck
+            'or slave in your',  # nocheck
+            'code and make every effort to use other terms. Using "// nocheck"',
+            '"# nocheck" or "<!-- nocheck -->"',
+            'at the end of the offending line will bypass this PRESUBMIT error',
+            'but avoid using this whenever possible. Reach out to',
+            'community@chromium.org if you have questions'),
+        True),)
+
+
+def _GetMessageForMatchingTerm(input_api, affected_file, line_number, line,
+                               term, message):
+  """Helper method for CheckInclusiveLanguage.
+
+  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 = []
+
+  # A // nocheck comment will bypass this error.
+  if line.endswith(" nocheck") or line.endswith("<!-- nocheck -->"):
+    return result
+
+  # Ignore C-style single-line comments about banned terms.
+  if input_api.re.search(r"//.*$", line):
+    line = input_api.re.sub(r"//.*$", "", line)
+
+  # Ignore lines from C-style multi-line comments.
+  if input_api.re.search(r"^\s*\*", line):
+    return result
+
+  # Ignore Python-style comments about banned terms.
+  # This actually removes comment text from the first # on.
+  if input_api.re.search(r"#.*$", line):
+    line = input_api.re.sub(r"#.*$", "", line)
+
+  matched = False
+  if term[0:1] == '/':
+    regex = term[1:]
+    if input_api.re.search(regex, line):
+      matched = True
+  elif term 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 CheckInclusiveLanguage(input_api, output_api,
+                           excluded_directories_relative_path=None,
+                           non_inclusive_terms=_NON_INCLUSIVE_TERMS):
+  """Make sure that banned non-inclusive terms are not used."""
+
+  # Presubmit checks may run on a bot where the changes are actually
+  # in a repo that isn't chromium/src (e.g., when testing src + tip-of-tree
+  # ANGLE), but this particular check only makes sense for changes to
+  # chromium/src.
+  if input_api.change.RepositoryRoot() != input_api.PresubmitLocalPath():
+    return []
+
+  warnings = []
+  errors = []
+
+  if excluded_directories_relative_path is None:
+    excluded_directories_relative_path = [
+        'infra',
+        'inclusive_language_presubmit_exempt_dirs.txt'
+    ]
+
+  # Note that this matches exact path prefixes, and does not match
+  # subdirectories. Only files directly in an exlcluded path will
+  # match.
+  def IsExcludedFile(affected_file, excluded_paths):
+    local_dir = input_api.os_path.dirname(affected_file.LocalPath())
+
+    return local_dir in excluded_paths
+
+  def CheckForMatch(affected_file, line_num, line, term, message, error):
+    problems = _GetMessageForMatchingTerm(input_api, affected_file, line_num,
+                                          line, term, message)
+
+    if problems:
+      if error:
+        errors.extend(problems)
+      else:
+        warnings.extend(problems)
+
+  excluded_paths = []
+  dirs_file_path = input_api.os_path.join(input_api.change.RepositoryRoot(),
+                                          *excluded_directories_relative_path)
+  f = input_api.ReadFile(dirs_file_path)
+
+  for line in f.splitlines():
+    path = line.split()[0]
+    if len(path) > 0:
+      excluded_paths.append(path)
+
+  excluded_paths = set(excluded_paths)
+  for f in input_api.AffectedFiles():
+    for line_num, line in f.ChangedContents():
+      for term, message, error in non_inclusive_terms:
+        if IsExcludedFile(f, excluded_paths):
+          continue
+        CheckForMatch(f, line_num, line, term, message, error)
+
+  result = []
+  if (warnings):
+    result.append(
+        output_api.PresubmitPromptWarning(
+            'Banned non-inclusive language was used.\n' + '\n'.join(warnings)))
+  if (errors):
+    result.append(
+        output_api.PresubmitError('Banned non-inclusive language was used.\n' +
+                                  '\n'.join(errors)))
+  return result
+

presubmit_canned_checks_test.py

@@ -0,0 +1,204 @@
+#!/usr/bin/env python
+# Copyright (c) 2021 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import os.path
+import subprocess
+import unittest
+
+from presubmit_canned_checks_test_mocks import MockFile, MockAffectedFile
+from presubmit_canned_checks_test_mocks import MockInputApi, MockOutputApi
+
+import presubmit_canned_checks
+
+class InclusiveLanguageCheckTest(unittest.TestCase):
+  def testBlockedTerms(self):
+    input_api = MockInputApi()
+    input_api.change.RepositoryRoot = lambda: ''
+    input_api.presubmit_local_path = ''
+
+    input_api.files = [
+      MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
+               'some/dir 2 1',
+               'some/other/dir 2 1',
+               ]),
+      MockFile('some/ios/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
+                '}']),
+      MockFile('some/mac/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction, BlackList) {', # nocheck
+                '}']),
+      MockFile('another/ios_file.mm',
+               ['class SomeTest : public testing::Test blocklist {};']),
+      MockFile('some/ios/file_egtest.mm',
+               ['- (void)testSomething { V(whitelist); }']), # nocheck
+      MockFile('some/ios/file_unittest.mm',
+               ['TEST_F(SomeTest, Whitelist) { V(allowlist); }']), # nocheck
+      MockFile('some/doc/file.md',
+               ['# Title',
+                'Some markdown text includes master.', # nocheck
+               ]),
+      MockFile('some/doc/ok_file.md',
+               ['# Title',
+                 # This link contains a '//' which the matcher thinks is a
+                 # C-style comment, and the 'master' term appears after the
+                 # '//' in the URL, so it gets ignored as a side-effect.
+                '[Ignored](https://git/project.git/+/master/foo)', # nocheck
+               ]),
+      MockFile('some/doc/branch_name_file.md',
+               ['# Title',
+                 # Matches appearing before `//` still trigger the check.
+                '[src/master](https://git/p.git/+/master/foo)', # nocheck
+               ]),
+      MockFile('some/java/file/TestJavaDoc.java',
+               ['/**',
+                ' * This line contains the word master,', # nocheck
+                 '* ignored because this is a comment. See {@link',
+                ' * https://s/src/+/master:tools/README.md}', # nocheck
+                ' */']),
+      MockFile('some/java/file/TestJava.java',
+               ['class TestJava {',
+                '  public String master;', # nocheck
+                '}']),
+      MockFile('some/html/file.html',
+               ['<-- an existing html multiline comment',
+                'says "master" here', # nocheck
+                'in the comment -->'])
+     ]
+
+    errors = presubmit_canned_checks.CheckInclusiveLanguage(input_api,
+                                                            MockOutputApi())
+    self.assertEqual(1, len(errors))
+    self.assertTrue('some/ios/file.mm' in errors[0].message)
+    self.assertTrue('another/ios_file.mm' not in errors[0].message)
+    self.assertTrue('some/mac/file.mm' in errors[0].message)
+    self.assertTrue('some/ios/file_egtest.mm' in errors[0].message)
+    self.assertTrue('some/ios/file_unittest.mm' in errors[0].message)
+    self.assertTrue('some/doc/file.md' not in errors[0].message)
+    self.assertTrue('some/doc/ok_file.md' not in errors[0].message)
+    self.assertTrue('some/doc/branch_name_file.md' not in errors[0].message)
+    self.assertTrue('some/java/file/TestJavaDoc.java' not in errors[0].message)
+    self.assertTrue('some/java/file/TestJava.java' not in errors[0].message)
+    self.assertTrue('some/html/file.html' not in errors[0].message)
+
+  def testBlockedTermsWithLegacy(self):
+    input_api = MockInputApi()
+    input_api.change.RepositoryRoot = lambda: ''
+    input_api.presubmit_local_path = ''
+
+    input_api.files = [
+      MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
+               'some/ios 2 1',
+               'some/other/dir 2 1',
+               ]),
+      MockFile('some/ios/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
+                '}']),
+      MockFile('some/ios/subdir/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
+                '}']),
+      MockFile('some/mac/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction, BlackList) {', # nocheck
+                '}']),
+      MockFile('another/ios_file.mm',
+               ['class SomeTest : public testing::Test blocklist {};']),
+      MockFile('some/ios/file_egtest.mm',
+               ['- (void)testSomething { V(whitelist); }']), # nocheck
+      MockFile('some/ios/file_unittest.mm',
+               ['TEST_F(SomeTest, Whitelist) { V(allowlist); }']), # nocheck
+    ]
+
+    errors = presubmit_canned_checks.CheckInclusiveLanguage(input_api,
+                                                            MockOutputApi())
+    self.assertEqual(1, len(errors))
+    self.assertTrue('some/ios/file.mm' not in errors[0].message)
+    self.assertTrue('some/ios/subdir/file.mm' in errors[0].message)
+    self.assertTrue('another/ios_file.mm' not in errors[0].message)
+    self.assertTrue('some/mac/file.mm' in errors[0].message)
+    self.assertTrue('some/ios/file_egtest.mm' not in errors[0].message)
+    self.assertTrue('some/ios/file_unittest.mm' not in errors[0].message)
+
+  def testBlockedTermsWithNocheck(self):
+    input_api = MockInputApi()
+    input_api.change.RepositoryRoot = lambda: ''
+    input_api.presubmit_local_path = ''
+
+    input_api.files = [
+      MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
+               'some/dir 2 1',
+               'some/other/dir 2 1',
+               ]),
+      MockFile('some/ios/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction, ',
+                ' blacklist) { // nocheck', # nocheck
+                '}']),
+      MockFile('some/mac/file.mm',
+               ['TEST(SomeClassTest, SomeInteraction, ',
+                'BlackList) { // nocheck', # nocheck
+                '}']),
+      MockFile('another/ios_file.mm',
+               ['class SomeTest : public testing::Test blocklist {};']),
+      MockFile('some/ios/file_egtest.mm',
+               ['- (void)testSomething { ',
+                'V(whitelist); } // nocheck']), # nocheck
+      MockFile('some/ios/file_unittest.mm',
+               ['TEST_F(SomeTest, Whitelist) // nocheck', # nocheck
+                ' { V(allowlist); }']),
+      MockFile('some/doc/file.md',
+               ['Master in markdown <!-- nocheck -->', # nocheck
+                '## Subheading is okay']),
+      MockFile('some/java/file/TestJava.java',
+               ['class TestJava {',
+                '  public String master; // nocheck', # nocheck
+                '}']),
+      MockFile('some/html/file.html',
+               ['<-- an existing html multiline comment',
+                'says "master" here --><!-- nocheck -->', # nocheck
+                '<!-- in the comment -->'])
+    ]
+
+    errors = presubmit_canned_checks.CheckInclusiveLanguage(input_api,
+                                                            MockOutputApi())
+    self.assertEqual(0, len(errors))
+
+  def testTopLevelDirExcempt(self):
+    input_api = MockInputApi()
+    input_api.change.RepositoryRoot = lambda: ''
+    input_api.presubmit_local_path = ''
+
+    input_api.files = [
+      MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
+               '. 2 1',
+               'some/other/dir 2 1',
+               ]),
+      MockFile('presubmit_canned_checks_test.py',
+               ['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
+                '}']),
+      MockFile('presubmit_canned_checks.py',
+               ['- (void)testSth { V(whitelist); } // nocheck']), # nocheck
+    ]
+
+    errors = presubmit_canned_checks.CheckInclusiveLanguage(input_api,
+                                                            MockOutputApi())
+    self.assertEqual(1, len(errors))
+    self.assertTrue('presubmit_canned_checks_test.py' in errors[0].message)
+    self.assertTrue('presubmit_canned_checks.py' not in errors[0].message)
+
+  def testChangeIsForSomeOtherRepo(self):
+    input_api = MockInputApi()
+    input_api.change.RepositoryRoot = lambda: 'v8'
+    input_api.presubmit_local_path = ''
+
+    input_api.files = [
+      MockFile('some_file', [
+               '# this is a blacklist', # nocheck
+               ]),
+    ]
+    errors = presubmit_canned_checks.CheckInclusiveLanguage(input_api,
+                                                            MockOutputApi())
+    self.assertEqual([], errors)
+
+
+if __name__ == '__main__':
+  unittest.main()

presubmit_canned_checks_test_mocks.py

@@ -0,0 +1,259 @@
+# Copyright 2021 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+from collections import defaultdict
+import fnmatch
+import json
+import logging
+import os
+import re
+import subprocess
+import sys
+
+from presubmit_canned_checks import _ReportErrorFileAndLine
+
+
+class MockCannedChecks(object):
+  def _FindNewViolationsOfRule(self, callable_rule, input_api,
+                               source_file_filter=None,
+                               error_formatter=_ReportErrorFileAndLine):
+    """Find all newly introduced violations of a per-line rule (a callable).
+
+    Arguments:
+      callable_rule: a callable taking a file extension and line of input and
+        returning True if the rule is satisfied and False if there was a
+        problem.
+      input_api: object to enumerate the affected files.
+      source_file_filter: a filter to be passed to the input api.
+      error_formatter: a callable taking (filename, line_number, line) and
+        returning a formatted error string.
+
+    Returns:
+      A list of the newly-introduced violations reported by the rule.
+    """
+    errors = []
+    for f in input_api.AffectedFiles(include_deletes=False,
+                                     file_filter=source_file_filter):
+      # For speed, we do two passes, checking first the full file.  Shelling out
+      # to the SCM to determine the changed region can be quite expensive on
+      # Win32.  Assuming that most files will be kept problem-free, we can
+      # skip the SCM operations most of the time.
+      extension = str(f.LocalPath()).rsplit('.', 1)[-1]
+      if all(callable_rule(extension, line) for line in f.NewContents()):
+        continue  # No violation found in full text: can skip considering diff.
+
+      for line_num, line in f.ChangedContents():
+        if not callable_rule(extension, line):
+          errors.append(error_formatter(f.LocalPath(), line_num, line))
+
+    return errors
+
+
+class MockInputApi(object):
+  """Mock class for the InputApi class.
+
+  This class can be used for unittests for presubmit by initializing the files
+  attribute as the list of changed files.
+  """
+
+  DEFAULT_FILES_TO_SKIP = ()
+
+  def __init__(self):
+    self.canned_checks = MockCannedChecks()
+    self.fnmatch = fnmatch
+    self.json = json
+    self.re = re
+    self.os_path = os.path
+    self.platform = sys.platform
+    self.python_executable = sys.executable
+    self.platform = sys.platform
+    self.subprocess = subprocess
+    self.sys = sys
+    self.files = []
+    self.is_committing = False
+    self.change = MockChange([])
+    self.presubmit_local_path = os.path.dirname(__file__)
+    self.logging = logging.getLogger('PRESUBMIT')
+
+  def CreateMockFileInPath(self, f_list):
+    self.os_path.exists = lambda x: x in f_list
+
+  def AffectedFiles(self, file_filter=None, include_deletes=False):
+    for file in self.files: # pylint: disable=redefined-builtin
+      if file_filter and not file_filter(file):
+        continue
+      if not include_deletes and file.Action() == 'D':
+        continue
+      yield file
+
+  def AffectedSourceFiles(self, file_filter=None):
+    return self.AffectedFiles(file_filter=file_filter)
+
+  def FilterSourceFile(self, file, # pylint: disable=redefined-builtin
+                       files_to_check=(), files_to_skip=()):
+    local_path = file.LocalPath()
+    found_in_files_to_check = not files_to_check
+    if files_to_check:
+      if isinstance(files_to_check, str):
+        raise TypeError('files_to_check should be an iterable of strings')
+      for pattern in files_to_check:
+        compiled_pattern = re.compile(pattern)
+        if compiled_pattern.search(local_path):
+          found_in_files_to_check = True
+          break
+    if files_to_skip:
+      if isinstance(files_to_skip, str):
+        raise TypeError('files_to_skip should be an iterable of strings')
+      for pattern in files_to_skip:
+        compiled_pattern = re.compile(pattern)
+        if compiled_pattern.search(local_path):
+          return False
+    return found_in_files_to_check
+
+  def LocalPaths(self):
+    return [file.LocalPath() for file in self.files] # pylint: disable=redefined-builtin
+
+  def PresubmitLocalPath(self):
+    return self.presubmit_local_path
+
+  def ReadFile(self, filename, mode='rU'):
+    if hasattr(filename, 'AbsoluteLocalPath'):
+       filename = filename.AbsoluteLocalPath()
+    for file_ in self.files:
+      if file_.LocalPath() == filename:
+        return '\n'.join(file_.NewContents())
+    # Otherwise, file is not in our mock API.
+    raise IOError, "No such file or directory: '%s'" % filename
+
+
+class MockOutputApi(object):
+  """Mock class for the OutputApi class.
+
+  An instance of this class can be passed to presubmit unittests for outputing
+  various types of results.
+  """
+
+  class PresubmitResult(object):
+    def __init__(self, message, items=None, long_text=''):
+      self.message = message
+      self.items = items
+      self.long_text = long_text
+
+    def __repr__(self):
+      return self.message
+
+  class PresubmitError(PresubmitResult):
+    def __init__(self, message, items=None, long_text=''):
+      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 PresubmitNotifyResult(PresubmitResult):
+    def __init__(self, message, items=None, long_text=''):
+      MockOutputApi.PresubmitResult.__init__(self, message, items, long_text)
+      self.type = 'notify'
+
+  class PresubmitPromptOrNotify(PresubmitResult):
+    def __init__(self, message, items=None, long_text=''):
+      MockOutputApi.PresubmitResult.__init__(self, message, items, long_text)
+      self.type = 'promptOrNotify'
+
+  def __init__(self):
+    self.more_cc = []
+
+  def AppendCC(self, more_cc):
+    self.more_cc.extend(more_cc)
+
+
+class MockFile(object):
+  """Mock class for the File class.
+
+  This class can be used to form the mock list of changed files in
+  MockInputApi for presubmit unittests.
+  """
+
+  def __init__(self, local_path, new_contents, old_contents=None, action='A',
+               scm_diff=None):
+    self._local_path = local_path
+    self._new_contents = new_contents
+    self._changed_contents = [(i + 1, l) for i, l in enumerate(new_contents)]
+    self._action = action
+    if scm_diff:
+      self._scm_diff = scm_diff
+    else:
+      self._scm_diff = (
+        "--- /dev/null\n+++ %s\n@@ -0,0 +1,%d @@\n" %
+            (local_path, len(new_contents)))
+      for l in new_contents:
+        self._scm_diff += "+%s\n" % l
+    self._old_contents = old_contents
+
+  def Action(self):
+    return self._action
+
+  def ChangedContents(self):
+    return self._changed_contents
+
+  def NewContents(self):
+    return self._new_contents
+
+  def LocalPath(self):
+    return self._local_path
+
+  def AbsoluteLocalPath(self):
+    return self._local_path
+
+  def GenerateScmDiff(self):
+    return self._scm_diff
+
+  def OldContents(self):
+    return self._old_contents
+
+  def rfind(self, p):
+    """os.path.basename is called on MockFile so we need an rfind method."""
+    return self._local_path.rfind(p)
+
+  def __getitem__(self, i):
+    """os.path.basename is called on MockFile so we need a get method."""
+    return self._local_path[i]
+
+  def __len__(self):
+    """os.path.basename is called on MockFile so we need a len method."""
+    return len(self._local_path)
+
+  def replace(self, altsep, sep):
+    """os.path.basename is called on MockFile so we need a replace method."""
+    return self._local_path.replace(altsep, sep)
+
+
+class MockAffectedFile(MockFile):
+  def AbsoluteLocalPath(self):
+    return self._local_path
+
+
+class MockChange(object):
+  """Mock class for Change class.
+
+  This class can be used in presubmit unittests to mock the query of the
+  current change.
+  """
+
+  def __init__(self, changed_files):
+    self._changed_files = changed_files
+    self.footers = defaultdict(list)
+
+  def LocalPaths(self):
+    return self._changed_files
+
+  def AffectedFiles(self, include_dirs=False, include_deletes=True,
+                    file_filter=None):
+    return self._changed_files
+
+  def GitFootersFromDescription(self):
+    return self.footers
+