Noissue - Improved accuracy of evaluated expressions for A103 and A207

flake8-eyeo/flake8_eyeo.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # Copyright (C) 2006-present 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
-import flake8
 
 try:
     ascii
 except NameError:
     ascii = repr
 
 __version__ = pkg_resources.get_distribution('flake8-eyeo').version
 
 DISCOURAGED_APIS = {
     're.match': 're.search',
     'codecs.open': 'io.open',
 }
 
 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):
-    names = {'__builtins__': {'True': True, 'False': False, 'None': None}}

[tlucas, 2017/10/06 11:41:36]

Why are these values necessary?

From what i understand, they only seem to be needed to get A207 (duplicate keys)
evaluated correctly, which also seems to work out of the box for python3*.

However, providing those values produces the following output for the given
example:

if False == foo:
    bar()

if True == foo:
    bar()

./main.py:1:4: A103 yoda condition
./main.py:1:10: E712 comparison to False should be 'if cond is False:' or 'if
not cond:'
./main.py:4:4: A103 yoda condition
./main.py:4:9: E712 comparison to True should be 'if cond is True:' or 'if
cond:'


If you would only set names to 'names = {'__builtins__': {}}' the following
(IMHO more valuable) output would be produced:

./main.py:1:10: E712 comparison to False should be 'if cond is False:' or 'if
not cond:'
./main.py:4:9: E712 comparison to True should be 'if cond is True:' or 'if cond:


Since evaluate() is only called for is_const() (A103) or the A207 checks, you
could add an argument 'names' to evaluate(), which could default to {}.

What do you think about this?

[Sebastian Noack, 2017/10/06 18:11:09]

True, False and None are special. They are the only builtins which are
primitives (i.e. immutable values of scalar built-in types). Semantically they
are more similar to numeric or string literals. That the Python tokenizer treats
them as names which are then resolved as builtins, is just an implementation
detail. However, in Python 3, they are read-only (and redefining them in Python
2 would break almost every program). So it is safe to assume that any occurrence
of False, True and None in the source code refer to the original value, while
other builtins are occasionally overridden. Hence treating every built-in but
True/False/None as volatile seems logical, both for A103 and A207.

Yes, `False == foo` is both violating A103 and E712. But from the different
column they indicate, you can see that E712 is referring to the operator, while
A103 is referring to the order of operands. So it is not really redundant. Also
that means that `False is foo` only violates A103 but not E712.

+def evaluate(node, namespace):
     try:
-        return eval(compile(ast.Expression(node), '', 'eval'), names)
+        return eval(compile(ast.Expression(node), '', 'eval'), namespace)
     except Exception:
         return VOLATILE
 
 
 def is_const(node):
-    return evaluate(node) is not VOLATILE
+    namespace = {'__builtins__': {'True': True, 'False': False, 'None': None}}
+    return evaluate(node, namespace) 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):
@@ skipping 261 matching lines @@
             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 = []
+        namespace = collections.defaultdict(object, vars(builtins))

[Vasily Kuznetsov, 2017/10/11 19:29:02]

This is clever!

         for node in nodes:
-            key = evaluate(node)
+            key = evaluate(node, namespace)
             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):
-    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_ast(tree):
+    visitor = TreeVisitor()
+    visitor.visit(tree)
+
+    for node, error in visitor.errors:
+        yield (node.lineno, node.col_offset, error, None)
 
 
 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')
 
 
 def check_quotes(logical_line, tokens, previous_logical, checker_state):
     first_token = True
 
@@ skipping 13 matching lines @@
             prefixes, quote, text = match.groups()
             prefixes = prefixes.lower()
 
             if 'u' in prefixes:
                 yield (start, 'A112 use "from __future__ import '
                               'unicode_literals" instead of '
                               'prefixing literals with "u"')
 
             if first_token and re.search(r'^(?:(?:def|class)\s|$)',
                                          previous_logical):
-                if quote != '"""':
-                    yield (start, 'A109 use triple double '
-                                  'quotes for docstrings')
+                pass  # Ignore docstrings
             elif start[0] != end[0]:
-                pass
+                pass  # Ignore multiline strings
             elif 'r' in prefixes:
                 if quote != "'" and not (quote == '"' and "'" in text):
                     yield (start, 'A110 use single quotes for raw string')
             else:
                 prefix = ''
                 if sys.version_info[0] >= 3:
                     if 'b' in prefixes:
                         prefix = 'b'
                 else:
                     u_literals = checker_state.get('has_unicode_literals')
@@ skipping 52 matching lines @@
                     # 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 []
 
 
-# With flake8 3, the way the entry points are register in setup.py,
-# they are recognized as a group, and the name and version is detected
-# automatically. For compatibility with flake8 2, however, we need to
-# assign the name and version to each checker individually.
-if int(flake8.__version__.split('.')[0]) < 3:
-    for checker in [ASTChecker, check_non_default_encoding,
-                    check_quotes, check_redundant_parenthesis]:
-        checker.name = 'eyeo'
-        checker.version = __version__
+for checker in [check_ast, check_non_default_encoding,
+                check_quotes, check_redundant_parenthesis]:
+    checker.name = 'eyeo'
+    checker.version = __version__