Noissue - Added flake8 extension accounting for our coding style and some other stuff

flake8-abp/flake8_abp.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License

[Vasily Kuznetsov, 2016/04/22 14:48:26]

Probably we should have some tests for it too. It should not be too difficult to
set up, just have one (or more if needed) code snipped that triggers each error.

[Sebastian Noack, 2016/05/07 17:14:03]

Done.

 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 

[Vasily Kuznetsov, 2016/04/22 14:48:26]

It would be good to describe how to use this extension with flake8. Maybe a
docstring here or a README file next to it. It seems that it's actually enough
to just install it and flake8 starts picking it up, but describing that would be
useful for people who don't know.

 import ast
 import re
 import tokenize
 import sys
-
-__version__ = '0.1'
-
-DEPRECATED_APIS = {
-    ('re', 'match'): 'A101 use re.search() instead re.match()',
-    ('codecs', 'open'): 'A102 use io.open() instead codecs.open()',
+import collections
+
+try:
+    import builtins
+except ImportError:
+    import __builtin__ as builtins
+
+import pkg_resources
+
+try:
+    ascii
+except NameError:
+    ascii = repr
+
+__version__ = pkg_resources.get_distribution('flake8-abp').version
+
+DISCOURAGED_APIS = {
+    're.match': 're.search',
+    'codecs.open': 'io.open',
 }
 
-BAILOUT = (ast.Return, ast.Raise, ast.Continue, ast.Break)

[Vasily Kuznetsov, 2016/04/22 14:48:26]

Maybe something like LEAVE_BLOCK would be a better name?

[Sebastian Noack, 2016/05/07 17:14:01]

Done.

+ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply', 'buffer', 'coerce',
+                                           'intern', 'file'}
+
+LEAVE_BLOCK = (ast.Return, ast.Raise, ast.Continue, ast.Break)
+VOLATILE = object()
+
+
+def evaluate(node):
+    try:
+        return eval(compile(ast.Expression(node), '', 'eval'), {})
+    except Exception:
+        return VOLATILE
 
 
 def is_const(node):
-    if isinstance(node, (ast.Str, ast.Num)):
-        return True
-    if hasattr(ast, 'Bytes') and isinstance(node, ast.Bytes):
-        return True
-
-    if isinstance(node, (ast.Tuple, ast.List, ast.Set)):
-        return all(is_const(elt) for elt in node.elts)
+    return evaluate(node) is not VOLATILE
+
+
+def get_identifier(node):
     if isinstance(node, ast.Name):
-        return node.id in {'None', 'True', 'False'}
-    if isinstance(node, ast.BinOp):
-        return is_const(node.left) and is_const(node.right)
-    if isinstance(node, ast.UnaryOp):
-        return is_const(node.operand)
-
-    return False
+        return node.id
+    if isinstance(node, ast.Attribute) and isinstance(node.value, ast.Name):
+        return '{}.{}'.format(node.value.id, node.attr)
+
+
+def get_statement(node):
+    return type(node).__name__.lower()
 
 
 class TreeVisitor(ast.NodeVisitor):
+    Scope = collections.namedtuple('Scope', ['node', 'names', 'globals'])
+
     def __init__(self):
         self.errors = []
-        self.stack = []

[Vasily Kuznetsov, 2016/04/22 14:48:25]

Perhaps it would be good to give this a more descriptive name. Like
`scopes_stack` or something similar. Otherwise `TreeVisitor` does a lot of
things and it's not clear what this stack is for.

[Sebastian Noack, 2016/05/07 17:14:02]

Done.

-
-    def _visit_block(self, nodes, mandatory=False, docstring=False):
+        self.scope_stack = []
+
+    def _visit_block(self, nodes, block_required=False,
+                     nodes_required=True, docstring=False,
+                     can_have_unused_expr=False):
         pass_node = None
-        bailed = False

[Vasily Kuznetsov, 2016/04/22 14:48:25]

And here same as for BAILOUT, perhaps "left_block"?

[Sebastian Noack, 2016/05/07 17:14:02]

I went for leave_node, for consistency with pass_node, since we keep track of
the node that caused the block to be left for better error messages now.

+        has_non_pass = False
+        leave_node = None
         dead_code = False
 
-        for node in nodes:
+        for i, node in enumerate(nodes):
             if isinstance(node, ast.Pass):
                 pass_node = node
-
-            if bailed and not dead_code:
+            else:
+                has_non_pass = True
+
+            if leave_node and not dead_code:
                 dead_code = True
