pythonstyle: Fix suppression support; improve SyntaxError reporting; …

…Only report each nit once

This patch cleans up a number of issues I encountered while trying to fix suppression support in the Python style checks.

* The suppression option wasn't working as expected because the file names passed to it were changed from relative to absolute. This moves parsing into get_nits so that it has access to the relative file path.
* SyntaxErrors show only the default error message without any source context. This patch adds source context display to syntax errors.
* Multiline nits are counted and outputed once for each line they contain. This changes it so that they are only counted and logged once.

The one change I want to call out specifically is that I changed Nit so that it doesn't know about the PythonFile anymore. I did this so that I could make a nit for SyntaxErrors without introducing a variant of PythonFile for syntactically incorrect Python files.

If there are external plugins that rely on Nit's current structure, this may break them.

Testing Done:
Wrote a test covering using suppression, and fixed it. Then added more tests around syntax error handling and fixed those.

After doing some testing, I noticed the multi-line display issue and fixed that as well following the same process.

CI away on the PR.

Bugs closed: 3128, 3139

Reviewed at https://rbcommons.com/s/twitter/r/3647/

closes pantsbuild#3128
closes pantsbuild#3139

contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/checker.py

@@ -5,7 +5,6 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-import os
 import re
 from collections import namedtuple
 
@@ -15,7 +14,7 @@
 from pants.base.exceptions import TaskError
 from pants.option.custom_types import file_option
 
-from pants.contrib.python.checks.tasks.checkstyle.common import Nit, PythonFile
+from pants.contrib.python.checks.tasks.checkstyle.common import CheckSyntaxError, Nit, PythonFile
 from pants.contrib.python.checks.tasks.checkstyle.file_excluder import FileExcluder
 from pants.contrib.python.checks.tasks.checkstyle.register_plugins import register_plugins
 
@@ -32,8 +31,8 @@ def checker(self, python_file):
     return self.subsystem.global_instance().get_plugin(python_file)
 
 
-def noqa_line_filter(python_file, line_number):
-  return _NOQA_LINE_SEARCH(python_file.lines[line_number]) is not None
+def line_contains_noqa(line):
+  return _NOQA_LINE_SEARCH(line) is not None
 
 
 def noqa_file_filter(python_file):
@@ -93,56 +92,57 @@ def register_plugin(cls, name, subsystem):
     cls._plugins.append(plugin)
     cls._subsystems += (plugin.subsystem, )
 
-  def get_nits(self, python_file):
+  def get_nits(self, filename):
     """Iterate over the instances style checker and yield Nits.
 
-    :param python_file: PythonFile Object
+    :param filename: str pointing to a file within the buildroot.
     """
+    try:
+      python_file = PythonFile.parse(filename, root=get_buildroot())
+    except CheckSyntaxError as e:
+      yield e.as_nit()
+      return
+
     if noqa_file_filter(python_file):
       return
 
     if self.options.suppress:
       # Filter out any suppressed plugins
       check_plugins = [plugin for plugin in self._plugins
-                       if self.excluder.should_include(python_file.filename, plugin.name)]
+                       if self.excluder.should_include(filename, plugin.name)]
     else:
       check_plugins = self._plugins
 
     for plugin in check_plugins:
-      for nit in plugin.checker(python_file):
-        if nit._line_number is None:
+
+      for i, nit in enumerate(plugin.checker(python_file)):
+        if i == 0:
+          # NB: Add debug log header for nits from each plugin, but only if there are nits from it.
+          self.context.log.debug('Nits from plugin {} for {}'.format(plugin.name, filename))
+
+        if not nit.has_lines_to_display:
           yield nit
           continue
 
-        nit_slice = python_file.line_range(nit._line_number)
-        for line_number in range(nit_slice.start, nit_slice.stop):
-          if noqa_line_filter(python_file, line_number):
-            break
-          else:
-            yield nit
+        if all(not line_contains_noqa(line) for line in nit.lines):
+          yield nit
 
   def check_file(self, filename):
     """Process python file looking for indications of problems.
 
     :param filename: (str) Python source filename
     :return: (int) number of failures
     """
