Avoid reporting useless-suppression on wrong-import-position (#5219)

[timmartin]

Type of Changes

	Type
✓	🐛 Bug fix

Description

With a suppressed wrong-import-position warning and useless-suppressions enabled, Pylint was issuing a useless-suppression warning even though the suppression was not useless. This was happening because of a change here that fixed #1366; It was skipping the entire calculation of the checker when the warning was disabled, which prevents it from recording that the checker would have been hit. There's a similar fix here for the same case with wrong-import-order.

Closes #5219

[coveralls]

Pull Request Test Coverage Report for Build 2259016368

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.0006%) to 95.167%

---

Totals
Change from base Build 2259008088:	-0.0006%
Covered Lines:	15830
Relevant Lines:	16634

---

💛 - Coveralls

[jacobtylerwalls]

Thanks for the PR. What do you think about this diff? A bit smaller. EDIT: never mind, looks like it fails a test.

diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py
index d90173c0..4fa01e02 100644
--- a/pylint/checkers/imports.py
+++ b/pylint/checkers/imports.py
@@ -503,6 +503,7 @@ class ImportsChecker(DeprecatedMixin, BaseChecker):
         self._imports_stack = []
         self._first_non_import_node = None
 
+    @check_messages(*MSGS)
     def compute_first_non_import_node(self, node):
         # if the node does not contain an import instruction, and if it is the
         # first node of the module, keep a track of it (all the import positions
@@ -543,6 +544,7 @@ class ImportsChecker(DeprecatedMixin, BaseChecker):
         visit_ifexp
     ) = visit_comprehension = visit_expr = visit_if = compute_first_non_import_node
 
+    @check_messages(*MSGS)
     def visit_functiondef(self, node: nodes.FunctionDef) -> None:
         # If it is the first non import instruction of the module, record it.
         if self._first_non_import_node:
@@ -594,16 +596,7 @@ class ImportsChecker(DeprecatedMixin, BaseChecker):
         # if a first non-import instruction has already been encountered,
         # it means the import comes after it and therefore is not well placed
         if self._first_non_import_node:
-            if self.linter.is_message_enabled(
-                "wrong-import-position", self._first_non_import_node.fromlineno
-            ):
-                self.add_message(
-                    "wrong-import-position", node=node, args=node.as_string()
-                )
-            else:
-                self.linter.add_ignored_message(
-                    "wrong-import-position", node.fromlineno, node
-                )
+            self.add_message("wrong-import-position", node=node, args=node.as_string())
 
     def _record_import(self, node, importedmodnode):
         """Record the package `node` imports from."""

[Pierre-Sassoulas]

@check_messages(*MSGS) has the same effect than no decorator at all so I think we could also remove this, see #6353. What I wonder is what is the purpose of self.linter.add_ignored_message and self.linter.is_message_enabled in general.

[jacobtylerwalls]

By the way, the test that failed with my smaller diff was the test in #1337. (Yes, 1337! 🙄 )

So I'm good with this approach, meaning we can still support that somewhat special case.

[DanielNoord]

By the way, the test that failed with my smaller diff was the test in #1337. (Yes, 1337! 🙄 )

So I'm good with this approach, meaning we can still support that somewhat special case.

Do we understand why that test fails? It seems like it should work right?

[jacobtylerwalls]

Do we understand why that test fails? It seems like it should work right?

My understanding is that it's a really special case to extend the disable functionality. wrong-import-position comes in pairs (it's always wrong on two lines). So these methods have special functionality to make it so you can just disable in-line once. Do I understand exactly how it works? Nope.

[timmartin]

I don't have much to add, except to say that I was trying to change the logic of the check as little as possible while avoiding the useless-suppression warning. It seemed plausible that these checks would have to work slightly differently because they are about a relation between two lines and not an error on a single line. But I didn't dig any deeper than that.

If anyone thinks this needs a different approach I can take another run at it.