Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix false positive superfluous-parens for tuples #4948

Merged
merged 1 commit into from
Sep 1, 2021
Merged

Fix false positive superfluous-parens for tuples #4948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ Release date: TBA

Closes #4945

* Fix false positive ``superfluous-parens`` for tuples created with inner tuples

Closes #4907


What's New in Pylint 2.10.3?
============================
Expand Down
4 changes: 4 additions & 0 deletions doc/whatsnew/2.11.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,7 @@ Other Changes
and ``pathlib.Path().write_text()``

Closes #4945

* Fix false positive ``superfluous-parens`` for tuples created with inner tuples

Closes #4907
8 changes: 4 additions & 4 deletions pylint/checkers/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,11 +401,11 @@ def _check_keyword_parentheses(
depth -= 1
if depth:
if contains_double_parens and tokens[i + 1].string == ")":
self.add_message(
"superfluous-parens", line=line_num, args=keyword_token
)
# For walrus operators in `if (not)` conditions and comprehensions
if keyword_token in {"in", "if", "not"}:
continue
return
contains_double_parens = 0
contains_double_parens -= 1
continue
# ')' can't happen after if (foo), since it would be a syntax error.
if tokens[i + 1].string in (":", ")", "]", "}", "in") or tokens[
Expand Down
5 changes: 0 additions & 5 deletions tests/checkers/unittest_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,6 @@ def testCheckKeywordParensHandlesUnnecessaryParens(self):
(Message("superfluous-parens", line=1, args="if"), "if (foo):", 0),
(Message("superfluous-parens", line=1, args="if"), "if ((foo, bar)):", 0),
(Message("superfluous-parens", line=1, args="if"), "if (foo(bar)):", 0),
(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the unittests also had a false positive.

Message("superfluous-parens", line=1, args="return"),
"return ((x for x in x))",
0,
),
(Message("superfluous-parens", line=1, args="not"), "not (foo)", 0),
(Message("superfluous-parens", line=1, args="not"), "if not (foo):", 1),
(Message("superfluous-parens", line=1, args="if"), "if (not (foo)):", 0),
Expand Down
33 changes: 25 additions & 8 deletions tests/functional/s/super/superfluous_parens.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Test the superfluous-parens warning."""
from __future__ import print_function
# pylint: disable=unneeded-not, unnecessary-comprehension, missing-function-docstring, invalid-name, fixme
# pylint: disable=import-error, missing-class-docstring, too-few-public-methods
import numpy as np
A = 3
if (A == 5): # [superfluous-parens]
pass
Expand All @@ -18,10 +19,10 @@
del(DICT['b']) # [superfluous-parens]
del DICT['a']

B = [x for x in ((3, 4))] # [superfluous-parens]
B = [x for x in ((3, 4))]
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]
D = [x for x in ((3, 4) if 1 > 0 else ((5, 6)))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the parens around (5, 6) not considered superfluous here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the report in #4907

There is no good way to determine whether the parenthesises are there by accident or if they are intentional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies if I'm missing something obvious, but I think it should be possible if we re-implement the checker to consume the AST instead of tokens. At the AST level we would immediately know that () is an empty tuple and thus does require parentheses.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already consume the AST so that's not really the issue here. It might be possible to fix this, but in m'n initial attempt I created a lot of false positives so we reverted to not doing this kind of check on nested tuples in comprehension.

I welcome any PR that tries to fix this though!

E = [x for x in ((3, 4) if 1 > 0 else ((((5, 6)))))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add the example from the issue ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I was to quick to push this. I hoped I could leave some checks for double parenthesis in, but seems like this only creates problems. I'm going to remove them.

Give me a minute!


# Test assertions
F = "Version 1.0"
Expand All @@ -34,14 +35,30 @@
NUMS_LIST = ['1', '2', '3']
NUMS_SET = {'1', '2', '3'}
NUMS_DICT = {'1': 1, '2': 2, '3': 3}
I = tuple(x for x in ((a, str(a)) for a in ()))

# Test functions
def function_A():
return (x for x in ((3, 4))) # [superfluous-parens]
return (x for x in ((3, 4)))

# 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
J = "TestString"
K = ("Test " + "String")
L = ("Test " + "String") in I
assert "" + ("Version " + "String") in I

# Test numpy
def function_B(var_1: int, var_2: int) -> np.ndarray:
result = (((var_1 & var_2)) > 0)
return result.astype(np.float32)

def function_C(var_1: int, var_2: int) -> np.ndarray:
return (((var_1 & var_2)) > 0).astype(np.float32)

# Test Class
class ClassA:
keys = []

def __iter__(self):
return ((k, getattr(self, k)) for k in self.keys)
16 changes: 6 additions & 10 deletions tests/functional/s/super/superfluous_parens.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
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
superfluous-parens:6:0::Unnecessary parens after 'if' keyword:HIGH
superfluous-parens:8:0::Unnecessary parens after 'not' keyword:HIGH
superfluous-parens:12:0::Unnecessary parens after 'for' keyword:HIGH
superfluous-parens:14:0::Unnecessary parens after 'if' keyword:HIGH
superfluous-parens:19:0::Unnecessary parens after 'del' keyword:HIGH
superfluous-parens:31:0::Unnecessary parens after 'assert' keyword:HIGH