-                self.errors.append((node, 'A151 dead code after '
-                                          'return/raise/continue/break'))
-
-            if isinstance(node, BAILOUT):
-                bailed = True
-
-            if not isinstance(node, ast.Expr):
+                statement = get_statement(leave_node)
+                self.errors.append((node, 'A202 dead code after '
+                                          '{}'.format(statement)))
+
+            if isinstance(node, LEAVE_BLOCK):
+                leave_node = node
+
+            if can_have_unused_expr or not isinstance(node, ast.Expr):
+                continue
+            if docstring and i == 0 and isinstance(node.value, ast.Str):
                 continue
             if isinstance(node.value, (ast.Call, ast.Yield)):
                 continue
-            if docstring and node is nodes[0] and isinstance(node.value, ast.Str):

[Vasily Kuznetsov, 2016/04/22 14:48:25]

This line and A153 line are too long. Not by much though -- we should decide on
the max length.

[Sebastian Noack, 2016/05/07 17:14:01]

I made this line shorter by simply comparing against the loop index rather than
comparing this node against the first node.

-                continue
-
-            self.errors.append((node, 'A152 unused expression'))
+
+            self.errors.append((node, 'A203 unused expression'))
 
         if pass_node:
-            if len(nodes) > 1:
-                self.errors.append((pass_node, 'A153 redundant pass statement'))
-
-            if not mandatory and all(isinstance(n, ast.Pass) for n in nodes):
-                self.errors.append((pass_node, 'A154 empty block'))
-
-    def _visit_block_node(self, node, **kwargs):
-        self._visit_block(node.body, **kwargs)
-        if hasattr(node, 'orelse'):
-            self._visit_block(node.orelse)
-        if hasattr(node, 'finalbody'):
-            self._visit_block(node.finalbody)
-        self.generic_visit(node)
-
-    visit_Try = visit_TryExcept = visit_TryFinally = _visit_block_node
-
-    visit_ExceptHandler = visit_While = \
-        lambda self, node: self._visit_block_node(node, mandatory=True)
-
-    visit_Module = visit_ClassDef = \
-        lambda self, node: self._visit_block_node(node, mandatory=True,
-                                                        docstring=True)

[Vasily Kuznetsov, 2016/04/22 14:48:25]

E127 on this line, it should be under "node".

[Sebastian Noack, 2016/05/07 17:14:03]

This is one of indentation errors I disagree with. IMO, it reads much better if
you have a single positional argument but multiple keyword arguments to align
the keyword arguments. In the case here it will even be aligned with the keyword
argument in the function call above. I wonder whether this is actually necessary
for compliance with PEP-8 or whether the pep8 utility is just too strict?

However, due to other changes the lambda function here is gone anyway.

[Vasily Kuznetsov, 2016/05/09 13:39:50]

