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
@@ skipping 29 matching lines @@
 }
 
 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.

     try:
-        return eval(compile(ast.Expression(node), '', 'eval'), {})
+        return eval(compile(ast.Expression(node), '', 'eval'), names)
     except Exception:
         return VOLATILE
 
 
 def is_const(node):
     return evaluate(node) is not VOLATILE
 
 
 def get_identifier(node):
     if isinstance(node, ast.Name):
@@ skipping 414 matching lines @@
 
 # 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__