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
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+
+import ast
+import re
+import tokenize
+import sys
+import collections
+
+try:
+    import builtins
+except ImportError:
+    import __builtin__ as builtins
+
+import pkg_resources
+
+__version__ = pkg_resources.get_distribution('flake8-abp').version
+
+DISCOURAGED_APIS = {
+    're.match': 're.search',
+    'codecs.open': 'io.open',
+}
+
+ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply', 'basestring', 'buffer',

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

What were the criteria for deciding what is an essential builtin? Maybe we
should document them somewhere. Otherwise it's not clear why, for example, it's
considered ok to redefine things like `unicode` and `basestring` but redefining
`id`, which is a useful name sometimes and is rarely used as a function, is not
acceptable.

[Sebastian Noack, 2016/05/09 16:49:11]

The names explicitly excluded were the builtins that have been removed in Python
3. Some of them must be defined in compatibility code, and others are already
discouraged in Python 2 and can just be ignored.

However, I figured that it might better to just ignore top-level module code
here to account for compatibility code. Also note that we already ignored class
level code to allow for class attributes that would conflict with builtins.
While only checking for built-in redefinition in methods now, I cut the list of
deprecated builtins that are allowed to override.

[Vasily Kuznetsov, 2016/05/09 17:37:32]

Yep, makes sense. It would be good to record it somewhere, at least in a comment
here. Or maybe this logic really belongs to the style guide.

[Sebastian Noack, 2016/05/11 10:50:09]

I think the general rule for the style guide should be:

  Don't override built-in names except for outdated built-ins

Note that compatibility code like

  try:
      ascii
  except NameError:
      ascii = repr

doesn't override builtins, since it only assigns to ascii (which is a built-in
in Python 3) if it doesn't exist. However, without actually running the code we
won't know. So we have to account for it by other means here. But it's not an
exception to the general rule.

As for what built-ins are considered essential see the Python documentation:
https://docs.python.org/2/library/functions.html#non-essential-built-in-funcs

However, while "file" isn't listed as non-essential builtin there, I consider it
one given the argumentation there. It's only useful for strict type checks which
you should rather avoid anyway. And using "file" as variable name is arguably
more readable than using "fp" or "handle".

[Vasily Kuznetsov, 2016/05/11 14:09:17]

Ok, I think saying that builtins should be overridden except for non-essential
ones from Python docs and `file` is clear and short. In general I agree with all
your logic here.

+                                           'cmp', 'coerce', 'execfile',
+                                           'file', 'intern', 'long',
+                                           'raw_input', 'reduce', 'reload',
+                                           'unichr', 'unicode', 'xrange'}
+
+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:

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

As I understand this exception would be thrown when there are external symbols
on which the node depends. Can it be anything other than NameError?
If not, maybe we should narrow down this exception to NameError?

[Sebastian Noack, 2016/05/09 16:49:11]

Yes, it can be one of a wide range (if not any) exception. Just a few examples
of expressions that the given node might hold, causing different exceptions:

0/0                 -> ZeroDivisionError
eval('3ewr*/+%')    -> SyntaxError
__import__('foo')   -> ImportError
hash([])            -> TypeError
int("x")            -> ValueError
'ä'.decode('ascii') -> UnicodeDecodeError
None.foo            -> AttributeError

[Vasily Kuznetsov, 2016/05/09 17:37:32]

I see. I just got a bit confused by this being called VOLATILE and the following
is_const function. So actually VOLATILE means that it depends on the environment
in some way or perhaps even always causes error (but not our error). Yeah, I
guess it makes sense to just catch `Exception` then.

