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.

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

Make disable-next only consider the succeeding line #7411

Merged
merged 3 commits into from Sep 5, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/7401.bugfix
@@ -0,0 +1,3 @@
``disable-next`` is now correctly scoped to only the succeeding line.

Closes #7401
3 changes: 1 addition & 2 deletions pylint/checkers/variables.py
Expand Up @@ -2209,9 +2209,8 @@ def _loopvar_name(self, node: astroid.Name) -> None:
# scope lookup rules would need to be changed to return the initial
# assignment (which does not exist in code per se) as well as any later
# modifications.
# pylint: disable-next=too-many-boolean-expressions
if (
not astmts
not astmts # pylint: disable=too-many-boolean-expressions
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this an acceptable change in behaviour? I think it is. This seems to be an unintended side effect of checking for block lines that we didn't consider previously.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make disable-next work on the whole if like before ? I think it's going to be reported as a regression, having to change disables when upgrading pylint does not feel great.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not if we also don't want to have the issue reported in the OP. I think it makes sense for disable-next only to work for the next line and I think this is a relatively minor change to have to fix in an auto-update. Obviously it is annoying but let's fix this now while the impact is still relatively minor (compared to a 10 year long supported behaviour).

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough !

or (
astmts[0].parent == astmts[0].root()
and astmts[0].parent.parent_of(node)
Expand Down
10 changes: 5 additions & 5 deletions pylint/lint/message_state_handler.py
Expand Up @@ -70,10 +70,10 @@ def _set_one_msg_status(
self, scope: str, msg: MessageDefinition, line: int | None, enable: bool
) -> None:
"""Set the status of an individual message."""
if scope == "module":
if scope in {"module", "line"}:
assert isinstance(line, int) # should always be int inside module scope

self.linter.file_state.set_msg_status(msg, line, enable)
self.linter.file_state.set_msg_status(msg, line, enable, scope)
if not enable and msg.symbol != "locally-disabled":
self.linter.add_message(
"locally-disabled", line=line, args=(msg.symbol, msg.msgid)
Expand Down Expand Up @@ -143,7 +143,7 @@ def _set_msg_status(
ignore_unknown: bool = False,
) -> None:
"""Do some tests and then iterate over message definitions to set state."""
assert scope in {"package", "module"}
assert scope in {"package", "module", "line"}

message_definitions = self._get_messages_to_set(msgid, enable, ignore_unknown)

Expand Down Expand Up @@ -197,7 +197,7 @@ def disable(
def disable_next(
self,
msgid: str,
scope: str = "package",
_: str = "package",
line: int | None = None,
ignore_unknown: bool = False,
) -> None:
Expand All @@ -207,7 +207,7 @@ def disable_next(
self._set_msg_status(
msgid,
enable=False,
scope=scope,
scope="line",
line=line + 1,
ignore_unknown=ignore_unknown,
)
Expand Down
50 changes: 35 additions & 15 deletions pylint/utils/file_state.py
Expand Up @@ -194,30 +194,50 @@ def _set_message_state_in_block(
state = lines[line]
original_lineno = line

# Update suppression mapping
if not state:
self._suppression_mapping[(msg.msgid, line)] = original_lineno
else:
self._suppression_mapping.pop((msg.msgid, line), None)
self._set_message_state_on_line(msg, line, state, original_lineno)

# Update message state for respective line
try:
self._module_msgs_state[msg.msgid][line] = state
except KeyError:
self._module_msgs_state[msg.msgid] = {line: state}
del lines[lineno]

def set_msg_status(self, msg: MessageDefinition, line: int, status: bool) -> None:
def _set_message_state_on_line(
self,
msg: MessageDefinition,
line: int,
state: bool,
original_lineno: int,
) -> None:
"""Set the state of a message on a line."""
# Update suppression mapping
if not state:
self._suppression_mapping[(msg.msgid, line)] = original_lineno
else:
self._suppression_mapping.pop((msg.msgid, line), None)

# Update message state for respective line
try:
self._module_msgs_state[msg.msgid][line] = state
except KeyError:
self._module_msgs_state[msg.msgid] = {line: state}

def set_msg_status(
self,
msg: MessageDefinition,
line: int,
status: bool,
scope: str = "package",
) -> None:
"""Set status (enabled/disable) for a given message at a given line."""
assert line > 0
assert self._module
# TODO: 3.0: Remove unnecessary assertion
assert self._msgs_store

# Expand the status to cover all relevant block lines
self._set_state_on_block_lines(
self._msgs_store, self._module, msg, {line: status}
)
if scope != "line":
# Expand the status to cover all relevant block lines
self._set_state_on_block_lines(
self._msgs_store, self._module, msg, {line: status}
)
else:
self._set_message_state_on_line(msg, line, status, line)

# Store the raw value
try:
Expand Down
7 changes: 7 additions & 0 deletions tests/functional/d/disable_msg_next_line.py
Expand Up @@ -18,3 +18,10 @@ def function_C():

def function_D(arg1, arg2): # [unused-argument, invalid-name]
return arg1


def function_E(): # [invalid-name]
# pylint: disable-next=unused-variable

Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
test = 43 # [unused-variable]
blah = 123 # [unused-variable]
3 changes: 3 additions & 0 deletions tests/functional/d/disable_msg_next_line.txt
Expand Up @@ -3,3 +3,6 @@ unused-variable:15:4:15:5:function_C:Unused variable 'x':UNDEFINED
f-string-without-interpolation:16:11:16:44:function_C:Using an f-string that does not have any interpolated variables:UNDEFINED
invalid-name:19:0:19:14:function_D:"Function name ""function_D"" doesn't conform to snake_case naming style":HIGH
unused-argument:19:21:19:25:function_D:Unused argument 'arg2':HIGH
invalid-name:23:0:23:14:function_E:"Function name ""function_E"" doesn't conform to snake_case naming style":HIGH
unused-variable:26:4:26:8:function_E:Unused variable 'test':UNDEFINED
unused-variable:27:4:27:8:function_E:Unused variable 'blah':UNDEFINED