From 3ddffd296a2edc6c8340a2463ef727217107bf66 Mon Sep 17 00:00:00 2001 From: Sylvain Date: Tue, 25 Aug 2015 19:51:45 +0100 Subject: [PATCH 1/4] Add checks for unconsistent returns Added in PEP 8 : """ Be consistent in return statements. Either all return statements in a function should return an expression, or none of them should. If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None, and an explicit return statement should be present at the end of the function (if reachable). """ Checking for unconsistent returns corresponds to implementing a check based on ast tree. Given an AST tree, one can easily collect return statements and check whether they return an explicit value or not. Also, from the AST tree, one can try to check whether the end of the function is reachable. Warning W740 is added for explicit inconsistent returns. Warning W741 is added for implicit inconsistent returns : when a functions explicitly returns a value at some point but does not at the (reachable) end of the function. If the end of a function is not reachable but warning is triggered, one might simply add a "return None" or an "assert False" : as one said : "Explicit is better than implicit.". Regarding the code : Implementation has been made as easy to understand as possible. The new check itself has been implemented in a new class UnconsistentReturns which uses yet another class FlowAnalysis which serves as an holder for various helper methods. Also, I took this chance to change a few details so that AST-tree checks fit more easily. This changes a few APIs. I don't know if anyone is relying on those. Regarding the tests : Adding the first AST-tree based check leads to most of the (incorrect) test code to be parsed which leads to many SyntaxError either related to a single version of Python or to all of them. A A new symbol has been to be able to convey the fact that an error is expecting only for such or such version of Python. I've fixed all issues related to SyntaxError so that they are all passing all right. I hope I haven't changed what is actually tested. --- docs/intro.rst | 7 ++ pep8.py | 268 +++++++++++++++++++++++++++++++--------- setup.py | 1 + testsuite/E10.py | 10 +- testsuite/E11.py | 6 +- testsuite/E12.py | 54 ++++---- testsuite/E12not.py | 41 +++--- testsuite/E20.py | 10 +- testsuite/E22.py | 2 +- testsuite/E25.py | 2 +- testsuite/E27.py | 1 + testsuite/E90.py | 8 +- testsuite/W19.py | 8 +- testsuite/W60.py | 12 +- testsuite/W74.py | 101 +++++++++++++++ testsuite/python3.py | 1 + testsuite/support.py | 16 ++- testsuite/test_api.py | 34 +++-- testsuite/test_shell.py | 4 +- 19 files changed, 428 insertions(+), 158 deletions(-) create mode 100644 testsuite/W74.py diff --git a/docs/intro.rst b/docs/intro.rst index 141463ce..3d743e0f 100644 --- a/docs/intro.rst +++ b/docs/intro.rst @@ -407,6 +407,13 @@ This is the current list of error and warning codes: +----------+----------------------------------------------------------------------+ | W604 | backticks are deprecated, use 'repr()' | +----------+----------------------------------------------------------------------+ ++----------+----------------------------------------------------------------------+ +| **W7** | *Statement warning* | ++----------+----------------------------------------------------------------------+ +| W740 | inconsistent use of return (explicit) | ++----------+----------------------------------------------------------------------+ +| W741 | inconsistent use of return (implicit on reachable end of function) | ++----------+----------------------------------------------------------------------+ **(*)** In the default configuration, the checks **E121**, **E123**, **E126**, diff --git a/pep8.py b/pep8.py index 34ce07ae..fe391595 100755 --- a/pep8.py +++ b/pep8.py @@ -54,6 +54,7 @@ import inspect import keyword import tokenize +import ast from optparse import OptionParser from fnmatch import fnmatch try: @@ -145,6 +146,7 @@ def tabs_or_spaces(physical_line, indent_char): for offset, char in enumerate(indent): if char != indent_char: return offset, "E101 indentation contains mixed spaces and tabs" + return None def tabs_obsolete(physical_line): @@ -156,6 +158,7 @@ def tabs_obsolete(physical_line): indent = INDENT_REGEX.match(physical_line).group(1) if '\t' in indent: return indent.index('\t'), "W191 indentation contains tabs" + return None def trailing_whitespace(physical_line): @@ -177,6 +180,7 @@ def trailing_whitespace(physical_line): return len(stripped), "W291 trailing whitespace" else: return 0, "W293 blank line contains whitespace" + return None def trailing_blank_lines(physical_line, lines, line_number, total_lines): @@ -193,6 +197,7 @@ def trailing_blank_lines(physical_line, lines, line_number, total_lines): return 0, "W391 blank line at end of file" if stripped_last_line == physical_line: return len(physical_line), "W292 no newline at end of file" + return None def maximum_line_length(physical_line, max_line_length, multiline): @@ -216,7 +221,7 @@ def maximum_line_length(physical_line, max_line_length, multiline): if ((len(chunks) == 1 and multiline) or (len(chunks) == 2 and chunks[0] == '#')) and \ len(line) - len(chunks[-1]) < max_line_length - 7: - return + return None if hasattr(line, 'decode'): # Python 2 # The line could contain multi-byte characters try: @@ -226,6 +231,7 @@ def maximum_line_length(physical_line, max_line_length, multiline): if length > max_line_length: return (max_line_length, "E501 line too long " "(%d > %d characters)" % (length, max_line_length)) + return None ############################################################################## @@ -752,16 +758,13 @@ def whitespace_around_named_parameter_equals(logical_line, tokens): Don't use spaces around the '=' sign when used to indicate a keyword argument or a default parameter value. - Okay: def complex(real, imag=0.0): - Okay: return magic(r=real, i=imag) + Okay: def complex(real, imag=0.0):\n return magic(r=real, i=imag) Okay: boolean(a == b) Okay: boolean(a != b) Okay: boolean(a <= b) Okay: boolean(a >= b) - Okay: def foo(arg: int = 42): - - E251: def complex(real, imag = 0.0): - E251: return magic(r = real, i = imag) + # Only with Python 3 - Okay: def foo(arg: int = 42):\n pass + E251: def complex(real, imag = 0.0):\n return magic(r = real, i = imag) """ parens = 0 no_space = False @@ -781,9 +784,9 @@ def whitespace_around_named_parameter_equals(logical_line, tokens): parens += 1 elif text == ')': parens -= 1 - elif in_def and text == ':' and parens == 1: + elif parens == 1 and in_def and text == ':': annotated_func_arg = True - elif parens and text == ',' and parens == 1: + elif parens == 1 and text == ',': annotated_func_arg = False elif parens and text == '=' and not annotated_func_arg: no_space = True @@ -1013,7 +1016,7 @@ def break_around_binary_operator(logical_line, tokens): Okay: (width == 0 +\n height == 0) Okay: foo(\n -x) - Okay: foo(x\n []) + Okay: foo(x,\n []) Okay: x = '''\n''' + '' Okay: foo(x,\n -y) Okay: foo(x, # comment\n -y) @@ -1046,11 +1049,11 @@ def comparison_to_singleton(logical_line, noqa): Comparisons to singletons like None should always be done with "is" or "is not", never the equality operators. - Okay: if arg is not None: - E711: if arg != None: - E711: if None == arg: - E712: if arg == True: - E712: if False == arg: + Okay: arg is not None + E711: arg != None + E711: None == arg + E712: arg == True + E712: False == arg Also, beware of writing if x when you really mean if x is not None -- e.g. when testing whether a variable or argument that defaults to None was @@ -1100,15 +1103,15 @@ def comparison_type(logical_line, noqa): Do not compare types directly. - Okay: if isinstance(obj, int): - E721: if type(obj) is type(1): + Okay: isinstance(obj, int) + E721: type(obj) is type(1) When checking if an object is a string, keep in mind that it might be a unicode string too! In Python 2.3, str and unicode have a common base class, basestring, so you can do: - Okay: if isinstance(obj, basestring): - Okay: if type(a1) is type(b1): + Okay: isinstance(obj, basestring) + Okay: type(a1) is type(b1) """ match = COMPARE_TYPE_REGEX.search(logical_line) if match and not noqa: @@ -1121,7 +1124,7 @@ def comparison_type(logical_line, noqa): def python_3000_has_key(logical_line, noqa): r"""The {}.has_key() method is removed in Python 3: use the 'in' operator. - Okay: if "alph" in d:\n print d["alph"] + Okay: "alph" in d W601: assert d.has_key('alph') """ pos = logical_line.find('.has_key(') @@ -1147,8 +1150,8 @@ def python_3000_not_equal(logical_line): The older syntax is removed in Python 3. - Okay: if a != 'no': - W603: if a <> 'no': + Okay: a != 'no' + W603: a <> 'no' """ pos = logical_line.find('<>') if pos > -1: @@ -1166,6 +1169,158 @@ def python_3000_backticks(logical_line): yield pos, "W604 backticks are deprecated, use 'repr()'" +class UnconsistentReturns(): + r"""Check that return statement are consistent. + + Functions should either return an explicit value in all return + statements (including the final value-less implicit return if + reachable) or in none of them. + If a return statement returns an explicit value in a function : + * return statements with no explicit values lead to W740. + * end of function (if reachable) leads to W741. + """ + + def __init__(self, tree, filename): + r"""Init.""" + self.tree = tree + self.filename = filename + + def run(self): + r"""Run the check.""" + return UnconsistentReturns.check_in_tree(self.tree) + + @staticmethod + def check_in_tree(tree): + r"""Check for inconsistent returns in tree.""" + assert isinstance(tree, ast.AST) + for node in ast.walk(tree): + if isinstance(node, ast.FunctionDef): + for err in UnconsistentReturns.check_in_func(node): + yield err + + @staticmethod + def check_in_func(func_node): + r"""Check for inconsistent returns (with or without values) in function. + """ + assert isinstance(func_node, ast.FunctionDef) + returns = list(FlowAnalysis.collect_return_nodes(func_node)) + returns_value = [ret for ret in returns if ret.value is not None] + if returns_value: + func_name = func_node.name + for r in returns: + if r.value is None: + yield (r.lineno, + r.col_offset, + "W740 unconsistent return values in %s" % func_name, + "toto") + if FlowAnalysis.end_of_block_is_reachable(func_node.body): + yield (func_node.lineno, + func_node.col_offset, + "W741 unconsistent return values in %s" % func_name, + "toto") + + +class FlowAnalysis(): + r"""Class of utility methods to perform flow analysis. + + Class container for various static methods. This class probably + shouldn't be a class but a module but it seems like being a single + file is one of the pep8 features.""" + + @staticmethod + def collect_return_nodes(func_node): + r"""Collect return nodes from the node describing a function. + + The tricky part is not to get the nodes corresponding to return + statements in nested function definitions. + Heavily based on the ast.walk() function. + """ + from collections import deque + assert isinstance(func_node, ast.FunctionDef) + todo = deque(ast.iter_child_nodes(func_node)) + while todo: + node = todo.popleft() + if not isinstance(node, ast.FunctionDef): + todo.extend(ast.iter_child_nodes(node)) + if isinstance(node, ast.Return): + yield node + + @staticmethod + def end_of_block_is_reachable(tree): + r"""Return true if the end of a block is reachable. + + A block can be a single ast.stmt or a list of them. + Detecting whether the end of some piece of code is reachable or + not corresponds to solving the halting problem which is known to be + impossible. However, we could solve a relaxed version of this : + indeed, we may assume that: + - all code is reachable except for obvious cases. + - only a few kind of statements may break the reachable property: + * return statements + * raise statements + * assert with "obviously"-False values. + We'll consider the end of block to be reachable if nothing breaks + the reachability property. + """ + this_func = FlowAnalysis.end_of_block_is_reachable # shorter name + if isinstance(tree, list): + return all(this_func(stmt) for stmt in tree) + assert isinstance(tree, ast.stmt) + # These stop reachability + if isinstance(tree, (ast.Return, ast.Raise)): + return False + elif isinstance(tree, ast.Assert): + return not FlowAnalysis.expression_must_be_false(tree.test) + # These propagage reachability + elif isinstance(tree, ast.If): + branches = [] + if not FlowAnalysis.expression_must_be_true(tree.test): + branches.append(tree.orelse) + if not FlowAnalysis.expression_must_be_false(tree.test): + branches.append(tree.body) + return any(this_func(brch) for brch in branches) + elif isinstance(tree, ast.TryFinally): + return this_func(tree.finalbody) and this_func(tree.body) + elif isinstance(tree, ast.TryExcept): + # TODO: orelse ignore at the moment + return this_func(tree.body) or \ + any(this_func(handler.body) for handler in tree.handlers) + elif isinstance(tree, ast.With): + return this_func(tree.body) + # Otherwise, assume reachability hasn't been broken + return True + + @staticmethod + def expression_must_be_true(tree): + assert isinstance(tree, ast.expr) + try: + if isinstance(tree, ast.NameConstant) and tree.value: + return True + except AttributeError: + pass # NameConstant is from Python 3.4 + if isinstance(tree, ast.Name) and tree.id == "True": + return True + elif isinstance(tree, ast.UnaryOp): + if isinstance(tree.op, ast.Not): + return FlowAnalysis.expression_must_be_false(tree.operand) + return False + + @staticmethod + def expression_must_be_false(tree): + assert isinstance(tree, ast.expr) + try: + if isinstance(tree, ast.NameConstant) and not tree.value: + return True + except AttributeError: + pass # NameConstant is from Python 3.4 + if isinstance(tree, ast.Name) and tree.id == "False": + return True + elif isinstance(tree, ast.UnaryOp): + if isinstance(tree.op, ast.Not): + return FlowAnalysis.expression_must_be_true(tree.operand) + return False + + ############################################################################## # Helper functions ############################################################################## @@ -1318,12 +1473,15 @@ def _get_parameters(function): if sys.version_info >= (3, 3): return list(inspect.signature(function).parameters) else: - return inspect.getargspec(function)[0] + getarg = getattr(inspect, 'getfullargspec', inspect.getargspec) + return getarg(function).args def register_check(check, codes=None): """Register a new check object.""" def _add_check(check, kind, codes, args): + if codes is None: + codes = ERRORCODE_REGEX.findall(check.__doc__ or '') if check in _checks[kind]: _checks[kind][check][0].extend(codes or []) else: @@ -1331,12 +1489,15 @@ def _add_check(check, kind, codes, args): if inspect.isfunction(check): args = _get_parameters(check) if args and args[0] in ('physical_line', 'logical_line'): - if codes is None: - codes = ERRORCODE_REGEX.findall(check.__doc__ or '') _add_check(check, args[0], codes, args) elif inspect.isclass(check): - if _get_parameters(check.__init__)[:2] == ['self', 'tree']: - _add_check(check, 'tree', codes, None) + init = getattr(check, '__init__', None) + # Exclude slot wrappers. + # Python 3 uses functions, Python 2 unbound methods. + if inspect.isfunction(init) or inspect.ismethod(init): + args = _get_parameters(init) + if args and args[0] == 'self' and args[1] == 'tree': + _add_check(check, args[1], codes, args) def init_checks_registry(): @@ -1347,6 +1508,8 @@ def init_checks_registry(): mod = inspect.getmodule(register_check) for (name, function) in inspect.getmembers(mod, inspect.isfunction): register_check(function) + for (name, klass) in inspect.getmembers(mod, inspect.isclass): + register_check(klass) init_checks_registry() @@ -1420,21 +1583,16 @@ def readline(self): def run_check(self, check, argument_names): """Run a check plugin.""" - arguments = [] - for name in argument_names: - arguments.append(getattr(self, name)) - return check(*arguments) - - def init_checker_state(self, name, argument_names): - """ Prepares a custom state for the specific checker plugin.""" if 'checker_state' in argument_names: - self.checker_state = self._checker_states.setdefault(name, {}) + self.checker_state = self._checker_states.setdefault( + check.__name__, {}) + arguments = [getattr(self, name) for name in argument_names] + return check(*arguments) def check_physical(self, line): """Run all physical checks on a raw input line.""" self.physical_line = line - for name, check, argument_names in self._physical_checks: - self.init_checker_state(name, argument_names) + for check, argument_names in self._physical_checks: result = self.run_check(check, argument_names) if result is not None: (offset, text) = result @@ -1490,10 +1648,7 @@ def check_logical(self): self.blank_before = self.blank_lines if self.verbose >= 2: print(self.logical_line[:80].rstrip()) - for name, check, argument_names in self._logical_checks: - if self.verbose >= 4: - print(' ' + name) - self.init_checker_state(name, argument_names) + for check, argument_names in self._logical_checks: for offset, text in self.run_check(check, argument_names) or (): if not isinstance(offset, tuple): for token_offset, pos in mapping: @@ -1512,8 +1667,9 @@ def check_ast(self): try: tree = compile(''.join(self.lines), '', 'exec', PyCF_ONLY_AST) except (ValueError, SyntaxError, TypeError): - return self.report_invalid_syntax() - for name, cls, __ in self._ast_checks: + self.report_invalid_syntax() + return + for cls, __ in self._ast_checks: checker = cls(tree, self.filename) for lineno, offset, text, check in checker.run(): if not self.lines or not noqa(self.lines[lineno - 1]): @@ -1656,7 +1812,7 @@ def error(self, line_number, offset, text, check): """Report an error, according to options.""" code = text[:4] if self._ignore_code(code): - return + return None if code in self.counters: self.counters[code] += 1 else: @@ -1664,7 +1820,7 @@ def error(self, line_number, offset, text, check): self.messages[code] = text[5:] # Don't care about expected errors or warnings if code in self.expected: - return + return None if self.print_filename and not self.file_errors: print(self.filename) self.file_errors += 1 @@ -1775,7 +1931,7 @@ def __init__(self, options): def error(self, line_number, offset, text, check): if line_number not in self._selected[self.filename]: - return + return None return super(DiffReport, self).error(line_number, offset, text, check) @@ -1854,7 +2010,7 @@ def input_dir(self, dirname): """Check all files in this directory and all subdirectories.""" dirname = dirname.rstrip('/') if self.excluded(dirname): - return 0 + return counters = self.options.report.counters verbose = self.options.verbose filepatterns = self.options.filename @@ -1894,11 +2050,11 @@ def ignore_code(self, code): return False. Else, if 'options.ignore' contains a prefix of the error code, return True. """ - if len(code) < 4 and any(s.startswith(code) - for s in self.options.select): + options = self.options + if len(code) < 4 and any(s.startswith(code) for s in options.select): return False - return (code.startswith(self.options.ignore) and - not code.startswith(self.options.select)) + return (code.startswith(options.ignore) and + not code.startswith(options.select)) def get_checks(self, argument_name): """Get all the checks for this category. @@ -1906,12 +2062,10 @@ def get_checks(self, argument_name): Find all globally visible functions where the first argument name starts with argument_name and which contain selected tests. """ - checks = [] - for check, attrs in _checks[argument_name].items(): - (codes, args) = attrs - if any(not (code and self.ignore_code(code)) for code in codes): - checks.append((check.__name__, check, args)) - return sorted(checks) + checks = [(check, args) + for check, (codes, args) in _checks[argument_name].items() + if any(not self.ignore_code(code) for code in codes)] + return sorted(checks, key=lambda arg: arg[0].__name__) def get_parser(prog='pep8', version=__version__): diff --git a/setup.py b/setup.py index 29182c6d..62ce493b 100644 --- a/setup.py +++ b/setup.py @@ -8,6 +8,7 @@ def get_version(): for line in f: if line.startswith('__version__'): return eval(line.split('=')[-1]) + return None def get_long_description(): diff --git a/testsuite/E10.py b/testsuite/E10.py index cd142e39..c2c476c0 100644 --- a/testsuite/E10.py +++ b/testsuite/E10.py @@ -1,8 +1,8 @@ -#: E101 W191 +#: E101 W191 E901:python3 for a in 'abc': for b in 'xyz': - print a # indented with 8 spaces - print b # indented with 1 tab + print(a) # indented with 8 spaces + print(b) # indented with 1 tab #: E101 E122 W191 W191 if True: pass @@ -35,7 +35,7 @@ def tearDown(self): def test_keys(self): """areas.json - All regions are accounted for.""" expected = set([ - u'Norrbotten', - u'V\xe4sterbotten', + 'Norrbotten', + 'V\xe4sterbotten', ]) #: diff --git a/testsuite/E11.py b/testsuite/E11.py index 4ed10963..c08bceb5 100644 --- a/testsuite/E11.py +++ b/testsuite/E11.py @@ -1,13 +1,13 @@ #: E111 if x > 2: - print x + print(x) #: E111 if True: print -#: E112 +#: E112 E901 if False: print -#: E113 +#: E113 E901 print print #: E114 E116 diff --git a/testsuite/E12.py b/testsuite/E12.py index a995c955..8b4e2569 100644 --- a/testsuite/E12.py +++ b/testsuite/E12.py @@ -1,22 +1,22 @@ #: E121 -print "E121", ( - "dent") +print("E121", ( + "dent")) #: E122 -print "E122", ( -"dent") +print("E122", ( +"dent")) #: E123 my_list = [ 1, 2, 3, 4, 5, 6, ] #: E124 -print "E124", ("visual", +print("E124", ("visual", "indent_two" - ) + )) #: E124 -print "E124", ("visual", +print("E124", ("visual", "indent_five" -) +)) #: E124 a = (123, ) @@ -25,20 +25,20 @@ col < 0 or self.moduleCount <= col): raise Exception("%s,%s - %s" % (row, col, self.moduleCount)) #: E126 -print "E126", ( - "dent") +print("E126", ( + "dent")) #: E126 -print "E126", ( - "dent") +print("E126", ( + "dent")) #: E127 -print "E127", ("over-", - "over-indent") +print("E127", ("over-", + "over-indent")) #: E128 -print "E128", ("visual", - "hanging") +print("E128", ("visual", + "hanging")) #: E128 -print "E128", ("under-", - "under-indent") +print("E128", ("under-", + "under-indent")) #: @@ -63,11 +63,11 @@ 4 + \ 5 + 6 #: E131 -print "hello", ( +print("hello", ( "there", # "john", - "dude") + "dude")) #: E126 part = set_mimetype(( a.get('mime_type', 'text')), @@ -100,11 +100,11 @@ or another_very_long_variable_name: raise Exception() #: E122 -dictionary = [ +dictionary = { "is": { "nested": yes(), }, -] +} #: E122 setup('', scripts=[''], @@ -117,9 +117,9 @@ #: E123 W291 -print "E123", ( +print("E123", ( "bad", "hanging", "close" - ) + )) # #: E123 E123 E123 result = { @@ -358,7 +358,7 @@ def example_issue254(): """.strip().split(): print(foo) #: E122:6:5 E122:7:5 E122:8:1 -print dedent( +print(dedent( ''' mkdir -p ./{build}/ mv ./build/ ./{build}/%(revision)s/ @@ -366,8 +366,8 @@ def example_issue254(): build='build', # more stuff ) -) -#: E701:1:8 E122:2:1 E203:4:8 E128:5:1 +)) +#: E701:1:8 E122:2:1 E203:4:8 E128:5:1 E901:python2 if True:\ print(True) diff --git a/testsuite/E12not.py b/testsuite/E12not.py index e76ef134..67836292 100644 --- a/testsuite/E12not.py +++ b/testsuite/E12not.py @@ -46,34 +46,34 @@ "indented for visual indent") -print "OK", ("visual", - "indent") +print("OK", ("visual", + "indent")) -print "Okay", ("visual", +print("Okay", ("visual", "indent_three" - ) + )) -print "a-ok", ( +print("a-ok", ( "there", "dude", -) +)) -print "hello", ( +print("hello", ( "there", - "dude") + "dude")) -print "hello", ( +print("hello", ( "there", # "john", - "dude") + "dude")) -print "hello", ( - "there", "dude") +print("hello", ( + "there", "dude")) -print "hello", ( +print("hello", ( "there", "dude", -) +)) # Aligned with opening delimiter foo = long_function_name(var_one, var_two, @@ -188,12 +188,12 @@ def long_function_name( if ((foo.bar("baz") and foo.bar("frop") )): - print "yes" + print("yes") # also ok, but starting to look like LISP if ((foo.bar("baz") and foo.bar("frop"))): - print "yes" + print("yes") if (a == 2 or b == "abc def ghi" @@ -213,12 +213,12 @@ def long_function_name( # -print 'l.{line}\t{pos}\t{name}\t{text}'.format( +print('l.{line}\t{pos}\t{name}\t{text}'.format( line=token[2][0], pos=pos, name=tokenize.tok_name[token[0]], text=repr(token[1]), -) +)) print('%-7d %s per second (%d total)' % ( options.counters[key] / elapsed, key, @@ -429,7 +429,7 @@ def unicode2html(s): help = "print total number of errors " \ "to standard error" - +#: E901:python3 help = u"print total number of errors " \ u"to standard error" @@ -521,8 +521,7 @@ def unicode2html(s): 'reasonComment_de', 'reasonComment_it'), '?'), "foo", context={'alpha': 4, 'beta': 53242234, 'gamma': 17}) - - +#: E901 def f(): try: if not Debug: diff --git a/testsuite/E20.py b/testsuite/E20.py index 2f1ecc28..85d98ca6 100644 --- a/testsuite/E20.py +++ b/testsuite/E20.py @@ -37,18 +37,18 @@ #: E203:1:10 if x == 4 : - print x, y + print(x, y) x, y = y, x -#: E203:2:15 E702:2:16 +#: E203:2:16 E702:2:17 if x == 4: - print x, y ; x, y = y, x + print(x, y) ; x, y = y, x #: E203:3:13 if x == 4: - print x, y + print(x, y) x, y = y , x #: Okay if x == 4: - print x, y + print(x, y) x, y = y, x a[b1, :] == a[b1, ...] b = a[:, b1] diff --git a/testsuite/E22.py b/testsuite/E22.py index 9d8efda5..c8f26960 100644 --- a/testsuite/E22.py +++ b/testsuite/E22.py @@ -142,7 +142,7 @@ def halves(n): print >>sys.stderr, "x is out of range." print >> sys.stdout, "x is an integer." x = x / 2 - 1 - +#: E901:2:6:python2 if alpha[:-i]: *a, b = (1, 2, 3) diff --git a/testsuite/E25.py b/testsuite/E25.py index ad8db882..71f49956 100644 --- a/testsuite/E25.py +++ b/testsuite/E25.py @@ -31,6 +31,6 @@ def foo(bar = False): d[type(None)] = _deepcopy_atomic # Annotated Function Definitions -#: Okay +#: E901:1:17:python2 def munge(input: AnyStr, sep: AnyStr = None, limit=1000) -> AnyStr: pass diff --git a/testsuite/E27.py b/testsuite/E27.py index f9d3e8e1..f737f474 100644 --- a/testsuite/E27.py +++ b/testsuite/E27.py @@ -6,6 +6,7 @@ True and False #: E271 if 1: + pass #: E273 True and False #: E273 E274 diff --git a/testsuite/E90.py b/testsuite/E90.py index 1db3d0e6..22511056 100644 --- a/testsuite/E90.py +++ b/testsuite/E90.py @@ -1,14 +1,14 @@ -#: E901 +#: E901 E901 } -#: E901 +#: E901 E901 = [x -#: E901 E101 W191 +#: E901 E901 E101 W191 while True: try: pass except: print 'Whoops' -#: E122 E225 E251 E251 E701 +#: E122 E225 E251 E251 E701 E901 # Do not crash if code is invalid if msg: diff --git a/testsuite/W19.py b/testsuite/W19.py index afdfb767..c952e3e4 100644 --- a/testsuite/W19.py +++ b/testsuite/W19.py @@ -70,12 +70,12 @@ def long_function_name( if ((foo.bar("baz") and foo.bar("frop") )): - print "yes" + print("yes") #: E101 W191 # also ok, but starting to look like LISP if ((foo.bar("baz") and foo.bar("frop"))): - print "yes" + print("yes") #: E101 W191 if (a == 2 or b == "abc def ghi" @@ -135,8 +135,8 @@ def long_function_name( def test_keys(self): """areas.json - All regions are accounted for.""" expected = set([ - u'Norrbotten', - u'V\xe4sterbotten', + 'Norrbotten', + 'V\xe4sterbotten', ]) #: W191 x = [ diff --git a/testsuite/W60.py b/testsuite/W60.py index 973d22ff..2050e761 100644 --- a/testsuite/W60.py +++ b/testsuite/W60.py @@ -1,15 +1,15 @@ -#: W601 +#: W601 E901:python3 if a.has_key("b"): print a -#: W602 +#: W602 E901:python3 raise DummyError, "Message" -#: W602 +#: W602 E901:python3 raise ValueError, "hello %s %s" % (1, 2) -#: Okay +#: E901:python3 raise type_, val, tb raise Exception, Exception("f"), t -#: W603 +#: W603 E901:python3 if x <> 0: x = 0 -#: W604 +#: W604 E901:python3 val = `1 + 2` diff --git a/testsuite/W74.py b/testsuite/W74.py new file mode 100644 index 00000000..36fe278d --- /dev/null +++ b/testsuite/W74.py @@ -0,0 +1,101 @@ +# Test suite to work on issue https://github.com/PyCQA/pep8/issues/399 +import math +import itertools + +# Code examples from GvR +# https://mail.python.org/pipermail/python-dev/2015-April/139054.html + +# Correct ### + + +def foo_ok(x): + if x >= 0: + return math.sqrt(x) + else: + return None + + +def bar_ok(x): + if x < 0: + return None + return math.sqrt(x) + + +def foobar_ok(x): + if True: + return None + else: + pass + + +# Not correct ### +#: W741:1:1 +def foo_ko(x): + if x >= 0: + return math.sqrt(x) +#: W740:3:9 +def bar_ko(x): + if x < 0: + return + return math.sqrt(x) + +# More examples for the sake of testings + +# Correct ### + + +def foo_ok(x): + if x >= 0: + return math.sqrt(x) + elif x == 0: + return 0 + else: + return None + + +def goldbach_conjecture_ok(): + for i in itertools.count(2): + if not can_be_expressed_as_prime_sum(i): + return i + assert not True + + +def outer_function(): + + def nested_function(): + return 6 * 9 + + print(42 == nested_function()) + return +# Not correct ### +#: W741:1:1 +def foo_ko(x): + if x >= 0: + return math.sqrt(x) + elif x == 0: + return 0 +#: W741:1:1 +def goldbach_conjecture_ko(): + for i in itertools.count(2): + if not can_be_expressed_as_prime_sum(i): + return i + + +# W741:1:1 +def return_finally1(): # return 1 + try: + return 1 + finally: + pass +#: W740:5:9 +def return_finally2(): # return None + try: + return 2 + finally: + return +#: W740:3:9 +def return_finally3(): # return 4 + try: + return + finally: + return 4 diff --git a/testsuite/python3.py b/testsuite/python3.py index 88818800..194d5e94 100644 --- a/testsuite/python3.py +++ b/testsuite/python3.py @@ -2,5 +2,6 @@ # Annotated function (Issue #29) +#: E901:1:11:python2 def foo(x: int) -> int: return x + 1 diff --git a/testsuite/support.py b/testsuite/support.py index 6bc795d7..d4878529 100644 --- a/testsuite/support.py +++ b/testsuite/support.py @@ -38,7 +38,7 @@ def error(self, line_number, offset, text, check): detailed_code = '%s:%s:%s' % (code, line_number, offset + 1) # Don't care about expected errors or warnings if code in self.expected or detailed_code in self.expected: - return + return None self._deferred_print.append( (line_number, offset, detailed_code, text[5:], check.__doc__)) self.file_errors += 1 @@ -91,7 +91,7 @@ def selftest(options): report = BaseReport(options) counters = report.counters checks = options.physical_checks + options.logical_checks - for name, check, argument_names in checks: + for check, argument_names in checks: for line in check.__doc__.splitlines(): line = line.lstrip() match = SELFTEST_REGEX.match(line) @@ -162,10 +162,16 @@ def run_tests(filename): if codes and index: if 'noeol' in codes: testcase[-1] = testcase[-1].rstrip('\n') - codes = [c for c in codes - if c not in ('Okay', 'noeol')] + expected = [] + for c in codes: + c, sep, version = c.partition(':python') + if version and str(sys.version_info.major) != version: + continue + if c in ('Okay', 'noeol'): + continue + expected.append(c) # Run the checker - runner(filename, testcase, expected=codes, + runner(filename, testcase, expected=expected, line_offset=line_offset) # output the real line numbers line_offset = index + 1 diff --git a/testsuite/test_api.py b/testsuite/test_api.py index 1cb0d4b5..9699a647 100644 --- a/testsuite/test_api.py +++ b/testsuite/test_api.py @@ -53,7 +53,7 @@ def check_dummy(physical_line, line_number): options = pep8.StyleGuide().options self.assertTrue(any(func == check_dummy - for name, func, args in options.physical_checks)) + for func, args in options.physical_checks)) def test_register_logical_check(self): def check_dummy(logical_line, tokens): @@ -74,7 +74,7 @@ def check_dummy(logical_line, tokens): options = pep8.StyleGuide().options self.assertTrue(any(func == check_dummy - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) def test_register_ast_check(self): pep8.register_check(DummyChecker, ['Z701']) @@ -82,11 +82,11 @@ def test_register_ast_check(self): self.assertTrue(DummyChecker in pep8._checks['tree']) codes, args = pep8._checks['tree'][DummyChecker] self.assertTrue('Z701' in codes) - self.assertTrue(args is None) + self.assertEqual(args, ['self', 'tree', 'filename']) options = pep8.StyleGuide().options self.assertTrue(any(cls == DummyChecker - for name, cls, args in options.ast_checks)) + for cls, args in options.ast_checks)) def test_register_invalid_check(self): class InvalidChecker(DummyChecker): @@ -124,7 +124,7 @@ def test_styleguide(self): report = pep8.StyleGuide().check_files([E11]) stdout = sys.stdout.getvalue().splitlines() self.assertEqual(len(stdout), report.total_errors) - self.assertEqual(report.total_errors, 17) + self.assertEqual(report.total_errors, 18, stdout) self.assertFalse(sys.stderr) self.reset() @@ -132,7 +132,7 @@ def test_styleguide(self): report = pep8.StyleGuide(paths=[E11]).check_files() stdout = sys.stdout.getvalue().splitlines() self.assertEqual(len(stdout), report.total_errors) - self.assertEqual(report.total_errors, 17) + self.assertEqual(report.total_errors, 18) self.assertFalse(sys.stderr) self.reset() @@ -253,40 +253,38 @@ def test_styleguide_checks(self): # Default lists of checkers self.assertTrue(len(pep8style.options.physical_checks) > 4) self.assertTrue(len(pep8style.options.logical_checks) > 10) - self.assertEqual(len(pep8style.options.ast_checks), 0) + self.assertTrue(len(pep8style.options.ast_checks) > 0) # Sanity check - for name, check, args in pep8style.options.physical_checks: - self.assertEqual(check.__name__, name) + for check, args in pep8style.options.physical_checks: self.assertEqual(args[0], 'physical_line') - for name, check, args in pep8style.options.logical_checks: - self.assertEqual(check.__name__, name) + for check, args in pep8style.options.logical_checks: self.assertEqual(args[0], 'logical_line') # Do run E11 checks options = pep8.StyleGuide().options self.assertTrue(any(func == pep8.indentation - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) options = pep8.StyleGuide(select=['E']).options self.assertTrue(any(func == pep8.indentation - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) options = pep8.StyleGuide(ignore=['W']).options self.assertTrue(any(func == pep8.indentation - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) options = pep8.StyleGuide(ignore=['E12']).options self.assertTrue(any(func == pep8.indentation - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) # Do not run E11 checks options = pep8.StyleGuide(select=['W']).options self.assertFalse(any(func == pep8.indentation - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) options = pep8.StyleGuide(ignore=['E']).options self.assertFalse(any(func == pep8.indentation - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) options = pep8.StyleGuide(ignore=['E11']).options self.assertFalse(any(func == pep8.indentation - for name, func, args in options.logical_checks)) + for func, args in options.logical_checks)) def test_styleguide_init_report(self): pep8style = pep8.StyleGuide(paths=[E11]) diff --git a/testsuite/test_shell.py b/testsuite/test_shell.py index e536852f..3200be85 100644 --- a/testsuite/test_shell.py +++ b/testsuite/test_shell.py @@ -75,8 +75,10 @@ def test_check_simple(self): stdout = stdout.splitlines() self.assertEqual(errcode, 1) self.assertFalse(stderr) - self.assertEqual(len(stdout), 17) + self.assertEqual(len(stdout), 18) for line, num, col in zip(stdout, (3, 6, 9, 12), (3, 6, 1, 5)): + if 'E901' in line: + continue path, x, y, msg = line.split(':') self.assertTrue(path.endswith(E11)) self.assertEqual(x, str(num)) From 3737e3220d88f943d10fe7f7d89fae7dd94f3d1c Mon Sep 17 00:00:00 2001 From: Sylvain Date: Wed, 26 Aug 2015 17:54:55 +0100 Subject: [PATCH 2/4] Fix comments related to unconsistent commits Comment was made that checks should be ignored by default. This is now the case and code/documentation and tests have been updated. Also, Travis commented that a few things went wrong on pretty much all Python versions. I think I have fixed most of the issues (related to the fact that : - some AST elements have changed - sys.version_info is not a namedtuple on Python 2.6). Also, I realised that a test would fail as a SyntaxError only on Python 2.6. For that reason, I've made the version tag on error somewhat more generic. Now, it can be any prefix for a version : 2 corresponds to all Python 2 version as 2.6 correponds to 2.6 and so on... --- docs/intro.rst | 9 +++++++-- pep8.py | 36 ++++++++++++++++++------------------ testsuite/W39.py | 2 +- testsuite/support.py | 2 +- testsuite/test_api.py | 4 ++-- testsuite/test_shell.py | 4 +--- 6 files changed, 30 insertions(+), 27 deletions(-) diff --git a/docs/intro.rst b/docs/intro.rst index 3d743e0f..cecb9dfd 100644 --- a/docs/intro.rst +++ b/docs/intro.rst @@ -410,9 +410,9 @@ This is the current list of error and warning codes: +----------+----------------------------------------------------------------------+ | **W7** | *Statement warning* | +----------+----------------------------------------------------------------------+ -| W740 | inconsistent use of return (explicit) | +| W740 (#) | inconsistent use of return (explicit) | +----------+----------------------------------------------------------------------+ -| W741 | inconsistent use of return (implicit on reachable end of function) | +| W741 (#) | inconsistent use of return (implicit on reachable end of function) | +----------+----------------------------------------------------------------------+ @@ -425,6 +425,11 @@ closing`` to report **E133** instead of **E123**. **(^)** These checks can be disabled at the line level using the ``# noqa`` special comment. This possibility should be reserved for special cases. +**(#)** In the default configuration, the checks **W740** and **W741** are +ignored for performance reasons. Indeed, they rely on an AST tree to be +built which is a a slower operation. + + *Special cases aren't special enough to break the rules.* diff --git a/pep8.py b/pep8.py index fe391595..acdc0bc3 100755 --- a/pep8.py +++ b/pep8.py @@ -66,7 +66,7 @@ __version__ = '1.6.3a0' DEFAULT_EXCLUDE = '.svn,CVS,.bzr,.hg,.git,__pycache__,.tox' -DEFAULT_IGNORE = 'E121,E123,E126,E226,E24,E704' +DEFAULT_IGNORE = 'E121,E123,E126,E226,E24,E704,W740,W741' try: if sys.platform == 'win32': USER_CONFIG = os.path.expanduser(r'~\.pep8') @@ -1279,10 +1279,16 @@ def end_of_block_is_reachable(tree): if not FlowAnalysis.expression_must_be_false(tree.test): branches.append(tree.body) return any(this_func(brch) for brch in branches) - elif isinstance(tree, ast.TryFinally): + elif isinstance(tree, getattr(ast, 'TryFinally', ())): return this_func(tree.finalbody) and this_func(tree.body) - elif isinstance(tree, ast.TryExcept): - # TODO: orelse ignore at the moment + elif isinstance(tree, getattr(ast, 'TryExcept', ())): + # TODO: orelse ignored at the moment + return this_func(tree.body) or \ + any(this_func(handler.body) for handler in tree.handlers) + elif isinstance(tree, getattr(ast, 'Try', ())): + if not this_func(tree.finalbody): + return False + # TODO: orelse ignored at the moment return this_func(tree.body) or \ any(this_func(handler.body) for handler in tree.handlers) elif isinstance(tree, ast.With): @@ -1293,13 +1299,10 @@ def end_of_block_is_reachable(tree): @staticmethod def expression_must_be_true(tree): assert isinstance(tree, ast.expr) - try: - if isinstance(tree, ast.NameConstant) and tree.value: - return True - except AttributeError: - pass # NameConstant is from Python 3.4 - if isinstance(tree, ast.Name) and tree.id == "True": - return True + if isinstance(tree, getattr(ast, 'NameConstant', ())): + return tree.value + elif isinstance(tree, ast.Name): + return tree.id == "True" elif isinstance(tree, ast.UnaryOp): if isinstance(tree.op, ast.Not): return FlowAnalysis.expression_must_be_false(tree.operand) @@ -1308,13 +1311,10 @@ def expression_must_be_true(tree): @staticmethod def expression_must_be_false(tree): assert isinstance(tree, ast.expr) - try: - if isinstance(tree, ast.NameConstant) and not tree.value: - return True - except AttributeError: - pass # NameConstant is from Python 3.4 - if isinstance(tree, ast.Name) and tree.id == "False": - return True + if isinstance(tree, getattr(ast, 'NameConstant', ())): + return not tree.value + elif isinstance(tree, ast.Name): + return tree.id == "False" elif isinstance(tree, ast.UnaryOp): if isinstance(tree.op, ast.Not): return FlowAnalysis.expression_must_be_true(tree.operand) diff --git a/testsuite/W39.py b/testsuite/W39.py index 68f886be..2b7d9749 100644 --- a/testsuite/W39.py +++ b/testsuite/W39.py @@ -5,7 +5,7 @@ # Two additional empty lines -#: W391:4:1 W293:3:1 W293:4:1 noeol +#: W391:4:1 W293:3:1 W293:4:1 E901:python2.6 noeol # The last lines contain space diff --git a/testsuite/support.py b/testsuite/support.py index d4878529..ce1ae216 100644 --- a/testsuite/support.py +++ b/testsuite/support.py @@ -165,7 +165,7 @@ def run_tests(filename): expected = [] for c in codes: c, sep, version = c.partition(':python') - if version and str(sys.version_info.major) != version: + if not sys.version.startswith(version): continue if c in ('Okay', 'noeol'): continue diff --git a/testsuite/test_api.py b/testsuite/test_api.py index 9699a647..8488189a 100644 --- a/testsuite/test_api.py +++ b/testsuite/test_api.py @@ -124,7 +124,7 @@ def test_styleguide(self): report = pep8.StyleGuide().check_files([E11]) stdout = sys.stdout.getvalue().splitlines() self.assertEqual(len(stdout), report.total_errors) - self.assertEqual(report.total_errors, 18, stdout) + self.assertEqual(report.total_errors, 17) self.assertFalse(sys.stderr) self.reset() @@ -181,7 +181,7 @@ def parse_argv(argstring): self.assertEqual(options.select, ()) self.assertEqual( options.ignore, - ('E121', 'E123', 'E126', 'E226', 'E24', 'E704') + ('E121', 'E123', 'E126', 'E226', 'E24', 'E704', 'W740', 'W741') ) options = parse_argv('--doctest').options diff --git a/testsuite/test_shell.py b/testsuite/test_shell.py index 3200be85..e536852f 100644 --- a/testsuite/test_shell.py +++ b/testsuite/test_shell.py @@ -75,10 +75,8 @@ def test_check_simple(self): stdout = stdout.splitlines() self.assertEqual(errcode, 1) self.assertFalse(stderr) - self.assertEqual(len(stdout), 18) + self.assertEqual(len(stdout), 17) for line, num, col in zip(stdout, (3, 6, 9, 12), (3, 6, 1, 5)): - if 'E901' in line: - continue path, x, y, msg = line.split(':') self.assertTrue(path.endswith(E11)) self.assertEqual(x, str(num)) From c31632167adfee12f5c2c730e5f99632e03275ee Mon Sep 17 00:00:00 2001 From: Sylvain Date: Wed, 26 Aug 2015 21:29:51 +0100 Subject: [PATCH 3/4] Fix tests on pypy SyntaxErrors were reported in a slightly different place making the tests fail. Removing error location for the 3 errors involved fixes the issue. --- testsuite/E22.py | 2 +- testsuite/E25.py | 2 +- testsuite/python3.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/testsuite/E22.py b/testsuite/E22.py index c8f26960..5a80e2fd 100644 --- a/testsuite/E22.py +++ b/testsuite/E22.py @@ -142,7 +142,7 @@ def halves(n): print >>sys.stderr, "x is out of range." print >> sys.stdout, "x is an integer." x = x / 2 - 1 -#: E901:2:6:python2 +#: E901:python2 if alpha[:-i]: *a, b = (1, 2, 3) diff --git a/testsuite/E25.py b/testsuite/E25.py index 71f49956..dddf2078 100644 --- a/testsuite/E25.py +++ b/testsuite/E25.py @@ -31,6 +31,6 @@ def foo(bar = False): d[type(None)] = _deepcopy_atomic # Annotated Function Definitions -#: E901:1:17:python2 +#: E901:python2 def munge(input: AnyStr, sep: AnyStr = None, limit=1000) -> AnyStr: pass diff --git a/testsuite/python3.py b/testsuite/python3.py index 194d5e94..efa4898a 100644 --- a/testsuite/python3.py +++ b/testsuite/python3.py @@ -2,6 +2,6 @@ # Annotated function (Issue #29) -#: E901:1:11:python2 +#: E901:python2 def foo(x: int) -> int: return x + 1 From fb81dc41983b5ada0519e7779a2221a639ca0a29 Mon Sep 17 00:00:00 2001 From: Sylvain Date: Sun, 30 Aug 2015 21:36:39 +0100 Subject: [PATCH 4/4] Enhance tests --- pep8.py | 2 +- testsuite/W74.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/pep8.py b/pep8.py index acdc0bc3..882ca9c8 100755 --- a/pep8.py +++ b/pep8.py @@ -763,7 +763,7 @@ def whitespace_around_named_parameter_equals(logical_line, tokens): Okay: boolean(a != b) Okay: boolean(a <= b) Okay: boolean(a >= b) - # Only with Python 3 - Okay: def foo(arg: int = 42):\n pass + Okay:python3 E901:python2: def foo(arg: int = 42):\n pass E251: def complex(real, imag = 0.0):\n return magic(r = real, i = imag) """ parens = 0 diff --git a/testsuite/W74.py b/testsuite/W74.py index 36fe278d..9efdc6e3 100644 --- a/testsuite/W74.py +++ b/testsuite/W74.py @@ -81,7 +81,6 @@ def goldbach_conjecture_ko(): return i -# W741:1:1 def return_finally1(): # return 1 try: return 1