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
 
+try:
+    ascii
+except NameError:
+    ascii = repr
+
 __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'}
+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:

[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):
+                     nodes_required=True, docstring=False,
+                     can_have_unused_expr=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):
+            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
 
             self.errors.append((node, 'A203 unused expression'))
 
         if pass_node:
             if not nodes_required or len(nodes) > 1:
@@ skipping 10 matching lines @@
         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 {} '
+                            '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)
+        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)
+        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 not isinstance(scope.node,
-                                                         ast.ClassDef):
+        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):
+        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,
@@ skipping 56 matching lines @@
                                       '% 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.

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

+            self.errors.append((node, 'A301 use {}() instead of '
                                       '{}()'.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.

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

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

instead -> instead of

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

Done.

+                                      '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'):
@@ skipping 78 matching lines @@
                     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.

+                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
@@ skipping 39 matching lines @@
                     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__