Fix false positives for superfluous-parens

[DanielNoord]

To ease the process of reviewing your PR, do make sure to complete the following boxes.

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature, or an important bug fix, add a What's New entry in
  doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🐛 Bug fix

Description

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

#3249 reported a lot of different false positives, numerous of which also included code that is not valid python code.
I think this should fix all false positives within valid code, but before closing that issue somebody might need to run through the issue again to see if all reports have been taken care of.

[Pierre-Sassoulas]

Wow, so much issue closed in one MR :) Thank you for doing the project management works too and checking all the issue with the problem !

[Pierre-Sassoulas]

Could you add something in the changelog and in 2.10.0's whatsnew ? :)

[DanielNoord]

Not sure what is going on:
This run worked and I thought I added lines to the changelog. Next run the CI failed.. Going to (try and) fix!

[Pierre-Sassoulas]

This is strange, the tests aren't usually flaky. Maybe the first run was with the current master ?

[DanielNoord]

I guess so. However, when I copy the code of format.py to my local installation the following code works and produces correct output:

if not (my_var := False):
    print(my_var)

print([i ** 2 for i in (range(10) if 7 > 5 else range(3))])


def a_function():
    return (x for x in ((3, 4)))  # Bad at col 25


print([i ** 2 for i in ((3, 4))])  # Bad at col 25

if not ((odd := 1)):  # Bad at col 9
    pass

However, the following messages raise errors in the test of unittest_format.py:

( # Line 187
    Message("superfluous-parens", line=1, args="in"),
    "return (x for x in ((3, 4)))",
    0,
)

(Message("superfluous-parens", line=1, args="not"), "if not ((odd := 1)):", 0), # Line 215
 

I believe those messages are tested in the test file in the 4th and 5th test. Any idea why it does work when replacing the code in the locally installed pylint but not in the test suite?

[Pierre-Sassoulas]

I don't see any problem with the unittest but this is arder to udnerstand than functional tests. Could you try to create a functional tests in tests/functional/s/superfluous_parenthesis.py and see how it goes (=copy paste this code and signal expected message with # [message-name] ? You might have to create another file for warlus operator that only work in python 3.10 with a corresponding .rc file.

[Pierre-Sassoulas]

We should not remove valid use cases from the functional tests.

[DanielNoord]

I moved this comment to 416 because I believe it cover 417-418 and is misplaced in the current code.

[DanielNoord]

Force-pushed to clean up the commit history.

First commit covers splitting the functional tests for superfluous-parens into a base and python 3.8 version to test the walrus operator. As mentioned in the OP, #3249 covers many different reports of false positives, but I think the additions to the functional tests cover all of them. I've also added some tests for cases of (((my_var))) which were not discussed, but were initially failing.

Second commit covers changes to the code and to the unittest. The codes passes the functional tests but fails the unittests. I don't know why, so will need some guidance to fix this before this PR can be undrafted.

Once again, sorry for creating this PR before it was ready. I thought it was finished but apparently not. I'm going to fix my local development environment before doing any additional work so I have better test results before creating PR's. With 15+ tests always failing I often miss cases where 16 instead of 15 tests fail...

[Pierre-Sassoulas]

Once again, sorry for creating this PR before it was ready. I thought it was finished but apparently not.

Don't worry, the continuous integration is here for checking on every environment and python-interpreters and it's expected that fixing everything might take some time :)

[DanielNoord]

Do you have any idea why the unit tests are failing?
I have not used those before and just copied the pattern used on other places in the file, but my additions seem to fail. Testing similar patterns with the functional testing does seem to work...

[Pierre-Sassoulas]

I made some tests locally, I pushed the result. I don't know exactly why but self.checker._check_keyword_parentheses was not resulting from the same output than for functional tests. I changed to process_token and I think the test case that is failing is a real issue now.

[DanielNoord]

I think the unittest is failing on ( Message("superfluous-parens", line=1, args="in"), "return (x for x in ((3, 4)))", 0), right?
However, that case seems to be tested by the functional test on line 21 I think. I don't see what makes pylint respond differently to both cases.

[Pierre-Sassoulas]

I rarely use the unittest myself, if it's a pain to do let's convert to functional tests :)

[coveralls]

Coverage increased (+0.3%) to 92.578% when pulling ee29f72 on DanielNoord:superfluous-parens into e7b71cf on PyCQA:main.

[DanielNoord]

All tests are now passing. Please see if you think all cases are covered in the tests, especially those mentioned in #3249.

[Pierre-Sassoulas]

Thank you for fixing the tests ! I think once we incorporate use case from the pinned issue we can merge :)

[DanielNoord]

I have updated the tests, let me know what you think.

I have removed these two tests:

I = ("Version " + G) in F
assert "" + ("Version " + G) in F

The current checker focuses on unnecessary parens after keywords. These tests cover unnecessary parens with string combination. Because of

if tokens[start + 1].string != "(":
            return

on line 373 in format.py and the fact that we only run when Keywords are present we won't be able to catch all of these errors. For example the first test described, as it doesn't include a keyword. I think opening a new issue for "Superfluous parens checker for string combinations" would be the best way forward.

[Pierre-Sassoulas]

I think opening a new issue for "Superfluous parens checker for string combinations" would be the best way forward.

Sure thing, this MR already fix so much issue at once already ! What do you think of letting the test case in the functional test without the # [superfluous-parentheses] and with a TODO ? (Like what was done for 3429 before)

[DanielNoord]

Sounds good to me, but perhaps first create the issue so we can link to it in the TODO?

[Pierre-Sassoulas]

Amazing work, thank you ! 👏