Although I follow your logic and it's probably indeed more readable this way, in
the examples from PEP8 there is nothing like it
(https://www.python.org/dev/peps/pep-0008/#indentation). So perhaps makes sense
to just follow what pep8 utility says, it seems like not a big deal to me.
Anyway, not important anymore.

While we're at it, I just realised that PEP8 now recommends line breaks before
binary operators (see
https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after...)
but pep8 actually would not accept such code.

-
-    def visit_Attribute(self, node):
-        if isinstance(node.ctx, ast.Load) and isinstance(node.value, ast.Name):
-            error = DEPRECATED_APIS.get((node.value.id, node.attr))
-            if error:
-                self.errors.append((node, error))
-        self.generic_visit(node)
-
-    def visit_ImportFrom(self, node):
-        for alias in node.names:
-            error = DEPRECATED_APIS.get((node.module, alias.name))
-            if error:
-                self.errors.append((node, error))
+            if not nodes_required or len(nodes) > 1:
+                self.errors.append((pass_node, 'A204 redundant '
+                                               'pass statement'))
+
+            if not block_required and not has_non_pass:
+                self.errors.append((pass_node, 'A205 empty block'))
+
+    def _check_redundant_else(self, node, handlers, clause):
+        if not node.orelse:
+            return
+
+        for handler in handlers:
+            for child in handler.body:
+                if isinstance(child, LEAVE_BLOCK):
+                    leave_node = child
+                    break
+            else:
+                return
+
+        statement = get_statement(leave_node)
+        self.errors.append((node.orelse[0],
+                            'A206 Extraneous else statement after {} '
+                            'in {}-clause'.format(statement, clause)))
+
+    def visit_If(self, node):
+        self._visit_block(node.body, block_required=bool(node.orelse))
+        self._visit_block(node.orelse)
+        self._check_redundant_else(node, [node], 'if')
+        self.generic_visit(node)
+
+    def visit_Try(self, node):
+        self._visit_block(node.body, can_have_unused_expr=bool(node.handlers))
+        self._visit_block(node.orelse)
+        self._visit_block(node.finalbody)
+        self._check_redundant_else(node, node.handlers, 'except')
+        self.generic_visit(node)
+
+    def visit_TryExcept(self, node):
+        self._visit_block(node.body, can_have_unused_expr=True)
+        self._visit_block(node.orelse)
+        self._check_redundant_else(node, node.handlers, 'except')
+        self.generic_visit(node)
+
+    def visit_TryFinally(self, node):
+        self._visit_block(node.body)
+        self._visit_block(node.finalbody)
+        self.generic_visit(node)
+
+    def visit_ExceptHandler(self, node):
+        self._visit_block(node.body, block_required=True)
+        self.generic_visit(node)
+
+    def _visit_stored_name(self, node, name):
+        scope = self.scope_stack[-1]
+        scope.names.add(name)
+
+        if name in ESSENTIAL_BUILTINS and isinstance(scope.node,
+                                                     ast.FunctionDef):
+            self.errors.append((node, 'A302 redefined built-in ' + name))
+
+    def visit_Name(self, node):
+        if isinstance(node.ctx, (ast.Store, ast.Param)):
+            self._visit_stored_name(node, node.id)
+
+    def visit_arg(self, node):
+        self._visit_stored_name(node, node.arg)
+
+    def _visit_with_scope(self, node):
+        scope = self.Scope(node, names=set(), globals=[])
+        self.scope_stack.append(scope)
+        self.generic_visit(node)
+        del self.scope_stack[-1]
+        return scope
+
+    def visit_Module(self, node):
+        self._visit_block(node.body, block_required=True,
+                          nodes_required=False, docstring=True)
+        self._visit_with_scope(node)
+
+    def visit_FunctionDef(self, node):
+        self._visit_stored_name(node, node.name)
+        self._visit_block(node.body, block_required=True, docstring=True)
+
+        scope = self._visit_with_scope(node)
+        global_names = set()
+
+        for declaration in scope.globals:
+            for name in declaration.names:
+                if name not in scope.names or name in global_names:
+                    statement = get_statement(declaration)
+                    self.errors.append((declaration,
+                                        'A201 redundant {} declaration for '
+                                        '{}'.format(statement, name)))
+                else:
+                    global_names.add(name)
+
+    visit_ClassDef = visit_FunctionDef
+
+    def visit_Global(self, node):
+        scope = self.scope_stack[-1]
+        scope.globals.append(node)
+
+        if isinstance(scope.node, ast.Module):
+            statement = get_statement(node)
+            self.errors.append((node, 'A201 {} declaration on '
+                                      'top-level'.format(statement)))
+
+    visit_Nonlocal = visit_Global
+
+    def _visit_iter(self, node):
+        if isinstance(node, (ast.Tuple, ast.Set)):
+            self.errors.append((node, 'A101 use lists for data '
+                                      'that have order'))
+
+    def visit_comprehension(self, node):
+        self._visit_iter(node.iter)
+        self.generic_visit(node)
+
+    def visit_For(self, node):
+        self._visit_iter(node.iter)
+        self._visit_block(node.body, block_required=True)
+        self._visit_block(node.orelse)
+        self.generic_visit(node)
+
+    def visit_While(self, node):
+        self._visit_block(node.body, block_required=True)
+        self._visit_block(node.orelse)
+        self.generic_visit(node)
 
     def visit_BinOp(self, node):
         if isinstance(node.op, ast.Mod) and isinstance(node.left, ast.Str):
-            self.errors.append((node, 'A111 use format() instead % operator '

[Vasily Kuznetsov, 2016/04/22 14:48:26]

Nit: "instead % operator" -> "instead of % operator"

[Sebastian Noack, 2016/05/07 17:13:59]

Done.

-                                      'for string formatting'))
+            self.errors.append((node, 'A107 use format() instead of '
+                                      '% operator for string formatting'))
 
         multi_addition = (isinstance(node.op, ast.Add) and
                           isinstance(node.left, ast.BinOp) and
                           isinstance(node.left.op, ast.Add))
         if multi_addition and (isinstance(node.left.left, ast.Str) or
                                isinstance(node.left.right, ast.Str) or
                                isinstance(node.right, ast.Str)):
-            self.errors.append((node, 'A112 use format() instead + operator '

[Vasily Kuznetsov, 2016/04/22 14:48:26]

Same as above, "instead of + operator"

[Sebastian Noack, 2016/05/07 17:14:02]

Done.

-                                      'when concatenating >2 strings'))
-
-        self.generic_visit(node)
-
-    def visit_comprehension(self, node):
-        if isinstance(node.iter, (ast.Tuple, ast.Set)):
-            self.errors.append((node.iter, 'A121 use lists for data '
-                                           'that have order'))
-        self.generic_visit(node)
-
-    def visit_For(self, node):
-        self._visit_block(node.body, mandatory=True)
-        self.visit_comprehension(node)
+            self.errors.append((node, 'A108 use format() instead of '
+                                      '+ operator when concatenating '
+                                      'more than two strings'))
+
+        self.generic_visit(node)
 
     def visit_Compare(self, node):
         left = node.left
         single = len(node.ops) == 1
 
         for op, right in zip(node.ops, node.comparators):
             membership = isinstance(op, (ast.In, ast.NotIn))
             symmetric = isinstance(op, (ast.Eq, ast.NotEq, ast.Is, ast.IsNot))
 
             if membership and isinstance(right, (ast.Tuple, ast.List)):
-                self.errors.append((right, 'A122 use sets for distinct '
+                self.errors.append((right, 'A102 use sets for distinct '
                                            'unordered data'))
 
             consts_first = single and not membership or symmetric
             if consts_first and is_const(left) and not is_const(right):
-                self.errors.append((left, 'A123 yoda condition'))
+                self.errors.append((left, 'A103 yoda condition'))
 
             left = right
 
         self.generic_visit(node)
 
+    def _check_deprecated(self, node, name):
+        substitute = DISCOURAGED_APIS.get(name)
+        if substitute:
+            self.errors.append((node, 'A301 use {}() instead of '
+                                      '{}()'.format(substitute, name)))
+
     def visit_Call(self, node):
-        func = node.func
-        if isinstance(func, ast.Name) and len(node.args) > 0:
-            arg = node.args[0]
-
-            if isinstance(arg, ast.Lambda) and func.id in {'filter', 'map'}:
-                self.errors.append((node, 'A131 use a comprehension '
-                                          'instead calling {}() with '
-                                          'lambda function'.format(func.id)))
-
-            if isinstance(arg, (ast.ListComp, ast.GeneratorExp)):
-                redundant_comp = func.id in {'list', 'set'}
-                if func.id == 'dict':
-                    redundant_comp = isinstance(arg.elt, (ast.Tuple, ast.List))
-
-                if redundant_comp:
-                    self.errors.append((node, 'A132 use a {0} comprehension '
-                                              'instead calling the {0}() '
-                                              'constructor'.format(func.id)))
-

[Vasily Kuznetsov, 2016/04/22 14:48:27]

We could probably also watch for list/tuple/set literals in calls to
list/tuple/set here. If we are not allowing set([i for i in foo]), it makes
sense to also disallow set([1, 2, 3]).

[Sebastian Noack, 2016/05/07 17:14:01]

Done. However, I intentionally ignore set literals as there is a legit use case
for list({x, y, z}), i.e. to exclude duplicated.

[Vasily Kuznetsov, 2016/05/09 13:39:50]

Acknowledged.

-        self.generic_visit(node)
-
-    def visit_FunctionDef(self, node):
-        self._visit_block(node.body, mandatory=True, docstring=True)
-
-        self.stack.append((set(), []))
-        self.generic_visit(node)
-        targets, globals = self.stack.pop()
-
-        for var in globals:
-            if any(name not in targets for name in var.names):
-                self.errors.append((var, 'A141 redundant global/nonlocal '
-                                         'declaration'))
-
-    def visit_Name(self, node):
-        if self.stack and isinstance(node.ctx, ast.Store):
-            self.stack[-1][0].add(node.id)
-
-    def visit_Global(self, node):
-        if self.stack:
-            self.stack[-1][1].append(node)
-        else:
-            self.errors.append((node, 'A141 global/nonlocal declaration '

[Vasily Kuznetsov, 2016/04/22 14:48:26]

This complains at the following snippet:

t = 1

class A:
    global t
    t = 2

I'm not sure what could be the use case for such assignments to a global
variable from inside the class scope, but perhaps we shouldn't immediately
forbid it.

[Sebastian Noack, 2016/05/07 17:14:03]

Done.

-                                      'on top-level'))
-
-    visit_Nonlocal = visit_Global
-
-    def visit_If(self, node):
-        has_else = bool(node.orelse)
-
-        if has_else and any(isinstance(n, BAILOUT) for n in node.body):
-            self.errors.append((node, 'A159 redundant else statement after '

[Vasily Kuznetsov, 2016/04/22 14:48:26]

This error is shown for the location of `if`. Would be better to show it on the
`else` instead.

[Sebastian Noack, 2016/05/07 17:13:59]

Well, the position of else is unknown since the else block is part of the If
node. However, I changed it to indicate the position of the first node in the if
block.

-                                      'return/raise/continue/break '
-                                      'in if-clause'))
-
-        self._visit_block(node.body, mandatory=has_else)
-        self._visit_block(node.orelse)
-        self.generic_visit(node)
+        func = get_identifier(node.func)
+        arg = next(iter(node.args), None)
+        redundant_literal = False
+
+        if isinstance(arg, ast.Lambda) and func in {'map', 'filter',
+                                                    'imap', 'ifilter',
+                                                    'itertools.imap',
+                                                    'itertools.ifilter'}:
+            self.errors.append((node, 'A104 use a comprehension '
+                                      'instead of calling {}() with '
+                                      'lambda function'.format(func)))
+        elif isinstance(arg, (ast.List, ast.Tuple)):
+            if func == 'dict':
+                redundant_literal = all(isinstance(elt, (ast.Tuple, ast.List))
+                                        for elt in arg.elts)
+            else:
+                redundant_literal = func in {'list', 'set', 'tuple'}
+        elif isinstance(arg, (ast.ListComp, ast.GeneratorExp)):
+            if func == 'dict':
+                redundant_literal = isinstance(arg.elt, (ast.Tuple, ast.List))
+            else:
+                redundant_literal = func in {'list', 'set'}
+
+        if redundant_literal:
+            self.errors.append((node, 'A105 use a {0} literal or '
+                                      'comprehension instead of calling '
+                                      '{0}()'.format(func)))
+
+        self._check_deprecated(node, func)
+        self.generic_visit(node)
+
+    def visit_Import(self, node):
+        for alias in node.names:
+            self._visit_stored_name(node, alias.asname or alias.name)
+
+            if hasattr(node, 'module'):
+                self._check_deprecated(node, '{}.{}'.format(node.module,
+                                                            alias.name))
+
+    visit_ImportFrom = visit_Import
 
     def visit_Assign(self, node):
         if isinstance(node.value, ast.BinOp) and len(node.targets) == 1:
             target = node.targets[0]
             left_is_target = (isinstance(target, ast.Name) and
                               isinstance(node.value.left, ast.Name) and
                               target.id == node.value.left.id)
             if left_is_target:
-                self.errors.append((node, 'A161 use in-place assignment, '
+                self.errors.append((node, 'A106 use augment assignment, '
                                           'e.g. x += y instead x = x + y'))
         self.generic_visit(node)
+
+    def _visit_hash_keys(self, nodes, what):
+        keys = []
+        for node in nodes:
+            key = evaluate(node)
+            if key is VOLATILE:
+                continue
+
+            if key in keys:
+                self.errors.append((node, 'A207 duplicate ' + what))
+                continue
+
+            keys.append(key)
+
+    def visit_Dict(self, node):
+        self._visit_hash_keys(node.keys, 'key in dict')
+
+    def visit_Set(self, node):
+        self._visit_hash_keys(node.elts, 'item in set')
 
 
 class ASTChecker(object):
     name = 'abp'
     version = __version__
 
     def __init__(self, tree, filename):
         self.tree = tree
 
     def run(self):
         visitor = TreeVisitor()
         visitor.visit(self.tree)
 
         for node, error in visitor.errors:
             yield (node.lineno, node.col_offset, error, type(self))
 
 
 def check_non_default_encoding(physical_line, line_number):
-    if (line_number <= 2 and re.search(r'^\s*#.*coding[:=]', physical_line)):
-        return (0, 'A201 non-default file encoding')
+    if line_number <= 2 and re.search(r'^\s*#.*coding[:=]', physical_line):
+        return (0, 'A303 non-default file encoding')
 
 check_non_default_encoding.name = 'abp-non-default-encoding'
 check_non_default_encoding.version = __version__
 
 
 def check_quotes(logical_line, tokens, previous_logical):
     first_token = True
-    offset = 0
 
     for kind, token, start, end, _ in tokens:
-        if kind == tokenize.INDENT:
-            offset = end[1]
+        if kind == tokenize.INDENT or kind == tokenize.DEDENT:
             continue
 
         if kind == tokenize.STRING:
-            pos = start[1] - offset
             match = re.search(r'^(u)?(b)?(r)?((""")?.*)$',
                               token, re.IGNORECASE | re.DOTALL)
             (is_unicode, is_bytes, is_raw,
              literal, has_doc_quotes) = match.groups()
 
             if first_token and re.search(r'^(?:(?:def|class)\s|$)',
                                          previous_logical):
                 if not has_doc_quotes:
-                    yield (pos, 'A301 use triple double quotes for docstrings')
-                if is_unicode or is_bytes or is_raw:
-                    yield (pos, "A302 don't use u, b or r for doc strings")
+                    yield (start, 'A109 use triple double '
+                                  'quotes for docstrings')
+                elif is_unicode or is_bytes or is_raw:
+                    yield (start, "A109 don't use u'', b'' "
+                                  "or r'' for doc strings")
             elif start[0] == end[0]:
                 if is_raw:
                     literal = re.sub(r'\\(?!{})'.format(literal[0]),

[Vasily Kuznetsov, 2016/04/22 14:48:25]

I think I still don't quite get the point of this but it seems to work right.
Why do we need to replace the quotes in raw strings?

[Sebastian Noack, 2016/05/07 17:14:01]

Well, the whole point of raw strings is that backslashes are interpreted
literally rather than escaping the next character (except when the next
character is a quote that needs to be escaped). So r'\ ' is equivalent to '\\ ',
and since repr() always uses the latter (non-raw) representation we have to
account for that.

                                      '\\\\\\\\', literal)
 
                 if sys.version_info[0] >= 3:
                     if is_bytes:
                         literal = 'b' + literal
-                    else:
-                        literal = re.sub(r'(?<!\\)\\x(?!a[0d])([a-f][0-9a-f])',
-                                         lambda m: chr(int(m.group(1), 16)),
-                                         literal)
                 elif is_unicode:
                     literal = 'u' + literal
 
-                if repr(eval(literal)) != literal:
-                    yield (pos, "A311 string literal doesn't match repr()")

[Vasily Kuznetsov, 2016/04/22 14:48:26]

It would be nice to explain in more detail what exactly is the problem here and
how it can be fixed. I can' come up with a nice short message though, so perhaps
this should be done in our style guide instead.

[Sebastian Noack, 2016/05/07 17:14:00]

Yes, that is something for the style guide. I will update it once this code has
been landed.

+                if ascii(eval(literal)) != literal:
+                    yield (start, "A110 string literal doesn't match"
+                                  '{}()'.format(ascii.__name__))
 
         first_token = False
 
 check_quotes.name = 'abp-quotes'
 check_quotes.version = __version__
+
+
+def check_redundant_parenthesis(logical_line, tokens):
+    start_line = tokens[0][2][0]
+    level = 0
+    statement = None
+
+    for i, (kind, token, _, end, _) in enumerate(tokens):
+        if kind == tokenize.INDENT or kind == tokenize.DEDENT:
+            continue
+
+        if statement is None:
+            # logical line doesn't start with an if, elif or while statement
+            if kind != tokenize.NAME or token not in {'if', 'elif', 'while'}:
+                break
+
+            # expression doesn't start with parenthesis
+            next_token = tokens[i + 1]
+            if next_token[:2] != (tokenize.OP, '('):
+                break
+
+            # expression is empty tuple
+            if tokens[i + 2][:2] == (tokenize.OP, ')'):
+                break
+
+            statement = token
+            pos = next_token[2]
+            continue
+
+        # expression ends on a different line, parenthesis are necessary
+        if end[0] > start_line:
+            break
+
+        if kind == tokenize.OP:
+            if token == ',':
+                # expression is non-empty tuple
+                if level == 1:
+                    break
+            elif token == '(':
+                level += 1
+            elif token == ')':
+                level -= 1
+                if level == 0:
+                    # outer parenthesis closed before end of expression
+                    if tokens[i + 1][:2] != (tokenize.OP, ':'):
+                        break
+
+                    return [(pos, 'A111 redundant parenthesis for {} '
+                                  'statement'.format(statement))]
+
+    return []
+
+check_redundant_parenthesis.name = 'abp-redundant-parenthesis'
+check_redundant_parenthesis.version = __version__