Add the ability to give advice on forbidden identifiers.

Add addvice for base::Bind*

Currently we are told not to use something, but it's not always clear what
we should use instead. This allows adding advice for the correct alternative
to the forbidden identifies.

This reapplies 6592bd30, fixes a bug
in that and adds a test to validate the compiled config.

Change-Id: I4ac12f7748be3f7fbcc03de36e44f35a15cb033e
Reviewed-on: https://chromium-review.googlesource.com/c/1260127Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597037}

third_party/blink/PRESUBMIT.py

@@ -166,9 +166,12 @@ def _CheckForForbiddenChromiumCode(input_api, output_api):
         if errors:
             errors = audit_non_blink_usage.check(path, f.ChangedContents())
             if errors:
-                for line_number, disallowed_identifier in errors:
-                    results.append(output_api.PresubmitError(
-                        '%s:%d uses disallowed identifier %s' % (path, line_number, disallowed_identifier)))
+                for error in errors:
+                    msg = '%s:%d uses disallowed identifier %s' % (
+                        path, error.line, error.identifier)
+                    if error.advice:
+                        msg += ". Advice: %s" % "\n".join(error.advice)
+                    results.append(output_api.PresubmitError(msg))
     return results
 
 

third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py

@@ -266,7 +266,11 @@ _CONFIG = [
             # Blink uses UKM for logging e.g. always-on leak detection (crbug/757374)
             'ukm::.+',
         ],
-        'disallowed': ['.+'],
+        'disallowed': [
+            '.+',
+            ('base::Bind(|Once|Repeating)',
+             'Use WTF::Bind or WTF::BindRepeating.'),
+        ],
     },
     {
         'paths': ['third_party/blink/renderer/bindings/'],
@@ -477,12 +481,30 @@ def _precompile_config():
             return re.compile('(?:%s)$' % '|'.join(match_list))
         return match_nothing_re
 
+    def compile_disallowed(disallowed_list):
+        """Transforms the disallowed list to one with the regexps compiled."""
+        if not disallowed_list:
+            return match_nothing_re, []
+        match_list = []
+        advice_list = []
+        for entry in disallowed_list:
+            if isinstance(entry, tuple):
+                match, advice = entry
+                match_list.append(match)
+                advice_list.append((compile_regexp(match), advice))
+            else:
+                # Just a string
+                match_list.append(entry)
+        return compile_regexp(match_list), advice_list
+
     compiled_config = []
     for raw_entry in _CONFIG:
+        disallowed, advice = compile_disallowed(raw_entry.get('disallowed'))
         compiled_config.append({
             'paths': raw_entry['paths'],
             'allowed': compile_regexp(raw_entry.get('allowed')),
-            'disallowed': compile_regexp(raw_entry.get('disallowed')),
+            'disallowed': disallowed,
+            'advice': advice,
         })
     return compiled_config
 
@@ -506,9 +528,10 @@ def _find_matching_entries(path):
 
     Returns:
         A list of entries, sorted in order of relevance. Each entry is a
-        dictionary with two keys:
+        dictionary with keys:
             allowed: A regexp for identifiers that should be allowed.
             disallowed: A regexp for identifiers that should not be allowed.
+            advice: (optional) A regexp for identifiers along with advice
     """
     entries = []
     for entry in _COMPILED_CONFIG:
@@ -522,6 +545,7 @@ def _find_matching_entries(path):
 
 
 def _check_entries_for_identifier(entries, identifier):
+    """Check if an identifier is allowed"""
     for entry in entries:
         if entry['allowed'].match(identifier):
             return True
@@ -531,6 +555,23 @@ def _check_entries_for_identifier(entries, identifier):
     return False
 
 
+def _find_advice_for_identifier(entries, identifier):
+    advice_list = []
+    for entry in entries:
+        for matcher, advice in entry.get('advice', []):
+            if matcher.match(identifier):
+                advice_list.append(advice)
+    return advice_list
+
+
+class BadIdentifier(object):
+    """Represents a single instance of a bad identifier."""
+    def __init__(self, identifier, line, advice=None):
+        self.identifier = identifier
+        self.line = line
+        self.advice = advice
+
+
 def check(path, contents):
     """Checks for disallowed usage of non-Blink classes, functions, et cetera.
 
@@ -539,7 +580,7 @@ def check(path, contents):
         contents: An array of line number, line tuples to check.
 
     Returns:
-        A list of line number, disallowed identifier tuples.
+        A list of (line number, disallowed identifier, advice) tuples.
     """
     results = []
     # Because Windows.
@@ -563,8 +604,11 @@ def check(path, contents):
             line = line[:idx]
         match = _IDENTIFIER_WITH_NAMESPACE_RE.search(line)
         if match:
-            if not _check_entries_for_identifier(entries, match.group(0)):
-                results.append((line_number, match.group(0)))
+            identifier = match.group(0)
+            if not _check_entries_for_identifier(entries, identifier):
+                advice = _find_advice_for_identifier(entries, identifier)
+                results.append(
+                    BadIdentifier(identifier, line_number, advice))
     return results
 
 
@@ -579,7 +623,7 @@ def main():
                 if disallowed_identifiers:
                     print '%s uses disallowed identifiers:' % path
                     for i in disallowed_identifiers:
-                        print i
+                        print (i.line, i.identifier, i.advice)
         except IOError as e:
             print 'could not open %s: %s' % (path, e)
 

third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage_test.py

@@ -23,6 +23,9 @@ class TestAuditNonBlinkUsageTest(unittest.TestCase):
                 self.assertIsInstance(entry['allowed'], self._REGEXP_CLASS)
             if 'disallowed' in entry:
                 self.assertIsInstance(entry['disallowed'], self._REGEXP_CLASS)
+            for match, advice in entry.get('advice', []):
+                self.assertIsInstance(match, self._REGEXP_CLASS)
+                self.assertIsInstance(advice, str)
 
 
 if __name__ == '__main__':