Improve no-long-lines check for Python files.

The previous implementation of CheckLongLines did not
handle global pylint disable/enable directives properly,
i.e. the difference between:

   # pylint: disable=line-too-long
   .... checks disabled for all lines here.
   # pylint: enable=line-too-long

versus:

   # Check only disabled for the line below
   some python statements   # pylint: disable=line-too-long

This CL changes the implementation to support Python files
properly. Note that in order to not disturb the mock-based
unit-tests, a new function is introduced to be able to
filter the list of affected files based on their file
extension.

BUG=890734
R=mattcary@chromium.org,ehmaldonado@chromium.org,dpranke@chromium.org

Change-Id: Id52deff53913b8d47a4157f42b1fffbd3b103201
Reviewed-on: https://chromium-review.googlesource.com/c/1396094Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>

presubmit_canned_checks.py

@@ -248,15 +248,38 @@ def _ReportErrorFileAndLine(filename, line_num, dummy_line):
   return '%s:%s' % (filename, line_num)
 
 
-def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None,
-                             error_formatter=_ReportErrorFileAndLine):
+def _GenerateAffectedFileExtList(input_api, source_file_filter):
+  """Generate a list of (file, extension) tuples from affected files.
+
+  The result can be fed to _FindNewViolationsOfRule() directly, or
+  could be filtered before doing that.
+
+  Args:
+    input_api: object to enumerate the affected files.
+    source_file_filter: a filter to be passed to the input api.
+  Yields:
+    A list of (file, extension) tuples, where |file| is an affected
+      file, and |extension| its file path extension.
+  """
+  for f in input_api.AffectedFiles(
+      include_deletes=False, file_filter=source_file_filter):
+    extension = str(f.LocalPath()).rsplit('.', 1)[-1]
+    yield (f, extension)
+
+
+def _FindNewViolationsOfRuleForList(callable_rule,
+                                    file_ext_list,
+                                    error_formatter=_ReportErrorFileAndLine):
   """Find all newly introduced violations of a per-line rule (a callable).
 
+  Prefer calling _FindNewViolationsOfRule() instead of this function, unless
+  the list of affected files need to be filtered in a special way.
+
   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.
+    file_ext_list: a list of input (file, extension) tuples, as returned by
+      _GenerateAffectedFileExtList().
     error_formatter: a callable taking (filename, line_number, line) and
       returning a formatted error string.
 
@@ -264,13 +287,11 @@ def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None,
     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 f, extension in file_ext_list:
     # 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.
 
@@ -281,6 +302,28 @@ def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None,
   return errors
 
 
+def _FindNewViolationsOfRule(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.
+  """
+  return _FindNewViolationsOfRuleForList(
+      callable_rule, _GenerateAffectedFileExtList(
+          input_api, source_file_filter), error_formatter)
+
+
 def CheckChangeHasNoTabs(input_api, output_api, source_file_filter=None):
   """Checks that there are no tab characters in any of the text files to be
   submitted.
@@ -385,11 +428,6 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None):
     if any((url in line) for url in ('file://', 'http://', 'https://')):
       return True
 
-    # If 'line-too-long' is explicitly suppressed for the line, any length is
-    # acceptable.
-    if 'pylint: disable=line-too-long' in line and file_extension == 'py':
-      return True
-
     if line_len > extra_maxlen:
       return False
 
@@ -402,12 +440,64 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None):
     return input_api.re.match(
         r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol, line)
 
+  def is_global_pylint_directive(line, pos):
+    """True iff the pylint directive starting at line[pos] is global."""
+    # Any character before |pos| that is not whitespace or '#' indidcates
+    # this is a local directive.
+    return not any([c not in " \t#" for c in line[:pos]])
+
+  def check_python_long_lines(affected_files, error_formatter):
+    errors = []
+    global_check_enabled = True
+
+    for f in affected_files:
+      file_path = f.LocalPath()
+      for idx, line in enumerate(f.NewContents()):
+        line_num = idx + 1
+        line_is_short = no_long_lines(PY_FILE_EXTS[0], line)
+
+        pos = line.find('pylint: disable=line-too-long')
+        if pos >= 0:
+          if is_global_pylint_directive(line, pos):
+            global_check_enabled = False  # Global disable
+          else:
+            continue  # Local disable.
+
+        do_check = global_check_enabled
+
+        pos = line.find('pylint: enable=line-too-long')
+        if pos >= 0:
+          if is_global_pylint_directive(line, pos):
+            global_check_enabled = True  # Global enable
+            do_check = True  # Ensure it applies to current line as well.
+          else:
+            do_check = True  # Local enable
+
+        if do_check and not line_is_short:
+          errors.append(error_formatter(file_path, line_num, line))
+
+    return errors
+
   def format_error(filename, line_num, line):
     return '%s, line %s, %s chars' % (filename, line_num, len(line))
 
-  errors = _FindNewViolationsOfRule(no_long_lines, input_api,
-                                    source_file_filter,
-                                    error_formatter=format_error)
+  file_ext_list = list(
+      _GenerateAffectedFileExtList(input_api, source_file_filter))
+
+  errors = []
+
+  # For non-Python files, a simple line-based rule check is enough.
+  non_py_file_ext_list = [x for x in file_ext_list if x[1] not in PY_FILE_EXTS]
+  if non_py_file_ext_list:
+    errors += _FindNewViolationsOfRuleForList(
+        no_long_lines, non_py_file_ext_list, error_formatter=format_error)
+
+  # However, Python files need more sophisticated checks that need parsing
+  # the whole source file.
+  py_file_list = [x[0] for x in file_ext_list if x[1] in PY_FILE_EXTS]
+  if py_file_list:
+    errors += check_python_long_lines(
+        py_file_list, error_formatter=format_error)
   if errors:
     msg = 'Found lines longer than %s characters (first 5 shown).' % maxlen
     return [output_api.PresubmitPromptWarning(msg, items=errors[:5])]

tests/presubmit_unittest.py

@@ -1743,6 +1743,38 @@ class CannedChecksUnittest(PresubmitTestsBase):
     self.assertEquals(len(results2), 1)
     self.assertEquals(results2[0].__class__, error_type)
 
+  def PythonLongLineTest(self, maxlen, content, should_pass):
+    """Runs a test of Python long-line checking rule.
+
+    Because ContentTest() cannot be used here due to the different code path
+    that the implementation of CheckLongLines() uses for Python files.
+
+    Args:
+      maxlen: Maximum line length for content.
+      content: Python source which is expected to pass or fail the test.
+      should_pass: True iff the test should pass, False otherwise.
+    """
+    change = presubmit.Change('foo1', 'foo1\n', self.fake_root_dir, None, 0, 0,
+                              None)
+    input_api = self.MockInputApi(change, False)
+    affected_file = self.mox.CreateMock(presubmit.GitAffectedFile)
+    input_api.AffectedFiles(
+        include_deletes=False,
+        file_filter=mox.IgnoreArg()).AndReturn([affected_file])
+    affected_file.LocalPath().AndReturn('foo.py')
+    affected_file.LocalPath().AndReturn('foo.py')
+    affected_file.NewContents().AndReturn(content.splitlines())
+
+    self.mox.ReplayAll()
+    results = presubmit_canned_checks.CheckLongLines(
+        input_api, presubmit.OutputApi, maxlen)
+    if should_pass:
+      self.assertEquals(results, [])
+    else:
+      self.assertEquals(len(results), 1)
+      self.assertEquals(results[0].__class__,
+                        presubmit.OutputApi.PresubmitPromptWarning)
+
   def ReadFileTest(self, check, content1, content2, error_type):
     change1 = presubmit.Change(
         'foo1', 'foo1\n', self.fake_root_dir, None, 0, 0, None)
@@ -1935,6 +1967,92 @@ class CannedChecksUnittest(PresubmitTestsBase):
                      'importSomething ' + 'A ' * 50, 'foo.java',
                      presubmit.OutputApi.PresubmitPromptWarning)
 
+  def testCannedCheckPythonLongLines(self):
+    # NOTE: Cannot use ContentTest() here because of the different code path
+    #       used for Python checks in CheckLongLines().
+    passing_cases = [
+        r"""
+01234568901234589012345689012345689
+A short line
+""",
+        r"""
+01234568901234589012345689012345689
+This line is too long but should pass # pylint: disable=line-too-long
+""",
+        r"""
+01234568901234589012345689012345689
+# pylint: disable=line-too-long
+This line is too long but should pass due to global disable
+""",
+        r"""
+01234568901234589012345689012345689
+   #pylint: disable=line-too-long
+This line is too long but should pass due to global disable.
+""",
+        r"""
+01234568901234589012345689012345689
+                       #    pylint: disable=line-too-long
+This line is too long but should pass due to global disable.
+""",
+        r"""
+01234568901234589012345689012345689
+# import is a valid exception
+import some.really.long.package.name.that.should.pass
+""",
+        r"""
+01234568901234589012345689012345689
+# from is a valid exception
+from some.really.long.package.name import passing.line
+""",
+        r"""
+01234568901234589012345689012345689
+                    import some.package
+""",
+        r"""
+01234568901234589012345689012345689
+                    from some.package import stuff
+""",
+    ]
+    for content in passing_cases:
+      self.PythonLongLineTest(40, content, should_pass=True)
+
+    failing_cases = [
+        r"""
+01234568901234589012345689012345689
+This line is definitely too long and should fail.
+""",
+        r"""
+01234568901234589012345689012345689
+# pylint: disable=line-too-long
+This line is too long and should pass due to global disable
+# pylint: enable=line-too-long
+But this line is too long and should still fail now
+""",
+        r"""
+01234568901234589012345689012345689
+# pylint: disable=line-too-long
+This line is too long and should pass due to global disable
+But this line is too long # pylint: enable=line-too-long
+""",
+        r"""
+01234568901234589012345689012345689
+This should fail because the global
+check is enabled on the next line.
+              #         pylint: enable=line-too-long
+""",
+        r"""
+01234568901234589012345689012345689
+# pylint: disable=line-too-long
+                  # pylint: enable-foo-bar should pass
+The following line should fail
+since global directives apply to
+the current line as well!
+                  # pylint: enable-line-too-long should fail
+""",
+    ]
+    for content in failing_cases[0:0]:
+      self.PythonLongLineTest(40, content, should_pass=False)
+
   def testCannedCheckJSLongLines(self):
     check = lambda x, y, _: presubmit_canned_checks.CheckLongLines(x, y, 10)
     self.ContentTest(check, 'GEN(\'#include "c/b/ui/webui/fixture.h"\');',