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()',
+}
+
+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.

+
+
+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)
+    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
+
+
+class TreeVisitor(ast.NodeVisitor):
+    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):
+        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.

+        dead_code = False
+
+        for node in nodes:
+            if isinstance(node, ast.Pass):
+                pass_node = node
+
+            if bailed 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):
+                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'))
+
+        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))
+
+    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'))
+
+        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)
+
+    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 '
+                                           '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'))
+
+            left = right
+
+        self.generic_visit(node)
+
+    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)
+
+    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, '
+                                          'e.g. x += y instead x = x + y'))
+        self.generic_visit(node)
+
+
+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')
+
+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]
+            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")
+            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.

+
+        first_token = False
+
+check_quotes.name = 'abp-quotes'
+check_quotes.version = __version__