-    try:
-      python_file = PythonFile.parse(os.path.join(get_buildroot(), filename))
-    except SyntaxError as e:
-      print('{filename}:SyntaxError: {error}'.format(filename=filename, error=e))
-      return 1
-
     # If the user specifies an invalid severity use comment.
-    severity = Nit.SEVERITY.get(self.options.severity, Nit.COMMENT)
+    log_threshold = Nit.SEVERITY.get(self.options.severity, Nit.COMMENT)
 
     failure_count = 0
     fail_threshold = Nit.WARNING if self.options.strict else Nit.ERROR
 
-    for i, nit in enumerate(self.get_nits(python_file)):
+    for i, nit in enumerate(self.get_nits(filename)):
       if i == 0:
         print()  # Add an extra newline to clean up the output only if we have nits.
-      if nit.severity >= severity:
+      if nit.severity >= log_threshold:
         print('{nit}\n'.format(nit=nit))
       if nit.severity >= fail_threshold:
         failure_count += 1

contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/common.py

@@ -8,6 +8,7 @@
 import ast
 import codecs
 import itertools
+import os
 import re
 import textwrap
 import tokenize
@@ -68,7 +69,17 @@ class PythonFile(object):
   """Checkstyle wrapper for Python source files."""
   SKIP_TOKENS = frozenset((tokenize.COMMENT, tokenize.NL, tokenize.DEDENT))
 
