Fix disabling of bad-option-value (#6556)

Co-authored-by: Pierre Sassoulas <[email protected]>

Author: DanielNoord

ChangeLog

@@ -9,6 +9,12 @@ Release date: TBA
 ..
   Put new features here and also in 'doc/whatsnew/2.14.rst'
 
+* We have improved our recognition of inline disable and enable comments. It is
+  now possible to disable ``bad-option-value`` inline  (as long as you disable it before
+  the bad option value is raised, i.e. ``disable=bad-option-value,bad-message`` not ``disable=bad-message,bad-option-value`` ) as well as certain other previously unsupported messages.
+
+  Closes #3312
+
 * Added new checker ``comparison-of-constants``.
 
   Closes #6076

doc/whatsnew/2.14.rst

@@ -129,6 +129,13 @@ Other Changes
 
   Closes #4301
 
+* We have improved our recognition of inline disable and enable comments. It is
+  now possible to disable ``bad-option-value`` inline (as long as you disable it before
+  the bad option value is raised, i.e. ``disable=bad-option-value,bad-message`` not ``disable=bad-message,bad-option-value`` ) as well as certain other
+  previously unsupported messages.
+
+  Closes #3312
+
 * Update ``invalid-slots-object`` message to show bad object rather than its inferred value.
 
   Closes #6101

pylint/lint/pylinter.py

@@ -937,8 +937,6 @@ def _check_astroid_module(
             self.process_tokens(tokens)
             if self._ignore_file:
                 return False
-            # walk ast to collect line numbers
-            self.file_state.collect_block_lines(self.msgs_store, node)
             # run raw and tokens checkers
             for raw_checker in rawcheckers:
                 raw_checker.process_module(node)

pylint/utils/file_state.py

@@ -74,25 +74,32 @@ def collect_block_lines(
         self, msgs_store: MessageDefinitionStore, module_node: nodes.Module
     ) -> None:
         """Walk the AST to collect block level options line numbers."""
+        warnings.warn(
+            "'collect_block_lines' has been deprecated and will be removed in pylint 3.0.",
+            DeprecationWarning,
+        )
         for msg, lines in self._module_msgs_state.items():
             self._raw_module_msgs_state[msg] = lines.copy()
         orig_state = self._module_msgs_state.copy()
         self._module_msgs_state = {}
         self._suppression_mapping = {}
         self._effective_max_line_number = module_node.tolineno
-        self._collect_block_lines(msgs_store, module_node, orig_state)
+        for msgid, lines in orig_state.items():
+            for msgdef in msgs_store.get_message_definitions(msgid):
+                self._set_state_on_block_lines(msgs_store, module_node, msgdef, lines)
 
-    def _collect_block_lines(
+    def _set_state_on_block_lines(
         self,
         msgs_store: MessageDefinitionStore,
         node: nodes.NodeNG,
-        msg_state: MessageStateDict,
+        msg: MessageDefinition,
+        msg_state: dict[int, bool],
     ) -> None:
         """Recursively walk (depth first) AST to collect block level options
-        line numbers.
+        line numbers and set the state correctly.
         """
         for child in node.get_children():
-            self._collect_block_lines(msgs_store, child, msg_state)
+            self._set_state_on_block_lines(msgs_store, child, msg, msg_state)
         # first child line number used to distinguish between disable
         # which are the first child of scoped node with those defined later.
         # For instance in the code below:
@@ -115,9 +122,7 @@ def _collect_block_lines(
             firstchildlineno = node.body[0].fromlineno
         else:
             firstchildlineno = node.tolineno
-        for msgid, lines in msg_state.items():
-            for msg in msgs_store.get_message_definitions(msgid):
-                self._set_message_state_in_block(msg, lines, node, firstchildlineno)
+        self._set_message_state_in_block(msg, msg_state, node, firstchildlineno)
 
     def _set_message_state_in_block(
         self,
@@ -139,18 +144,61 @@ def _set_message_state_in_block(
                 if lineno > firstchildlineno:
                     state = True
                 first_, last_ = node.block_range(lineno)
+                # pylint: disable=useless-suppression
+                # For block nodes first_ is their definition line. For example, we
+                # set the state of line zero for a module to allow disabling
+                # invalid-name for the module. For example:
+                # 1. # pylint: disable=invalid-name
+                # 2. ...
+                # OR
+                # 1. """Module docstring"""
+                # 2. # pylint: disable=invalid-name
+                # 3. ...
+                #
+                # But if we already visited line 0 we don't need to set its state again
+                # 1. # pylint: disable=invalid-name
+                # 2. # pylint: enable=invalid-name
+                # 3. ...
+                # The state should come from line 1, not from line 2
+                # Therefore, if the 'fromlineno' is already in the states we just start
+                # with the lineno we were originally visiting.
+                # pylint: enable=useless-suppression
+                if (
+                    first_ == node.fromlineno
+                    and first_ >= firstchildlineno
+                    and node.fromlineno in self._module_msgs_state.get(msg.msgid, ())
+                ):
+                    first_ = lineno
+
             else:
                 first_ = lineno
                 last_ = last
             for line in range(first_, last_ + 1):
-                # do not override existing entries
-                if line in self._module_msgs_state.get(msg.msgid, ()):
+                # Do not override existing entries. This is especially important
+                # when parsing the states for a scoped node where some line-disables
+                # have already been parsed.
+                if (
+                    (
+                        isinstance(node, nodes.Module)
+                        and node.fromlineno <= line < lineno
+                    )
+                    or (
+                        not isinstance(node, nodes.Module)
+                        and node.fromlineno < line < lineno
+                    )
+                ) and line in self._module_msgs_state.get(msg.msgid, ()):
                     continue
                 if line in lines:  # state change in the same 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)
+
+                # Update message state for respective line
                 try:
                     self._module_msgs_state[msg.msgid][line] = state
                 except KeyError:
@@ -160,10 +208,20 @@ def _set_message_state_in_block(
     def set_msg_status(self, msg: MessageDefinition, line: int, status: bool) -> 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}
+        )
+
+        # Store the raw value
         try:
-            self._module_msgs_state[msg.msgid][line] = status
+            self._raw_module_msgs_state[msg.msgid][line] = status
         except KeyError:
-            self._module_msgs_state[msg.msgid] = {line: status}
+            self._raw_module_msgs_state[msg.msgid] = {line: status}
 
     def handle_ignored_message(
         self, state_scope: Literal[0, 1, 2] | None, msgid: str, line: int | None

tests/functional/b/bad_option_value_disable.py

@@ -0,0 +1,14 @@
+"""Tests for the disabling of bad-option-value."""
+# pylint: disable=invalid-name
+
+# pylint: disable=bad-option-value
+
+var = 1  # pylint: disable=a-removed-option
+
+# pylint: enable=bad-option-value
+
+var = 1  # pylint: disable=a-removed-option # [bad-option-value]
+
+# bad-option-value needs to be disabled before the bad option
+var = 1  # pylint: disable=a-removed-option, bad-option-value # [bad-option-value]
+var = 1  # pylint: disable=bad-option-value, a-removed-option

tests/functional/b/bad_option_value_disable.txt

@@ -0,0 +1,2 @@
+bad-option-value:10:0:None:None::Bad option value for disable. Don't recognize message a-removed-option.:UNDEFINED
+bad-option-value:13:0:None:None::Bad option value for disable. Don't recognize message a-removed-option.:UNDEFINED

tests/lint/unittest_lint.py

@@ -191,8 +191,14 @@ def reporter():
 @pytest.fixture
 def initialized_linter(linter: PyLinter) -> PyLinter:
     linter.open()
-    linter.set_current_module("toto", "mydir/toto")
-    linter.file_state = FileState("toto", linter.msgs_store)
+    linter.set_current_module("long_test_file", "long_test_file")
+    linter.file_state = FileState(
+        "long_test_file",
+        linter.msgs_store,
+        linter.get_ast(
+            str(join(REGRTEST_DATA_DIR, "long_test_file.py")), "long_test_file"
+        ),
+    )
     return linter
 
 
@@ -272,9 +278,9 @@ def test_enable_message_block(initialized_linter: PyLinter) -> None:
     filepath = join(REGRTEST_DATA_DIR, "func_block_disable_msg.py")
     linter.set_current_module("func_block_disable_msg")
     astroid = linter.get_ast(filepath, "func_block_disable_msg")
+    linter.file_state = FileState("func_block_disable_msg", linter.msgs_store, astroid)
     linter.process_tokens(tokenize_module(astroid))
     fs = linter.file_state
-    fs.collect_block_lines(linter.msgs_store, astroid)
     # global (module level)
     assert linter.is_message_enabled("W0613")
     assert linter.is_message_enabled("E1101")
@@ -311,7 +317,6 @@ def test_enable_message_block(initialized_linter: PyLinter) -> None:
     assert linter.is_message_enabled("E1101", 75)
     assert linter.is_message_enabled("E1101", 77)
 
-    fs = linter.file_state
     assert fs._suppression_mapping["W0613", 18] == 17
     assert fs._suppression_mapping["E1101", 33] == 30
     assert ("E1101", 46) not in fs._suppression_mapping

tests/regrtest_data/long_test_file.py

@@ -0,0 +1,100 @@
+"""
+This file is used for bad-option-value's test. We needed a module that isn’t restricted by line numbers
+as the tests use various line numbers to test the behaviour.
+
+Using an empty module creates issue as you can’t disable something on line 10 if it doesn’t exist. Thus, we created an extra long file so we never run into an issue with that.
+"""
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+print(1)

tests/test_deprecation.py

@@ -9,6 +9,7 @@
 from typing import Any
 
 import pytest
+from astroid import nodes
 
 from pylint.checkers import BaseChecker
 from pylint.checkers.mapreduce_checker import MapReduceMixin
@@ -100,3 +101,10 @@ def test_filestate() -> None:
     with pytest.warns(DeprecationWarning):
         FileState(msg_store=MessageDefinitionStore())
     FileState("foo", MessageDefinitionStore())
+
+
+def test_collectblocklines() -> None:
+    """Test FileState.collect_block_lines."""
+    state = FileState("foo", MessageDefinitionStore())
+    with pytest.warns(DeprecationWarning):
+        state.collect_block_lines(MessageDefinitionStore(), nodes.Module("foo"))