Add a check for if statement conditions which are non-empty tuples (#512)

* Add a check for if statement conditions which are non-empty tuples

With the increasing prevalence of wrapping conditionals like:

    if (
        some_value != 'some_condition' and
        other_value is not 42
    )

and for wrapping other constructs with trailing commas, like:

    x = (
        some_value,
        other_value,
    )

it becomes quite easy to accidentally combine these into a
conditional statement which is actually always true:

    if (
        some_value != 'some_condition' and
        other_value is not 42,
    )

This commit introduces a check for such constructs.

* Make this error message make sense

* Further clarity this message's docstring

Co-Authored-By: Ran Benita <ran234@gmail.com>

Co-authored-by: Ran Benita <ran234@gmail.com>

3 files changed, 39 insertions, 2 deletions

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 c8bf9c0..1bb4ab0 100644
--- a/pyflakes/messages.py
+++ b/pyflakes/messages.py
@@ -233,6 +233,13 @@ 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 non-empty tuple literal, which are always True.
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}