Skip to content

fix: support block-level wrong-import-position pragma suppression #10590

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.

Sign up for GitHub

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

Open
emmanuel-ferdman wants to merge 6 commits into
base: main
Choose a base branch
from
+65 −29
Open

fix: support block-level wrong-import-position pragma suppression #10590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8cace41
fix: scope wrong-import-position pragma to specific line only
emmanuel-ferdman Sep 25, 2025
a7e937a
fix: scope wrong-import-position pragma to specific line only
emmanuel-ferdman Sep 25, 2025
f957d35
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 25, 2025
1015d06
Support block-level wrong-import-position pragma suppression
emmanuel-ferdman Oct 19, 2025
4c56438
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 19, 2025
7fb7b18
fix: support block-level wrong-import-position pragma suppression
emmanuel-ferdman Oct 19, 2025
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
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/10589.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Allow ``wrong-import-position`` pragma on non-import lines to suppress following imports until the next non-import statement.

Closes #10589
61 changes: 36 additions & 25 deletions pylint/checkers/imports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ def __init__(self, linter: PyLinter) -> None:
BaseChecker.__init__(self, linter)
self.import_graph: defaultdict[str, set[str]] = defaultdict(set)
self._imports_stack: list[tuple[ImportNode, str]] = []
self._first_non_import_node = None
self._non_import_nodes: list[nodes.NodeNG] = []
self._module_pkg: dict[Any, Any] = (
{}
) # mapping of modules to the pkg they belong in
Expand Down Expand Up @@ -607,7 +607,7 @@ def leave_module(self, node: nodes.Module) -> None:
met.add(package)

self._imports_stack = []
self._first_non_import_node = None
self._non_import_nodes = []

