From 4a4e48158e62b6eacff0005a61014e9b6661fdfa Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Tue, 31 Jul 2018 01:08:09 -0400 Subject: [PATCH] Python 3 fixes - fix contrib/python checkstyle (#6274) Beyond the usual unicode vs bytes issues, the AST grammar changed substantially between Python 2 vs 3. For example, `ast.TryExcept` was renamed to `ast.Try`. See http://joao.npimentel.net/2015/07/23/python-2-vs-python-3-ast-differences/ for all the changes. --- .../python/checks/tasks/checkstyle/common.py | 19 ++++++++++--------- .../tasks/checkstyle/except_statements.py | 4 +++- .../checkstyle/missing_contextmanager.py | 10 ++++++++-- .../tasks/checkstyle/print_statements.py | 6 ++++++ .../tasks/checkstyle/plugin_test_base.py | 2 +- .../checks/tasks/checkstyle/test_checker.py | 18 +++++++++--------- .../checkstyle/test_except_statements.py | 16 ++++++++++------ .../tasks/checkstyle/test_file_excluder.py | 1 + .../tasks/checkstyle/test_import_order.py | 8 ++++---- .../tasks/checkstyle/test_print_statements.py | 4 ++++ 10 files changed, 56 insertions(+), 32 deletions(-) diff --git a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/common.py b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/common.py index e0dbf343998..a2e5ea67893 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/common.py +++ b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/common.py @@ -5,7 +5,6 @@ from __future__ import absolute_import, division, print_function, unicode_literals import ast -import codecs import itertools import os import re @@ -14,6 +13,7 @@ from abc import abstractmethod from builtins import object from collections import Sequence +from io import StringIO import six from pants.util.meta import AbstractClass @@ -91,7 +91,7 @@ def _remove_coding_header(cls, blob): """ # Remove the # coding=utf-8 to avoid AST erroneous parse errors # https://bugs.python.org/issue22221 - lines = blob.split('\n') + lines = blob.decode('utf-8').split('\n') if lines and 'coding=utf-8' in lines[0]: lines[0] = '#remove coding' return '\n'.join(lines).encode('ascii', errors='replace') @@ -99,7 +99,7 @@ def _remove_coding_header(cls, blob): def __init__(self, blob, tree, filename): self._blob = self._remove_coding_header(blob) self.tree = tree - self.lines = OffByOneList(self._blob.split('\n')) + self.lines = OffByOneList(self._blob.decode('utf-8').split('\n')) self.filename = filename self.logical_lines = dict((start, (start, stop, indent)) for start, stop, indent in self.iter_logical_lines(self._blob)) @@ -127,7 +127,7 @@ def parse(cls, filename, root=None): else: full_filename = filename - with codecs.open(full_filename, encoding='utf-8') as fp: + with open(full_filename, 'rb') as fp: blob = fp.read() tree = cls._parse(blob, filename) @@ -144,7 +144,7 @@ def from_statement(cls, statement, filename=''): if lines and not lines[0]: # Remove the initial empty line, which is an artifact of dedent. lines = lines[1:] - blob = '\n'.join(lines) + blob = '\n'.join(lines).encode('utf-8') tree = cls._parse(blob, filename) return cls(blob, tree, filename) @@ -154,7 +154,8 @@ def iter_tokens(cls, blob): :param blob: Input string with python file contents :return: token iterator """ - return tokenize.generate_tokens(six.StringIO(blob).readline) + readline_func = StringIO(blob.decode('utf-8')).readline + return tokenize.generate_tokens(readline_func) @property def tokens(self): @@ -271,9 +272,9 @@ def __init__(self, code, severity, filename, message, line_range=None, lines=Non self._lines = lines def __str__(self): - """convert ascii for safe terminal output""" + """Sanitize to Ascii for safe terminal output""" flat = list(self.flatten_lines([self.message], self.lines)) - return '\n |'.join(flat).encode('ascii', errors='replace') + return '\n |'.join(flat).encode('ascii', errors='replace').decode('ascii') @property def line_number(self): @@ -310,7 +311,7 @@ def __init__(self, syntax_error, blob, filename): def as_nit(self): line_range = slice(self._syntax_error.lineno, self._syntax_error.lineno + 1) - lines = OffByOneList(self._blob.split('\n')) + lines = OffByOneList(self._blob.decode('utf-8').split('\n')) # NB: E901 is the SyntaxError PEP8 code. # See:https://pycodestyle.readthedocs.io/en/latest/intro.html#error-codes return Nit('E901', Nit.ERROR, self.filename, diff --git a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/except_statements.py b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/except_statements.py index ec2f31fb94f..6839b77b318 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/except_statements.py +++ b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/except_statements.py @@ -6,6 +6,8 @@ import ast +from future.utils import PY3 + from pants.contrib.python.checks.tasks.checkstyle.common import CheckstylePlugin @@ -21,7 +23,7 @@ def blanket_excepts(cls, node): @classmethod def iter_excepts(cls, tree): for ast_node in ast.walk(tree): - if isinstance(ast_node, ast.TryExcept): + if isinstance(ast_node, ast.Try if PY3 else ast.TryExcept): yield ast_node def nits(self): diff --git a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/missing_contextmanager.py b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/missing_contextmanager.py index 883cb35b516..6d72ebe136c 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/missing_contextmanager.py +++ b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/missing_contextmanager.py @@ -6,6 +6,8 @@ import ast +from future.utils import PY3 + from pants.contrib.python.checks.tasks.checkstyle.common import CheckstylePlugin @@ -20,8 +22,12 @@ class MissingContextManager(CheckstylePlugin): def nits(self): with_contexts = set(self.iter_ast_types(ast.With)) - with_context_calls = set(node.context_expr for node in with_contexts - if isinstance(node.context_expr, ast.Call)) + # Grammar changed between Python 2 vs Python 3 to access the with statement's surrounding expressions. + # Refer to http://joao.npimentel.net/2015/07/23/python-2-vs-python-3-ast-differences/. + with_context_exprs = (set(node.context_expr for with_context in with_contexts for node in with_context.items) + if PY3 else + set(node.context_expr for node in with_contexts)) + with_context_calls = set(expr for expr in with_context_exprs if isinstance(expr, ast.Call)) for call in self.iter_ast_types(ast.Call): if isinstance(call.func, ast.Name) and call.func.id == 'open' \ diff --git a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/print_statements.py b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/print_statements.py index e77b79427e6..edcbe13e1c8 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/print_statements.py +++ b/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/print_statements.py @@ -7,6 +7,8 @@ import ast import re +from future.utils import PY3 + from pants.contrib.python.checks.tasks.checkstyle.common import CheckstylePlugin @@ -16,6 +18,10 @@ class PrintStatements(CheckstylePlugin): FUNCTIONY_EXPRESSION = re.compile(r'^\s*\(.*\)\s*$') def nits(self): + if PY3: + # Python 3 interpreter will raise SyntaxError upon reading a print statement. + # So, this module cannot be meaningfully used when ran with a Python 3 interpreter. + return for print_stmt in self.iter_ast_types(ast.Print): # In Python 3.x and in 2.x with __future__ print_function, prints show up as plain old # function expressions. ast.Print does not exist in Python 3.x. However, allow use diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/plugin_test_base.py b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/plugin_test_base.py index ff2b7999479..3b34e197213 100644 --- a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/plugin_test_base.py +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/plugin_test_base.py @@ -27,7 +27,7 @@ def file_required(self): def create_python_file(self, file_content): if self.file_required: tmpdir = safe_mkdtemp() - with open(os.path.join(tmpdir, 'file.py'), 'wb') as fp: + with open(os.path.join(tmpdir, 'file.py'), 'w') as fp: fp.write(file_content) fp.close() return PythonFile.parse(fp.name) diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_checker.py b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_checker.py index 2c1172212ea..3a1c94f14ab 100644 --- a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_checker.py +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_checker.py @@ -35,7 +35,7 @@ def test_no_sources(self): self.assertEqual(None, task.execute()) def test_pass(self): - self.create_file('a/python/pass.py', contents=dedent(""" + self.create_file('a/python/pass.py', mode='w', contents=dedent(""" print('Print is a function') """)) target = self.make_target('a/python:pass', PythonLibrary, sources=['pass.py']) @@ -44,7 +44,7 @@ def test_pass(self): self.assertEqual(0, task.execute()) def test_failure(self): - self.create_file('a/python/fail.py', contents=dedent(""" + self.create_file('a/python/fail.py', mode='w', contents=dedent(""" print 'Print should not be used as a statement' """)) target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) @@ -55,10 +55,10 @@ def test_failure(self): 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(""" + self.create_file('a/python/fail.py', mode='w', contents=dedent(""" print 'Print should not be used as a statement' """)) - suppression_file = self.create_file('suppress.txt', contents=dedent(""" + suppression_file = self.create_file('suppress.txt', mode='w', 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) @@ -68,7 +68,7 @@ def test_suppressed_file_passes(self): self.assertEqual(0, task.execute()) def test_failure_fail_false(self): - self.create_file('a/python/fail.py', contents=dedent(""" + self.create_file('a/python/fail.py', mode='w', contents=dedent(""" print 'Print should not be used as a statement' """)) target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) @@ -78,7 +78,7 @@ def test_failure_fail_false(self): self.assertEqual(1, task.execute()) def test_syntax_error(self): - self.create_file('a/python/error.py', contents=dedent(""" + self.create_file('a/python/error.py', mode='w', contents=dedent(""" invalid python """)) target = self.make_target('a/python:error', PythonLibrary, sources=['error.py']) @@ -89,7 +89,7 @@ def test_syntax_error(self): self.assertEqual(1, task.execute()) def test_failure_print_nit(self): - self.create_file('a/python/fail.py', contents=dedent(""" + self.create_file('a/python/fail.py', mode='w', contents=dedent(""" print 'Print should not be used as a statement' """)) target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) @@ -105,7 +105,7 @@ def test_failure_print_nit(self): str(nits[0])) def test_syntax_error_nit(self): - self.create_file('a/python/error.py', contents=dedent(""" + self.create_file('a/python/error.py', mode='w', contents=dedent(""" invalid python """)) target = self.make_target('a/python:error', PythonLibrary, sources=['error.py']) @@ -123,7 +123,7 @@ def test_syntax_error_nit(self): str(nits[0])) def test_multiline_nit_printed_only_once(self): - self.create_file('a/python/fail.py', contents=dedent(""" + self.create_file('a/python/fail.py', mode='w', contents=dedent(""" print ('Multi' 'line') + 'expression' """)) diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_except_statements.py b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_except_statements.py index f08bd5ae921..e04ba9e7ab1 100644 --- a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_except_statements.py +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_except_statements.py @@ -7,6 +7,7 @@ from pants_test.contrib.python.checks.tasks.checkstyle.plugin_test_base import \ CheckstylePluginTestBase +from pants.contrib.python.checks.tasks.checkstyle.common import CheckSyntaxError from pants.contrib.python.checks.tasks.checkstyle.except_statements import ExceptStatements @@ -25,12 +26,15 @@ def test_except_statements(self): for clause in ('except:', 'except :', 'except\t:'): self.assertNit(EXCEPT_TEMPLATE.format(clause), 'T803') - for clause in ( - 'except KeyError, e:', - 'except (KeyError, ValueError), e:', - 'except KeyError, e :', - 'except (KeyError, ValueError), e\t:'): - self.assertNit(EXCEPT_TEMPLATE.format(clause), 'T601') + try: + for clause in ( + 'except KeyError, e:', + 'except (KeyError, ValueError), e:', + 'except KeyError, e :', + 'except (KeyError, ValueError), e\t:'): + self.assertNit(EXCEPT_TEMPLATE.format(clause), 'T601') + except CheckSyntaxError: # Fix Python 3 raising SyntaxError + pass for clause in ( 'except KeyError:', diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_file_excluder.py b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_file_excluder.py index b774fa39c68..a372e9fae9c 100644 --- a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_file_excluder.py +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_file_excluder.py @@ -33,6 +33,7 @@ def setUp(self): def _create_scalastyle_excludes_file(self, exclude_patterns=None): return self.create_file( relpath='scalastyle_excludes.txt', + mode='w', contents='\n'.join(exclude_patterns) if exclude_patterns else '') def test_excludes_cpp_any(self): diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_import_order.py b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_import_order.py index 86f3ef45178..1a478714eb3 100644 --- a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_import_order.py +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_import_order.py @@ -126,7 +126,7 @@ def test_multiple_imports_error(self): self.assertEqual(1, len(chunk_errors)) self.assertEqual('T405', chunk_errors[0].code) self.assertEqual(Nit.ERROR, chunk_errors[0].severity) - self.assertItemsEqual([ImportType.STDLIB, ImportType.TWITTER], module_types) + self.assertEqual(sorted([ImportType.STDLIB, ImportType.TWITTER]), sorted(module_types)) io = self.get_plugin(""" import io, pkg_resources @@ -135,9 +135,9 @@ def test_multiple_imports_error(self): self.assertEqual(1, len(import_chunks)) module_types, chunk_errors = io.classify_imports(import_chunks[0]) self.assertEqual(3, len(chunk_errors)) - self.assertItemsEqual(['T403', 'T405', 'T402'], - [chunk_error.code for chunk_error in chunk_errors]) - self.assertItemsEqual([ImportType.STDLIB, ImportType.THIRD_PARTY], module_types) + self.assertEqual(sorted(['T403', 'T405', 'T402']), + sorted([chunk_error.code for chunk_error in chunk_errors])) + self.assertEqual(sorted([ImportType.STDLIB, ImportType.THIRD_PARTY]), sorted(module_types)) def test_import_lexical_order(self): imp = """ diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_print_statements.py b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_print_statements.py index 9242b4c6104..8b2f847d676 100644 --- a/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_print_statements.py +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/tasks/checkstyle/test_print_statements.py @@ -4,6 +4,8 @@ from __future__ import absolute_import, division, print_function, unicode_literals +import pytest +from future.utils import PY3 from pants_test.contrib.python.checks.tasks.checkstyle.plugin_test_base import \ CheckstylePluginTestBase @@ -30,6 +32,8 @@ def test_print_function(self): """ self.assertNoNits(statement) + @pytest.mark.skipif(PY3, reason='Print statement check disabled on Python 3 because interpreter will already ' + 'throw a syntax error.') def test_print_statement(self): statement = """ print["I do what I want"]