Repository: PyCQA/pyflakes
Pull request #512 (2020-02-14 22:55:50)

Add a check for if statement conditions which are non-empty tuples
With the increasing prevalence of wrapping conditionals like:

``` python
    if (
        some_value != 'some_condition' and
        other_value is not 42
    )
```

and for wrapping other constructs with trailing commas, like:

``` python
    x = (
        some_value,
        other_value,
    )
```

it becomes quite easy to accidentally combine these into a conditional statement which is actually always true:

``` python
    if (
        some_value != 'some_condition' and
        other_value is not 42,
    )
```

This commit introduces a check for such constructs.

sigmavirus24: I'm worried about false positives here but I'm also failing to think of any examples where they might be valid

asottile: > 
> 
> I'm worried about false positives here but I'm also failing to think of any examples where they might be valid

I think any that would be problematic here would already be problematic for the existing `assert <tuple>` check -- so I imagine this to be pretty safe

Patch:
diff --git a/pyflakes/checker.py b/pyflakes/checker.py
index f886c26..f0b7c37 100644
--- a/pyflakes/checker.py
+++ b/pyflakes/checker.py
@@ -1409,14 +1409,14 @@ class Checker(object):
         pass
 
     # "stmt" type nodes
-    DELETE = PRINT = FOR = ASYNCFOR = WHILE = IF = WITH = WITHITEM = \
+    DELETE = PRINT = FOR = ASYNCFOR = WHILE = WITH = WITHITEM = \
         ASYNCWITH = ASYNCWITHITEM = TRYFINALLY = EXEC = \
         EXPR = ASSIGN = handleChildren
 
     PASS = ignore
 
     # "expr" type nodes
-    BOOLOP = UNARYOP = IFEXP = SET = \
+    BOOLOP = UNARYOP = SET = \
         REPR = ATTRIBUTE = \
         STARRED = NAMECONSTANT = NAMEDEXPR = handleChildren
 
@@ -1773,6 +1773,13 @@ class Checker(object):
                         )
         self.handleChildren(node)
 
+    def IF(self, node):
+        if isinstance(node.test, ast.Tuple) and node.test.elts != []:
+            self.report(messages.IfTuple, node)
+        self.handleChildren(node)
+
+    IFEXP = IF
+
     def ASSERT(self, node):
         if isinstance(node.test, ast.Tuple) and node.test.elts != []:
             self.report(messages.AssertTuple, node)
diff --git a/pyflakes/messages.py b/pyflakes/messages.py
index e93a493..1bb4ab0 100644
--- a/pyflakes/messages.py
+++ b/pyflakes/messages.py
@@ -233,9 +233,16 @@ class TooManyExpressionsInStarredAssignment(Message):
     message = 'too many expressions in star-unpacking assignment'
 
 
+class IfTuple(Message):
+    """
+    Conditional test is a non-empty tuple literal, which are always True.
+    """
+    message = '\'if tuple literal\' is always true, perhaps remove accidental comma?'
+
+
 class AssertTuple(Message):
     """
-    Assertion test is a tuple, which are always True.
+    Assertion test is a non-empty tuple literal, which are always True.
     """
     message = 'assertion is always true, perhaps remove parentheses?'
 


Test patch:
diff --git a/pyflakes/test/test_other.py b/pyflakes/test/test_other.py
index 282accb..7a02468 100644
--- a/pyflakes/test/test_other.py
+++ b/pyflakes/test/test_other.py
@@ -1433,6 +1433,29 @@ class TestUnusedAssignment(TestCase):
         self.flakes("a = foo if True else 'oink'", m.UndefinedName)
         self.flakes("a = 'moo' if True else bar", m.UndefinedName)
 
+    def test_if_tuple(self):
+        """
+        Test C{if (foo,)} conditions.
+        """
+        self.flakes("""if (): pass""")
+        self.flakes("""
+        if (
+            True
+        ):
+            pass
+        """)
+        self.flakes("""
+        if (
+            True,
+        ):
+            pass
+        """, m.IfTuple)
+        self.flakes("""
+        x = 1 if (
+            True,
+        ) else 2
+        """, m.IfTuple)
+
     def test_withStatementNoNames(self):
         """
         No warnings are emitted for using inside or after a nameless C{with}