-  def _remove_coding_header(self, blob):
+  @classmethod
+  def _parse(cls, blob, filename):
+    blob_minus_coding_header = cls._remove_coding_header(blob)
+    try:
+      tree = ast.parse(blob_minus_coding_header, filename)
+    except SyntaxError as e:
+      raise CheckSyntaxError(e, blob, filename)
+    return tree
+
+  @classmethod
+  def _remove_coding_header(cls, blob):
     """
     There is a bug in ast.parse that cause it to throw a syntax error if
     you have a header similar to...
@@ -85,9 +96,9 @@ def _remove_coding_header(self, blob):
       lines[0] = '#remove coding'
     return '\n'.join(lines).encode('ascii', errors='replace')
 
-  def __init__(self, blob, filename='<expr>'):
+  def __init__(self, blob, tree, filename):
     self._blob = self._remove_coding_header(blob)
-    self.tree = ast.parse(self._blob, filename)
+    self.tree = tree
     self.lines = OffByOneList(self._blob.split('\n'))
     self.filename = filename
     self.logical_lines = dict((start, (start, stop, indent))
@@ -103,21 +114,39 @@ def __str__(self):
     return 'PythonFile({filename})'.format(filename=self.filename)
 
   @classmethod
-  def parse(cls, filename):
-    with codecs.open(filename, encoding='utf-8') as fp:
+  def parse(cls, filename, root=None):
+    """Parses the file at filename and returns a PythonFile.
+
+    If root is specified, it will open the file with root prepended to the path. The idea is to
+    allow for errors to contain a friendlier file path than the full absolute path.
+    """
+    if root is not None:
+      if os.path.isabs(filename):
+        raise ValueError("filename must be a relative path if root is specified")
+      full_filename = os.path.join(root, filename)
+    else:
+      full_filename = filename
+
+    with codecs.open(full_filename, encoding='utf-8') as fp:
       blob = fp.read()
-    return cls(blob, filename)
+
+    tree = cls._parse(blob, filename)
+
+    return cls(blob, tree, filename)
 
   @classmethod
-  def from_statement(cls, statement):
+  def from_statement(cls, statement, filename='<expr>'):
     """A helper to construct a PythonFile from a triple-quoted string, for testing.
     :param statement: Python file contents
     :return: Instance of PythonFile
     """
     lines = textwrap.dedent(statement).split('\n')
     if lines and not lines[0]:  # Remove the initial empty line, which is an artifact of dedent.
       lines = lines[1:]
-    return cls('\n'.join(lines))
+
+    blob = '\n'.join(lines)
+    tree = cls._parse(blob, filename)
+    return cls(blob, tree, filename)
 
   @classmethod
   def iter_tokens(cls, blob):
@@ -229,16 +258,17 @@ class Nit(object):
   def flatten_lines(*line_or_line_list):
     return itertools.chain(*line_or_line_list)
 
-  def __init__(self, code, severity, python_file, message, line_number=None):
+  def __init__(self, code, severity, filename, message, line_range=None, lines=None):
     if severity not in self.SEVERITY:
       raise ValueError('Severity should be one of {}'.format(' '.join(self.SEVERITY.values())))
-    self.python_file = python_file
     if not re.match(r'[A-Z]\d{3}', code):
       raise ValueError('Code must contain a prefix letter followed by a 3 digit number')
+    self.filename = filename
     self.code = code
     self.severity = severity
     self._message = message
-    self._line_number = line_number
+    self._line_range = line_range
+    self._lines = lines
 
   def __str__(self):
     """convert ascii for safe terminal output"""
@@ -247,8 +277,8 @@ def __str__(self):
 
   @property
   def line_number(self):
-    if self._line_number:
-      line_range = self.python_file.line_range(self._line_number)
+    if self._line_range:
+      line_range = self._line_range
       if line_range.stop - line_range.start > 1:
         return '%03d-%03d' % (line_range.start, line_range.stop - 1)
       else:
@@ -259,13 +289,33 @@ def message(self):
     return '{code}:{severity:<7} {filename}:{linenum} {message}'.format(
         code=self.code,
         severity=self.SEVERITY[self.severity],
-        filename=self.python_file.filename,
+        filename=self.filename,
         linenum=self.line_number or '*',
         message=self._message)
 
   @property
   def lines(self):
-    return self.python_file[self._line_number] if self._line_number else []
+    return self._lines or []
+
+  @property
+  def has_lines_to_display(self):
+    return len(self.lines) > 0
+
+
+class CheckSyntaxError(Exception):
+  def __init__(self, syntax_error, blob, filename):
+    self.filename = filename
+    self._blob = blob
+    self._syntax_error = syntax_error
+
+  def as_nit(self):
+    line_range = slice(self._syntax_error.lineno, self._syntax_error.lineno + 1)
+    lines = OffByOneList(self._blob.split('\n'))
+    # NB: E901 is the SyntaxError PEP8 code.
+    # See:http://pep8.readthedocs.org/en/latest/intro.html#error-codes
+    return Nit('E901', Nit.ERROR, self.filename,
+               'SyntaxError: {error}'.format(error=self._syntax_error.msg),
+               line_range=line_range, lines=lines)
 
 
 class CheckstylePlugin(AbstractClass):
@@ -302,7 +352,17 @@ def nit(self, code, severity, message, line_number_or_ast=None):
       line_number = line_number_or_ast
     elif isinstance(line_number_or_ast, ast.AST):
       line_number = getattr(line_number_or_ast, 'lineno', None)
-    return Nit(code, severity, self.python_file, message, line_number)
+
+    if line_number:
+      line_range = self.python_file.line_range(line_number)
+      lines = self.python_file[line_number]
+    else:
+      line_range = None
+      lines = None
+
+    return Nit(code, severity, self.python_file.filename, message,
+               line_range=line_range,
+               lines=lines)
 
   def comment(self, code, message, line_number_or_ast=None):
     return self.nit(code, Nit.COMMENT, message, line_number_or_ast)

contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/pyflakes.py

@@ -31,12 +31,14 @@ class FlakeError(Nit):
   }
 
   def __init__(self, python_file, flake_message):
+    line_range = python_file.line_range(flake_message.lineno)
     super(FlakeError, self).__init__(
         self.get_error_code(flake_message),
         Nit.ERROR,
-        python_file,
+        python_file.filename,
         flake_message.message % flake_message.message_args,
-        flake_message.lineno)
+        line_range,
+        python_file.lines[line_range])
 
   @classmethod
   def get_error_code(cls, message):

contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/plugin_test_base.py

@@ -31,7 +31,7 @@ def assertNit(self, file_content, expected_code, expected_severity=Nit.ERROR,
     self.assertEqual(expected_code, nits[0].code)
     self.assertEqual(expected_severity, nits[0].severity)
     if expected_line_number is not None:
-      self.assertEqual(expected_line_number, nits[0]._line_number)
+      self.assertEqual(expected_line_number, nits[0].line_number)
 
   def assertNoNits(self, file_content):
     plugin = self.get_plugin(file_content)

contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_checker.py

@@ -54,6 +54,19 @@ def test_failure(self):
       task.execute()
     self.assertIn('1 Python Style issues found', str(task_error.exception))
 
+  def test_suppressed_file_passes(self):
+    self.create_file('a/python/fail.py', contents=dedent("""
+                         print 'Print should not be used as a statement'
+                       """))
+    suppression_file = self.create_file('suppress.txt', contents=dedent("""
+    a/python/fail\.py::print-statements"""))
+    target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py'])
+    self.set_options(suppress=suppression_file)
+    context = self.context(target_roots=[target], )
+    task = self.create_task(context)
+
+    self.assertEqual(0, task.execute())
+
   def test_failure_fail_false(self):
     self.create_file('a/python/fail.py', contents=dedent("""
                        print 'Print should not be used as a statement'
