From 50f7b16486baec52761bb9c173084f11c067a71e Mon Sep 17 00:00:00 2001 From: Bryce Guinta Date: Tue, 16 Jun 2020 09:25:25 -0400 Subject: [PATCH 1/3] Improve readability of FormatChecker._check_keyword_parentheses --- pylint/checkers/format.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/pylint/checkers/format.py b/pylint/checkers/format.py index ae7cf4260e..24618923c4 100644 --- a/pylint/checkers/format.py +++ b/pylint/checkers/format.py @@ -53,6 +53,7 @@ import tokenize from functools import reduce # pylint: disable=redefined-builtin from typing import List +from tokenize import TokenInfo from astroid import nodes @@ -353,7 +354,7 @@ def new_line(self, tokens, line_end, line_start): def process_module(self, _module): self._keywords_with_parens = set() - def _check_keyword_parentheses(self, tokens, start): + def _check_keyword_parentheses(self, tokens: List[TokenInfo], start: int) -> None: """Check that there are not unnecessary parens after a keyword. Parens are unnecessary if there is exactly one balanced outer pair on a @@ -365,29 +366,29 @@ def _check_keyword_parentheses(self, tokens, start): start: int; the position of the keyword in the token list. """ # If the next token is not a paren, we're fine. - if self._bracket_stack[-1] == ":" and tokens[start][1] == "for": + if self._bracket_stack[-1] == ":" and tokens[start].string == "for": self._bracket_stack.pop() - if tokens[start + 1][1] != "(": + if tokens[start + 1].string != "(": return found_and_or = False depth = 0 - keyword_token = str(tokens[start][1]) - line_num = tokens[start][2][0] + keyword_token = str(tokens[start].string) + line_num = tokens[start].start[0] for i in range(start, len(tokens) - 1): token = tokens[i] # If we hit a newline, then assume any parens were for continuation. - if token[0] == tokenize.NL: + if token.type == tokenize.NL: return - if token[1] == "(": + if token.string == "(": depth += 1 - elif token[1] == ")": + elif token.string == ")": depth -= 1 if depth: continue # ')' can't happen after if (foo), since it would be a syntax error. - if tokens[i + 1][1] in (":", ")", "]", "}", "in") or tokens[i + 1][ - 0 - ] in (tokenize.NEWLINE, tokenize.ENDMARKER, tokenize.COMMENT): + 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 i == start + 2: return From ba0c794bbcf611af3f0702fce17358180b077a2b Mon Sep 17 00:00:00 2001 From: Bryce Guinta Date: Tue, 16 Jun 2020 10:29:33 -0400 Subject: [PATCH 2/3] Remove unused member variable of FormatChecker --- pylint/checkers/format.py | 11 +++++------ tests/checkers/unittest_format.py | 4 ---- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/pylint/checkers/format.py b/pylint/checkers/format.py index 24618923c4..130ddd0c5f 100644 --- a/pylint/checkers/format.py +++ b/pylint/checkers/format.py @@ -352,7 +352,7 @@ def new_line(self, tokens, line_end, line_start): self.check_lines(line, line_num) def process_module(self, _module): - self._keywords_with_parens = set() + pass def _check_keyword_parentheses(self, tokens: List[TokenInfo], start: int) -> None: """Check that there are not unnecessary parens after a keyword. @@ -401,11 +401,10 @@ def _check_keyword_parentheses(self, tokens: List[TokenInfo], start: int) -> Non self.add_message( "superfluous-parens", line=line_num, args=keyword_token ) - elif keyword_token not in self._keywords_with_parens: - if not found_and_or: - self.add_message( - "superfluous-parens", line=line_num, args=keyword_token - ) + elif not found_and_or: + self.add_message( + "superfluous-parens", line=line_num, args=keyword_token + ) return elif depth == 1: # This is a tuple, which is always acceptable. diff --git a/tests/checkers/unittest_format.py b/tests/checkers/unittest_format.py index 75b0c79674..7c07ac9ed7 100644 --- a/tests/checkers/unittest_format.py +++ b/tests/checkers/unittest_format.py @@ -137,7 +137,6 @@ class TestSuperfluousParentheses(CheckerTestCase): CHECKER_CLASS = FormatChecker def testCheckKeywordParensHandlesValidCases(self): - self.checker._keywords_with_parens = set() cases = [ "if foo:", "if foo():", @@ -157,7 +156,6 @@ def testCheckKeywordParensHandlesValidCases(self): self.checker._check_keyword_parentheses(_tokenize_str(code), 0) def testCheckKeywordParensHandlesUnnecessaryParens(self): - self.checker._keywords_with_parens = set() cases = [ (Message("superfluous-parens", line=1, args="if"), "if (foo):", 0), (Message("superfluous-parens", line=1, args="if"), "if ((foo, bar)):", 0), @@ -187,7 +185,6 @@ def testCheckKeywordParensHandlesUnnecessaryParens(self): self.checker._check_keyword_parentheses(_tokenize_str(code), offset) def testCheckIfArgsAreNotUnicode(self): - self.checker._keywords_with_parens = set() cases = [("if (foo):", 0), ("assert (1 == 1)", 0)] for code, offset in cases: @@ -205,7 +202,6 @@ def testFuturePrintStatementWithoutParensWarning(self): self.checker.process_tokens(_tokenize_str(code)) def testKeywordParensFalsePositive(self): - self.checker._keywords_with_parens = set() code = "if 'bar' in (DICT or {}):" with self.assertNoMessages(): self.checker._check_keyword_parentheses(_tokenize_str(code), start=2) From 06daa924e390c1b2b3b0a8c6879f71236192dba2 Mon Sep 17 00:00:00 2001 From: Bryce Guinta Date: Tue, 16 Jun 2020 09:47:21 -0400 Subject: [PATCH 3/3] Fix false positive superfluous parens for walrus operator Close #3383 --- ChangeLog | 4 ++++ doc/whatsnew/2.6.rst | 2 ++ pylint/checkers/format.py | 23 +++++++++++++++++++---- tests/checkers/unittest_format.py | 21 +++++++++++++++++++++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2dc3aba713..33128066dd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -26,6 +26,10 @@ Release date: TBA * Add an faq detailing which messages to disable to avoid duplicates w/ other popular linters +* Fix superfluous-parens false-positive for the walrus operator + + Close #3383 + * Fix a bug with `ignore-docstrings` ignoring all lines in a module * Fix `pre-commit` config that could lead to undetected duplicate lines of code diff --git a/doc/whatsnew/2.6.rst b/doc/whatsnew/2.6.rst index 5ab2028c16..dc659d6562 100644 --- a/doc/whatsnew/2.6.rst +++ b/doc/whatsnew/2.6.rst @@ -23,3 +23,5 @@ Other Changes * The `no-space-check` option has been removed, it's no longer possible to consider empty line like a `trailing-whitespace` by using clever options. * `mixed-indentation` has been removed, it is no longer useful since TabError is included directly in python3 + +* Fix superfluous-parens false-positive for the walrus operator diff --git a/pylint/checkers/format.py b/pylint/checkers/format.py index 130ddd0c5f..52d852a258 100644 --- a/pylint/checkers/format.py +++ b/pylint/checkers/format.py @@ -52,8 +52,8 @@ import tokenize from functools import reduce # pylint: disable=redefined-builtin -from typing import List from tokenize import TokenInfo +from typing import List from astroid import nodes @@ -371,14 +371,25 @@ def _check_keyword_parentheses(self, tokens: List[TokenInfo], start: int) -> Non if tokens[start + 1].string != "(": return found_and_or = False + contains_walrus_operator = False + walrus_operator_depth = 0 depth = 0 keyword_token = str(tokens[start].string) line_num = tokens[start].start[0] for i in range(start, len(tokens) - 1): token = tokens[i] + # If we hit a newline, then assume any parens were for continuation. if token.type == tokenize.NL: return + # Since the walrus operator doesn't exist below python3.8, the tokenizer + # generates independent tokens + if ( + token.string == ":=" # <-- python3.8+ path + or token.string + tokens[i + 1].string == ":=" + ): + contains_walrus_operator = True + walrus_operator_depth = depth if token.string == "(": depth += 1 elif token.string == ")": @@ -386,10 +397,14 @@ def _check_keyword_parentheses(self, tokens: List[TokenInfo], start: int) -> Non if depth: 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)): + 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 if i == start + 2: return if keyword_token == "not": diff --git a/tests/checkers/unittest_format.py b/tests/checkers/unittest_format.py index 7c07ac9ed7..562649eb93 100644 --- a/tests/checkers/unittest_format.py +++ b/tests/checkers/unittest_format.py @@ -184,6 +184,27 @@ def testCheckKeywordParensHandlesUnnecessaryParens(self): with self.assertAddsMessages(msg): self.checker._check_keyword_parentheses(_tokenize_str(code), offset) + 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) + + 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) + def testCheckIfArgsAreNotUnicode(self): cases = [("if (foo):", 0), ("assert (1 == 1)", 0)]