def compute_first_non_import_node(
self,
Expand All @@ -621,12 +621,7 @@ def compute_first_non_import_node(
| nodes.Try
),
) -> None:
# 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
# of the module will be compared to the position of this first
# instruction)
if self._first_non_import_node:
return
# Track non-import nodes at module level to check import positions
if not isinstance(node.parent, nodes.Module):
return
if isinstance(node, nodes.Try) and any(
Expand All @@ -644,7 +639,8 @@ def compute_first_non_import_node(
]
if all(valid_targets):
return
self._first_non_import_node = node

self._non_import_nodes.append(node)

visit_try = visit_assignattr = visit_assign = visit_ifexp = visit_comprehension = (
visit_expr
Expand All @@ -653,12 +649,7 @@ def compute_first_non_import_node(
def visit_functiondef(
self, node: nodes.FunctionDef | nodes.While | nodes.For | nodes.ClassDef
) -> None:
# If it is the first non import instruction of the module, record it.
if self._first_non_import_node:
return

# Check if the node belongs to an `If` or a `Try` block. If they
# contain imports, skip recording this node.
# Record non-import instruction unless inside an If/Try block that contains imports
if not isinstance(node.parent.scope(), nodes.Module):
return

Expand All @@ -670,7 +661,7 @@ def visit_functiondef(
if any(root.nodes_of_class((nodes.Import, nodes.ImportFrom))):
return

self._first_non_import_node = node
self._non_import_nodes.append(node)

visit_classdef = visit_for = visit_while = visit_functiondef

Expand Down Expand Up @@ -699,19 +690,39 @@ def _check_position(self, node: ImportNode) -> None:

Send a message if `node` comes before another instruction
"""
# 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
# Check if import comes after a non-import statement
if self._non_import_nodes:
# Check for inline pragma on the import line
if not self.linter.is_message_enabled(
"wrong-import-position", 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
)
return

# Check for pragma on the preceding non-import statement
most_recent_non_import = None
for non_import_node in self._non_import_nodes:
if non_import_node.fromlineno < node.fromlineno:
most_recent_non_import = non_import_node
else:
break

if most_recent_non_import:
check_line = most_recent_non_import.fromlineno
if not self.linter.is_message_enabled(
"wrong-import-position", check_line
):
self.linter.add_ignored_message(
"wrong-import-position", check_line, most_recent_non_import
)
self.linter.add_ignored_message(
"wrong-import-position", node.fromlineno, node
)
return

self.add_message("wrong-import-position", node=node, args=node.as_string())

def _record_import(
self,
Expand Down
12 changes: 8 additions & 4 deletions tests/functional/d/disable_wrong_import_position.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
"""Checks that disabling 'wrong-import-position' on a statement prevents it from
invalidating subsequent imports."""
"""Test wrong-import-position pragma on non-import statement."""
# pylint: disable=unused-import

CONSTANT = True # pylint: disable=wrong-import-position

import os
import sys

CONSTANT_A = False # pylint: disable=wrong-import-position
import time

CONSTANT_B = True
import logging # [wrong-import-position]
1 change: 1 addition & 0 deletions tests/functional/d/disable_wrong_import_position.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
wrong-import-position:11:0:11:14::"Import ""import logging"" should be placed at the top of the module":UNDEFINED
16 changes: 16 additions & 0 deletions tests/functional/w/wrong_import_position_pragma_scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
"""Test wrong-import-position pragma scoping."""
# pylint: disable=unused-import

import os
import sys

# Pragma on non-import suppresses following imports until next non-import
CONSTANT_A = False # pylint: disable=wrong-import-position
import time
Copy link
Member

@jacobtylerwalls jacobtylerwalls Nov 30, 2025

Choose a reason for hiding this comment

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

Can we add a test case where this import is indented under a try or an if? I'm showing that this is not currently handled.

Copy link
Contributor Author

@emmanuel-ferdman emmanuel-ferdman Dec 3, 2025

Choose a reason for hiding this comment

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

@jacobtylerwalls You are correct - there is inconsistent behavior and the question is what is the expected behaviour?

For the following example:

import os
import sys

CONSTANT = True  # pylint: disable=wrong-import-position

try:  # or if/for/while/with/def/class
    import json
...

import time  # <-- Is this flagged?

The currently PR code produce the following behaviour:

Structure import time flagged?
try NO
if YES
for YES
while YES
with NO
def YES
class YES

Could you please help here define the expected behavior here?

Copy link
Member

@jacobtylerwalls jacobtylerwalls Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to just define what a consecutive import is and then stick to the letter of the rule. (We disable until the consecutive imports stop.)

For me, these two are consecutive imports:

try:
    import black
except ImportError:
    import ruff

import time

These two are not:

try:
    import black
except ImportError:
    HAS_BLACK = False
else:
    HAS_BLACK = True

import time

Same with if/with/for/while:

if PY314:
    import annotationlib

import time  # consecutive

def and class can't be empty, they must at least have a .../pass

if PY314:
    import annotationlib

    def get_annotations():
        pass  # a non-import statement

import time  # no longer consecutive

Does that seem sane?

Copy link

@ruck94301 ruck94301 Dec 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@jacobtylerwalls provides 4 examples of what he expects from the wrong-import-position rule itself.
I think you guys are discussing CHANGING the behavior, so these examples are not expected to match pylint 4.0.3. Nevertheless, it seems useful to state what existing 4.0.3 behavior is.

Your Expected 1 agrees with pylint 4.0.3.

Your Expected 2 disagrees. It seems like 4.0.3 treats the whole try-block as an import (a “wrong-import-position import”?, a WIPI?), because the try-clause smells like an import. The contents of the except-clause and else-clause are ignored. See tests/functional/w/wrong_import_position2.py

"""Checks import order rule with nested non_import sentence"""
# pylint: disable=unused-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
try:
    from sys import argv
except ImportError:
    pass
else:
    pass

import os

Also note that the try clause could contain non-imports before and after the “import black”, but those would not matter to the wrong-import-position algorithm. The try-block would STILL be assessed as an import for the purpose of this rule.
However if the try-clause DIDN'T contain an import, then yeah, the try block WOULDN'T smell like a WIPI.

Your Expected 3 -- disagrees with 4.0.3. ANY if-block is a non-import (non-WIPI) see tests/functional/w/wrong_import_position14.py

"""Checks import position rule"""
# pylint: disable=unused-import,undefined-variable,import-error
if x:
    import os
import y  # [wrong-import-position]

Your Expected 4 -- agrees, but still, function def in the content of the if-block doesn't matter.

If I was king, I think I wouldn't even try to parse the contents of try/if/while/for/with. I'd treat them (their block, that is) as non-import regardless of content -- that is, just like the if-statement seems to be treated now.
And report any following module-level imports as wrong-import-position.
And a disable-pragma on the try/if/while/for/with statement, or a disable-next before, should cause the whole block to be treated as if it was an import.
So, I'm not challenging the sanity of your expectations, but I'm offering a different (sane?) POV.
This too would be behavior changing because existing wrong_import_position2.py would be broken :-/

Copy link

@ruck94301 ruck94301 Dec 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Or, another POV -- I could see an argument for flattening try-blocks and ignoring its exception paths.
Replace

try:
    statement 1
except:
    statement 2
else:
    statement 3
finally:
    statement 4

with

statement 1
statement 3
statement 4

because you're representing the success path. And you'd want to recurse to handle the try inside try.
Meh, seems too clever by half. If I had a vote, I'd oppose descending below the first level of the ast nodes. Treat a 'try' node as a non-import.

And I'm still not seeing a good rationale for flattening with statements to be anything other than a non-import.


CONSTANT_B = True
import logging # [wrong-import-position]

# Inline pragma on import line
CONSTANT_C = 42
import json # pylint: disable=wrong-import-position
1 change: 1 addition & 0 deletions tests/functional/w/wrong_import_position_pragma_scope.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
wrong-import-position:12:0:12:14::"Import ""import logging"" should be placed at the top of the module":UNDEFINED