Fix false positives for superfluous-parens (#4784)

* Split functional tests for ``superfluous-parents`` * Fix false positives for superfluous-parens This fixes the false positives identified in #2818, #3249, #3608 & #4346 All false positives reported fell under keywords before walrus operator or if-keyword within generators/comprehension. This closes #2818, closes #3429, closes #3608, closes #4346 * Move the superfluous functional tests to functional/s/super Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
pylint-dev · Aug 3, 2021 · 85b69c9 · 85b69c9
1 parent 7bb5043
commit 85b69c9
Show file tree

Hide file tree

Showing 11 changed files with 168 additions and 45 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -68,6 +68,13 @@ Release date: TBA
 
   Closes #4681
 
+* Fix false positives for ``superfluous-parens`` with walrus operator, ternary operator and inside list comprehension.
+
+  Closes #2818
+  Closes #3249
+  Closes #3608
+  Closes #4346
+
 
 What's New in Pylint 2.9.6?
 ===========================

diff --git a/doc/whatsnew/2.10.rst b/doc/whatsnew/2.10.rst
@@ -54,3 +54,10 @@ Other Changes
   Ref #1162
   Closes #1990
   Closes #4168
+
+* Fix false positives for ``superfluous-parens`` with walrus operator, ternary operator and inside list comprehension.
+
+  Closes #2818
+  Closes #3249
+  Closes #3608
+  Closes #4346
diff --git a/pylint/checkers/format.py b/pylint/checkers/format.py
@@ -37,6 +37,7 @@
 # Copyright (c) 2019 Hugo van Kemenade <hugovk@users.noreply.github.com>
 # Copyright (c) 2020 Raphael Gaschignard <raphael@rtpg.co>
 # Copyright (c) 2021 Marc Mueller <30130371+cdce8p@users.noreply.github.com>
+# Copyright (c) 2021 Daniel van Noord <13665637+DanielNoord@users.noreply.github.com>
 
 # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
 # For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
@@ -374,6 +375,7 @@ def _check_keyword_parentheses(
         found_and_or = False
         contains_walrus_operator = False
         walrus_operator_depth = 0
+        contains_double_parens = 0
         depth = 0
         keyword_token = str(tokens[start].string)
         line_num = tokens[start].start[0]
@@ -393,19 +395,25 @@ def _check_keyword_parentheses(
                 walrus_operator_depth = depth
             if token.string == "(":
                 depth += 1
+                if tokens[i + 1].string == "(":
+                    contains_double_parens = 1
             elif token.string == ")":
                 depth -= 1
                 if depth:
+                    if contains_double_parens and tokens[i + 1].string == ")":
+                        self.add_message(
+                            "superfluous-parens", line=line_num, args=keyword_token
+                        )
+                        return
+                    contains_double_parens = 0
                     continue
                 # ')' can't happen after if (foo), since it would be a syntax error.
                 if tokens[i + 1].string in (":", ")", "]", "}", "in") or tokens[
                     i + 1
                 ].type in (tokenize.NEWLINE, tokenize.ENDMARKER, tokenize.COMMENT):
-                    # The empty tuple () is always accepted.
                     if contains_walrus_operator and walrus_operator_depth - 1 == depth:
-                        # Reset variable for possible following expressions
-                        contains_walrus_operator = False
-                        continue
+                        return
+                    # The empty tuple () is always accepted.
                     if i == start + 2:
                         return
                     if keyword_token == "not":
@@ -417,7 +425,7 @@ def _check_keyword_parentheses(
                         self.add_message(
                             "superfluous-parens", line=line_num, args=keyword_token
                         )
-                    elif not found_and_or:
+                    elif not found_and_or and keyword_token != "in":
                         self.add_message(
                             "superfluous-parens", line=line_num, args=keyword_token
                         )
@@ -440,6 +448,13 @@ def _check_keyword_parentheses(
                 # without an error.
                 elif token[1] == "for":
                     return
+                # A generator expression can have a 'else' token in it.
+                # We check the rest of the tokens to see if any problems incure after
+                # the 'else'.
+                elif token[1] == "else":
+                    if "(" in (i.string for i in tokens[i:]):
+                        self._check_keyword_parentheses(tokens[i:], 0)
+                    return
 
     def _prepare_token_dispatcher(self):
         dispatch = {}

diff --git a/tests/checkers/unittest_format.py b/tests/checkers/unittest_format.py
@@ -21,6 +21,7 @@
 # Copyright (c) 2020 hippo91 <guillaume.peillex@gmail.com>
 # Copyright (c) 2021 Marc Mueller <30130371+cdce8p@users.noreply.github.com>
 # Copyright (c) 2021 Andreas Finkler <andi.finkler@gmail.com>
+# Copyright (c) 2021 Daniel van Noord <13665637+DanielNoord@users.noreply.github.com>
 
 # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
 # For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
@@ -152,6 +153,8 @@ def testCheckKeywordParensHandlesValidCases(self):
             "for x in (x for x in x):",
             "not (foo or bar)",
             "not (foo or bar) and baz",
+            "return [x for x in (3 if 1 else [4])]",
+            "return (x for x in ((3, 4) if 2 > 1 else (5, 6)))",
         ]
         with self.assertNoMessages():
             for code in cases:
@@ -188,24 +191,24 @@ def testCheckKeywordParensHandlesUnnecessaryParens(self):
 
     def testNoSuperfluousParensWalrusOperatorIf(self):
         """Parenthesis change the meaning of assignment in the walrus operator
-        and so are not superfluous:"""
-        code = "if (odd := is_odd(i))"
-        offset = 0
-        with self.assertNoMessages():
-            self.checker._check_keyword_parentheses(_tokenize_str(code), offset)
+        and so are not always superfluous:"""
+        cases = [
+            ("if (odd := is_odd(i))\n"),
+            ("not (foo := 5)\n"),
+        ]
+        for code in cases:
+            with self.assertNoMessages():
+                self.checker.process_tokens(_tokenize_str(code))
 
     def testPositiveSuperfluousParensWalrusOperatorIf(self):
-        """Test positive superfluous parens with the walrus operator"""
-        code = "if ((odd := is_odd(i))):"
-        msg = Message("superfluous-parens", line=1, args="if")
-        with self.assertAddsMessages(msg):
-            self.checker._check_keyword_parentheses(_tokenize_str(code), 0)
-
-    def testNoSuperfluousParensWalrusOperatorNot(self):
-        """Test superfluous-parens with the not operator"""
-        code = "not (foo := 5)"
-        with self.assertNoMessages():
-            self.checker._check_keyword_parentheses(_tokenize_str(code), 0)
+        """Test positive superfluous parens cases with the walrus operator"""
+        cases = [
+            (Message("superfluous-parens", line=1, args="if"), "if ((x := y)):\n"),
+            (Message("superfluous-parens", line=1, args="not"), "if not ((x := y)):\n"),
+        ]
+        for msg, code in cases:
+            with self.assertAddsMessages(msg):
+                self.checker.process_tokens(_tokenize_str(code))
 
     def testCheckIfArgsAreNotUnicode(self):
         cases = [("if (foo):", 0), ("assert (1 == 1)", 0)]

diff --git a/tests/functional/s/super/superfluous_parens.py b/tests/functional/s/super/superfluous_parens.py
@@ -0,0 +1,47 @@
+"""Test the superfluous-parens warning."""
+from __future__ import print_function
+# pylint: disable=unneeded-not, unnecessary-comprehension, missing-function-docstring, invalid-name, fixme
+A = 3
+if (A == 5):  # [superfluous-parens]
+    pass
+if not (A == 5):  # [superfluous-parens]
+    pass
+if not (3 or 5):
+    pass
+for (x) in (1, 2, 3):  # [superfluous-parens]
+    print(x)
+if (1) in (1, 2, 3):  # [superfluous-parens]
+    pass
+if (1, 2) in (1, 2, 3):
+    pass
+DICT = {'a': 1, 'b': 2}
+del(DICT['b'])  # [superfluous-parens]
+del DICT['a']
+
+B = [x for x in ((3, 4))]  # [superfluous-parens]
+C = [x for x in ((3, 4) if 1 > 0 else (5, 6))]
+D = [x for x in ((3, 4) if 1 > 0 else ((5, 6)))]  # [superfluous-parens]
+E = [x for x in ((3, 4) if 1 > 0 else ((((5, 6)))))]  # [superfluous-parens]
+
+# Test assertions
+F = "Version 1.0"
+G = "1.0"
+assert "Version " + G in F
+assert ("Version " + G) in F # [superfluous-parens]
+
+# Test assignment
+H = 2 + (5 * 3)
+NUMS_LIST = ['1', '2', '3']
+NUMS_SET = {'1', '2', '3'}
+NUMS_DICT = {'1': 1, '2': 2, '3': 3}
+
+# Test functions
+def function_A():
+    return (x for x in ((3, 4)))  # [superfluous-parens]
+
+# TODO: Test string combinations, see https://github.com/PyCQA/pylint/issues/4792
+# Lines 45, 46 & 47 should raise the superfluous-parens message
+I = "TestString"
+J = ("Test " + "String")
+K = ("Test " + "String") in I
+assert "" + ("Version " + "String") in I
diff --git a/tests/functional/s/super/superfluous_parens.txt b/tests/functional/s/super/superfluous_parens.txt
@@ -0,0 +1,10 @@
+superfluous-parens:5:0::Unnecessary parens after 'if' keyword:HIGH
+superfluous-parens:7:0::Unnecessary parens after 'not' keyword:HIGH
+superfluous-parens:11:0::Unnecessary parens after 'for' keyword:HIGH
+superfluous-parens:13:0::Unnecessary parens after 'if' keyword:HIGH
+superfluous-parens:18:0::Unnecessary parens after 'del' keyword:HIGH
+superfluous-parens:21:0::Unnecessary parens after 'in' keyword:HIGH
+superfluous-parens:23:0::Unnecessary parens after 'else' keyword:HIGH
+superfluous-parens:24:0::Unnecessary parens after 'else' keyword:HIGH
+superfluous-parens:30:0::Unnecessary parens after 'assert' keyword:HIGH
+superfluous-parens:40:0::Unnecessary parens after 'in' keyword:HIGH
diff --git a/tests/functional/s/super/superfluous_parens_walrus_py38.py b/tests/functional/s/super/superfluous_parens_walrus_py38.py
@@ -0,0 +1,51 @@
+"""Test the superfluous-parens warning with python 3.8 functionality (walrus operator)"""
+# pylint: disable=missing-function-docstring, invalid-name, missing-class-docstring, import-error
+import numpy
+
+# Test parens in if statements
+if not (x := False):
+    print(x)
+
+if not (foo := 5):
+    pass
+
+A = 1
+if odd := isinstance(A, int):
+    pass
+
+if not ((x := 1)):  # [superfluous-parens]
+    pass
+
+if ((x := A)):   # [superfluous-parens]
+    pass
+
+if not ((x := A)):  # [superfluous-parens]
+    pass
+
+if not ((((x := 1)))):  # [superfluous-parens]
+    pass
+
+# Test assertions
+assert (ret := str(42))
+
+# Teast 2D arrays with numpy (since they allow commas within indexing)
+ARRAY_2D = numpy.zeros((3, 3), dtype=bool)
+E = not (vals := ARRAY_2D[:, :].all())
+F = not (vals := ARRAY_2D[2, 2].all())
+G = not (vals := ARRAY_2D[::].all())
+H = not (vals := ARRAY_2D[2:2].all())
+
+# Test yield
+class TestYieldClass:
+    @classmethod
+    def function_A(cls):
+        yield (var := 1 + 1)
+        print(var)
+
+    @classmethod
+    def function_B(cls):
+        yield str(1 + 1)
+
+    @classmethod
+    def function_C(cls):
+        yield (1 + 1) # [superfluous-parens]
diff --git a/tests/functional/s/super/superfluous_parens_walrus_py38.rc b/tests/functional/s/super/superfluous_parens_walrus_py38.rc
@@ -0,0 +1,2 @@
+[testoptions]
+min_pyver=3.8
diff --git a/tests/functional/s/super/superfluous_parens_walrus_py38.txt b/tests/functional/s/super/superfluous_parens_walrus_py38.txt
@@ -0,0 +1,5 @@
+superfluous-parens:16:0::Unnecessary parens after 'not' keyword:HIGH
+superfluous-parens:19:0::Unnecessary parens after 'if' keyword:HIGH
+superfluous-parens:22:0::Unnecessary parens after 'not' keyword:HIGH
+superfluous-parens:25:0::Unnecessary parens after 'not' keyword:HIGH
+superfluous-parens:51:0::Unnecessary parens after 'yield' keyword:HIGH
diff --git a/tests/functional/s/superfluous_parens.py b/tests/functional/s/superfluous_parens.py
diff --git a/tests/functional/s/superfluous_parens.txt b/tests/functional/s/superfluous_parens.txt