+        return VOLATILE
+
+
+def is_const(node):
+    return evaluate(node) is not VOLATILE
+
+
+def get_identifier(node):
+    if isinstance(node, ast.Name):
+        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.scope_stack = []
+
+    def _visit_block(self, nodes, block_required=False,
+                     nodes_required=True, docstring=False):
+        pass_node = None
+        has_non_pass = False
+        leave_node = None
+        dead_code = False
+
+        for i, node in enumerate(nodes):
+            if isinstance(node, ast.Pass):
+                pass_node = node
+            else:
+                has_non_pass = True
+
+            if leave_node and not dead_code:
+                dead_code = True
+                statement = get_statement(leave_node)
+                self.errors.append((node, 'A202 dead code after '
+                                          '{}'.format(statement)))
+
+            if isinstance(node, LEAVE_BLOCK):
+                leave_node = node
+
+            if 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
+
+            self.errors.append((node, 'A203 unused expression'))
+
+        if pass_node:
+            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 redundant 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)
+        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)
+        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 not isinstance(scope.node,
+                                                         ast.ClassDef):
+            self.errors.append((node, 'A302 redefined built-in ' + name))
+
+    def visit_Name(self, node):
+        if isinstance(node.ctx, ast.Store):
+            self._visit_stored_name(node, node.id)
+
+    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, '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, 'A108 use format() instead of '
+                                      '+ operator when concatenating '
+                                      '>2 strings'))

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

Maybe write it out: "more than two strings"? It's longer but I think a bit more
readable.

[Sebastian Noack, 2016/05/09 16:49:11]

I had it spelled out initially, but the message is already quite long, and it
certainly sticks out when spelled out. However, I changed it back for now.

[Vasily Kuznetsov, 2016/05/09 17:37:32]

I see. I don't feel too strongly about it, so up to you.

+
+        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, '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, '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 '

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

instead -> instead of

[Sebastian Noack, 2016/05/09 16:49:12]

Done.

+                                      '{}()'.format(substitute, name)))
+
+    def visit_Call(self, 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 calling {}() with '

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

instead -> instead of

[Sebastian Noack, 2016/05/09 16:49:11]

Done.

+                                      '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 calling '

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

instead -> instead of

[Sebastian Noack, 2016/05/09 16:49:11]

Done.

+                                      '{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, '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, '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
+
+    for kind, token, start, end, _ in tokens:
+        if kind == tokenize.INDENT or kind == tokenize.DEDENT:
+            continue
+
+        if kind == tokenize.STRING:
+            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 (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]),
+                                     '\\\\\\\\', 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 (start, "A110 string literal doesn't match repr()")

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

Maybe a bit clearer: "string literal doesn't match its repr()". It's still not
self-sufficient of course, and it's not alone. We will probably need to explain
the rationale for most of these errors somewhere anyway, so I guess the
elaborate version will go into that document.

[Sebastian Noack, 2016/05/09 16:49:12]

I think that would be inaccurate. A string (literal) doesn't have a repr(), it's
an independent function that can be called on a string.

[Vasily Kuznetsov, 2016/05/09 17:37:32]

So is it about the difference between a string and a string literal or about the
inaccuracy of calling `repr(foo)` "a repr() of foo"?

The first objection is technically right, the literal doesn't have a repr(),
it's just part of the program source code and what does have a repr() is the
string that is created when the program is executed. But I was thinking that
distinguishing the string from the literal that it's created from is a bit too
much for our purposes here.

The second one depends on your point of view. If you think strictly
object-orientedly then a repr() of foo would be `foo.repr()` and not
`repr(foo)`. But Python has some functional programming in it as well. I think
most people would understand what I mean by len() of foo (`len(foo)`) and
similarly repr() of foo seemed to be ok. Perhaps we should ask a third person to
see what makes more sense.

[Sebastian Noack, 2016/05/11 10:50:09]

Yeah, I disagree. But also note that on Python 3 this message would be "A110
string literal doesn't match its ascii()" now, which sounds even more awkward to
me. What do you think?

[Vasily Kuznetsov, 2016/05/11 14:09:17]

Yeah, I agree with you that the proposed Python 3 message sounds very awkward.
So to be consistent, let's leave the wording as is.

+
+        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__