Noissue - Improved accuracy of evaluated expressions for A103 and A207

flake8-eyeo/flake8_eyeo.py

Index: flake8-eyeo/flake8_eyeo.py
===================================================================
--- a/flake8-eyeo/flake8_eyeo.py
+++ b/flake8-eyeo/flake8_eyeo.py
@@ -47,8 +47,9 @@
 
 
 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