Fix false positive superfluous-parens for tuples
#4948
Conversation
| 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)))] | ||
| E = [x for x in ((3, 4) if 1 > 0 else ((((5, 6)))))] |
There was a problem hiding this comment.
Could we also add the example from the issue ?
There was a problem hiding this comment.
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!
Pierre-Sassoulas
added
Bug 🪲
False Positive 🦟
Tuples can be created with inner tuples. This creates double parenthesis which we flagged incorrectly. This closes pylint-dev#4907
DanielNoord
force-pushed
the
superfluous-parens-fix
branch
from
3e148b5 to
e60cd4b
Compare
| @@ -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), | |||
| ( | |||
There was a problem hiding this comment.
Turns out the unittests also had a false positive.
Pull Request Test Coverage Report for Build 1189234537
💛 - Coveralls |
| 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)))] |
There was a problem hiding this comment.
Why are the parens around (5, 6) not considered superfluous here?
There was a problem hiding this comment.
See the report in #4907
There is no good way to determine whether the parenthesises are there by accident or if they are intentional.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
doc/whatsnew/<current release.rst>.Type of Changes
Description
Tuples can be created with inner tuples. This creates double parenthesis which we flagged incorrectly.
This closes #4907