Skip to content

[trailing-comma-tuple] Fix enabling with message control locally when disabled for the file #9609

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.

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .pyenchant_pylint_custom_dict.txt
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ testoptions
tmp
tokencheckers
tokeninfo
tokenization
tokenize
tokenizer
toml
Expand Down
2 changes: 0 additions & 2 deletions doc/data/messages/t/trailing-comma-tuple/details.rst

This file was deleted.

4 changes: 4 additions & 0 deletions doc/whatsnew/fragments/9608.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
``trailing-comma-tuple`` should now be correctly emitted when it was disabled globally
but enabled via local message control, after removal of an over-optimisation.

Refs #9608.
35 changes: 31 additions & 4 deletions pylint/checkers/refactoring/refactoring_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -652,9 +652,29 @@ def process_tokens(self, tokens: list[tokenize.TokenInfo]) -> None:
trailing_comma_tuple_enabled_for_file = self.linter.is_message_enabled(
"trailing-comma-tuple"
)
trailing_comma_tuple_enabled_once: bool = trailing_comma_tuple_enabled_for_file
# Process tokens and look for 'if' or 'elif'
for index, token in enumerate(tokens):
token_string = token[1]
if (
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about factoring this out into a helper method? It's almost 20 lines with comments.

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas May 18, 2024

Choose a reason for hiding this comment

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

I do think we need to make this kind of optimization where we do not compute things when a message is disabled easier to use because the faster computation is the one you never do.

If we create an helper then we'd probably want to parametrize "trailing-comma-tuple" / "R1707" so it can be useful elsewhere with other messages (then we should be using the message store to get old name of the message if a message was ever renamed ?). And it adds a call.

We have the "perfect function" for this already with self.linter.is_message_enabled( applied on a line (barring some optimization to do here related to not launching it on each token as they are multiple token per line). It felt very heavy handed in this case so I did something very specific without check for old name / and without exact check of enabling or not.

To be honest I made a lot of assumptions without ever doing a benchmark. One way to keep this clean and dry would be to make the message store better for this use case (token checks, and probably enabled but not absolutely sure it still is?).

Copy link
Member

Choose a reason for hiding this comment

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

No worries, we can merge as is. I wasn't suggesting reusing it elsewhere, I just meant hoisting it out of this current location into a few lines up in the same file.

not trailing_comma_tuple_enabled_once
and token_string.startswith("#")
# We have at least 1 '#' (one char) at the start of the token
and "pylint:" in token_string[1:]
# We have at least '#' 'pylint' ( + ':') (8 chars) at the start of the token
and "enable" in token_string[8:]
# We have at least '#', 'pylint', ( + ':'), 'enable' (+ '=') (15 chars) at
# the start of the token
and any(
c in token_string[15:] for c in ("trailing-comma-tuple", "R1707")
)
):
# Way to not have to check if "trailing-comma-tuple" is enabled or
# disabled on each line: Any enable for it during tokenization and
# we'll start using the costly '_is_trailing_comma' to check if we
# need to raise the message. We still won't raise if it's disabled
# again due to the usual generic message control handling later.
trailing_comma_tuple_enabled_once = True
if token_string == "elif":
# AST exists by the time process_tokens is called, so
# it's safe to assume tokens[index+1] exists.
Expand All @@ -663,10 +683,17 @@ def process_tokens(self, tokens: list[tokenize.TokenInfo]) -> None:
# token[2] is the actual position and also is
# reported by IronPython.
self._elifs.extend([token[2], tokens[index + 1][2]])
elif trailing_comma_tuple_enabled_for_file and _is_trailing_comma(
tokens, index
):
self.add_message("trailing-comma-tuple", line=token.start[0])
elif (
trailing_comma_tuple_enabled_for_file
or trailing_comma_tuple_enabled_once
) and _is_trailing_comma(tokens, index):
# If "trailing-comma-tuple" is enabled globally we always check _is_trailing_comma
# it might be for nothing if there's a local disable, or if the message control is
# not enabling 'trailing-comma-tuple', but the alternative is having to check if
# it's enabled for a line each line (just to avoid calling '_is_trailing_comma').
self.add_message(
"trailing-comma-tuple", line=token.start[0], confidence=HIGH
)

@utils.only_required_for_messages("consider-using-with")
def leave_module(self, _: nodes.Module) -> None:
Expand Down
12 changes: 7 additions & 5 deletions pylint/lint/message_state_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,14 @@ def is_message_enabled(
line: int | None = None,
confidence: interfaces.Confidence | None = None,
) -> bool:
"""Return whether this message is enabled for the current file, line and
confidence level.
"""Is this message enabled for the current file ?

This function can't be cached right now as the line is the line of
the currently analysed file (self.file_state), if it changes, then the
result for the same msg_descr/line might need to change.
Optionally, is it enabled for this line and confidence level ?

The current file is implicit and mandatory. As a result this function
can't be cached right now as the line is the line of the currently
analysed file (self.file_state), if it changes, then the result for
the same msg_descr/line might need to change.

:param msg_descr: Either the msgid or the symbol for a MessageDefinition
:param line: The line of the currently analysed file
Expand Down
10 changes: 10 additions & 0 deletions tests/functional/t/trailing_comma_tuple.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,13 @@ def some_other_func():

JJJ = some_func(0,
0)

# pylint: disable-next=trailing-comma-tuple
AAA = 1,
BBB = "aaaa", # [trailing-comma-tuple]
# pylint: disable=trailing-comma-tuple
CCC="aaa",
III = some_func(0,
0),
# pylint: enable=trailing-comma-tuple
FFF=['f'], # [trailing-comma-tuple]
20 changes: 11 additions & 9 deletions tests/functional/t/trailing_comma_tuple.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
trailing-comma-tuple:3:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:4:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:5:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:6:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:31:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:34:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:38:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:41:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:47:0:None:None::Disallow trailing comma tuple:UNDEFINED
trailing-comma-tuple:3:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:4:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:5:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:6:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:31:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:34:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:38:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:41:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:47:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:54:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:60:0:None:None::Disallow trailing comma tuple:HIGH
24 changes: 24 additions & 0 deletions tests/functional/t/trailing_comma_tuple_9608.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
"""Check trailing comma tuple optimization."""
# pylint: disable=missing-docstring

AAA = 1,
BBB = "aaaa",
CCC="aaa",
FFF=['f'],

def some_func(first, second):
if first:
return first,
if second:
return (first, second,)
return first, second,

#pylint:enable = trailing-comma-tuple
AAA = 1, # [trailing-comma-tuple]
BBB = "aaaa", # [trailing-comma-tuple]
# pylint: disable=trailing-comma-tuple
CCC="aaa",
III = some_func(0,
0),
# pylint: enable=trailing-comma-tuple
FFF=['f'], # [trailing-comma-tuple]
5 changes: 5 additions & 0 deletions tests/functional/t/trailing_comma_tuple_9608.rc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[MAIN]
disable=trailing-comma-tuple

[testoptions]
exclude_from_minimal_messages_config=True
Copy link
Member Author

Choose a reason for hiding this comment

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

Got very confused by that error at first (Didn't see it was a specific automated test that was failing). I chose the fast fix but maybe taking into account the conf file after disabling everything and checking what's expected in the functional test would be better than the current behavior.

3 changes: 3 additions & 0 deletions tests/functional/t/trailing_comma_tuple_9608.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
trailing-comma-tuple:17:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:18:0:None:None::Disallow trailing comma tuple:HIGH
trailing-comma-tuple:24:0:None:None::Disallow trailing comma tuple:HIGH
Loading