@@ -72,4 +85,57 @@ def test_syntax_error(self):
     self.set_options(fail=False)
     context = self.context(target_roots=[target])
     task = self.create_task(context)
+
     self.assertEqual(1, task.execute())
+
+  def test_failure_print_nit(self):
+    self.create_file('a/python/fail.py', contents=dedent("""
+                         print 'Print should not be used as a statement'
+                       """))
+    target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py'])
+    context = self.context(target_roots=[target])
+    task = self.create_task(context)
+
+    nits = list(task.get_nits('a/python/fail.py'))
+
+    self.assertEqual(1, len(nits))
+    self.assertEqual(
+      """T607:ERROR   a/python/fail.py:002 Print used as a statement.\n"""
+      """     |print 'Print should not be used as a statement'""",
+      str(nits[0]))
+
+  def test_syntax_error_nit(self):
+    self.create_file('a/python/error.py', contents=dedent("""
+                         invalid python
+                       """))
+    target = self.make_target('a/python:error', PythonLibrary, sources=['error.py'])
+    self.set_options(fail=False)
+    context = self.context(target_roots=[target])
+    task = self.create_task(context)
+
+    nits = list(task.get_nits('a/python/error.py'))
+
+    self.assertEqual(1, len(nits))
+    self.assertEqual("""E901:ERROR   a/python/error.py:002 SyntaxError: invalid syntax\n"""
+                     """     |\n"""
+                     """     |invalid python\n"""
+                     """     |""",
+                     str(nits[0]))
+
+  def test_multiline_nit_printed_only_once(self):
+    self.create_file('a/python/fail.py', contents=dedent("""
+                         print ('Multi'
+                                'line') + 'expression'
+                       """))
+    target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py'])
+    context = self.context(target_roots=[target])
+    task = self.create_task(context)
+
+    nits = list(task.get_nits('a/python/fail.py'))
+
+    self.assertEqual(1, len(nits))
+    self.assertEqual(
+      """T607:ERROR   a/python/fail.py:002-003 Print used as a statement.\n"""
+      """     |print ('Multi'\n"""
+      """     |       'line') + 'expression'""",
+      str(nits[0]))