Warn on Unassigned `==` Expression as Statement

[Scott Sanderson]

Working with some NumPy code recently, I accidentally wrote this:

<build output array>
<build mask array>

out[mask] == NaN # This should have been a single `=`, not a ==.
return out

After a few minutes of debugging my NaN-less array, I realized that I had accidentally written `==` instead of `=`, turning my assignment statement into no-op comparison operator.

I'd be nice if PyFlakes gave a warning for a statement that's just an expression performing an equality comparison. There are very few cases where I'd expect that something like this is doing what the author intended.

A more aggressive version of this check would be to warn on any expression that's just a binary operator. The assumption there would be that the "statement" `a + b` is at best dead code, and at worst an operator with a highly non-obvious side-effect.

I'd be happy to work on either version of this if there's support for it.

[John Vandenberg (jayvdb)]

A minimal fix detecting ast.Compare within ast.Expr is simple.

After a little playing, I think the best vector to approach this is to add checking of ast.Expr children nodes.
Expr is a fallback node type, only occurring when Assign, If, etc are not present. Generally speaking, Expr means an unused expression.
There are a few cases where it is needed, such as docstrings (Str), yield, Call being the main one, some dubious cases like Attribute and Name which should have side effects but often do, and then all binary, bool and compare ops which should ever be found in an Expr context.

One significant problem is the pyflakes test suite extensively uses code snippets that include unused expressions so the snippet is simple, so a good fix will need to alter lots of tests so the code tested is more like real code.

[John Vandenberg (jayvdb)]

"..like Attribute and Name which should have side effects but often do.."

should have been

"..like Attribute and Name which should *not* have side effects but often do.."

I've put up a PR for an even more aggressive solution that Scott proposed, reporting at any Expr which is likely unused.

https://github.com/pyflakes/pyflakes/pull/55

It doesnt introduce many tests explicitly about the new error condition; at present I am mostly interested in whether any sensible use of Expr are being prevented with this new error.

That still allows plain `foo` (ast.Name) which is almost always silly except when being used inside try block with exception NameError caught, and the more useful `foo.bar` (ast.Attribute) which is often used to trigger an AttributeError. I could add a bit of logic to only allow these two within a try: block.

Also allowed is `...` which in Python 3 can be used as a placeholder, and it should be a separate error condition if pyflakes is going to treat it as problematic. pyflakes tests also use that syntax.

[asmeurer (asmeurer)]

Strictly speaking, it is possible for a[b] == c to have side-effects, and hence be used as a standalone statement.

[John Vandenberg (jayvdb)]

Agreed. However many of the side-effects of "a[b] == c", most notably defaultdict population, could be obtained by using "a[b]" instead. "a[b]" can be permitted by adding ast.Subscript to the list of allowed Expr node types.

If pyflakes really needs to permit "a[b] == c", could we introduce a 'strict' mode that reports extremely dubious cases.

Worth noting there are only four hits in the cpython tip
One is Lib/getpass.py:177, which is using a tuple to assert two names exist in an imported module. This report would be avoided by being more permissive in try: blocks.

The other three of them are code introduced for http://bugs.python.org/issue23780 , which is intentionally triggering an exception.

[John Vandenberg (jayvdb)]

At the beginning of December I put up a patch for this: https://github.com/pyflakes/pyflakes/pull/55