fix: scope wrong-import-position pragma to specific line only

[emmanuel-ferdman]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Previously, a # pylint: disable=wrong-import-position pragma on a non-import statement would incorrectly suppress wrong-import-position messages for all subsequent import statements in the module. This violated the expected behavior where single-line pragmas should only affect their own line. The fix changes the message enablement check from using the first non-import node's line number to using the current import node's line number, ensuring proper pragma isolation.

Example:

import logging

logger = logging.getLogger()  # pylint: disable=wrong-import-position
import os  # This should trigger wrong-import-position
import sys  # This should trigger wrong-import-position

Expected Output:

• Before: No wrong-import-position messages (incorrect behavior)
• After: wrong-import-position messages for both import os and import sys (correct behavior)

Closes #10589

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.95%. Comparing base (68ab16f) to head (f957d35).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10590   +/-   ##
=======================================
  Coverage   95.95%   95.95%           
=======================================
  Files         176      176           
  Lines       19455    19455           
=======================================
  Hits        18668    18668           
  Misses        787      787           

Files with missing lines	Coverage Δ
pylint/checkers/imports.py	94.87% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Pierre-Sassoulas]

I'm wondering if the 'right way to fix this' isn't to return uninferable in astroid instead. Because it's possible that a function validly return an empty list.

[emmanuel-ferdman]

@Pierre-Sassoulas Should it be fixed in the astroid library itself or using an astroid hook in pylint library?

[Pierre-Sassoulas]

Astroid itself, we're the same maintainers and 99% of astroid user are also pylint users :)

[emmanuel-ferdman]

@Pierre-Sassoulas thanks for the guidance! I opened a PR in the astroid repo for this issue. Since this PR is already open, I also included a fix for another pylint issue. Apologies for the force push 🙌

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit f957d35

[DanielNoord]

Thanks for the PR!

In the future it would be better to open a new one as that makes communication scoped a bit better. We can still continue with this one though :)

[DanielNoord]

I'm not sure if this is the correct behaviour, or at least should be considered a breaking change?

The test you're changing above seems to explicitly test for this case:
A previous disable should also count for subsequent imports.

@Pierre-Sassoulas What do you think?

[Pierre-Sassoulas]

Ha, yes, we reviewed independently but I think we said the same thing..

[DanielNoord]

@Pierre-Sassoulas You approved, but this question still stands I guess? We're now actively removing support for behaviour for which we had a test. I'm not sure why we added this behaviour in the first place, but considering we are changing behaviour isn't this a breaking change?

[Pierre-Sassoulas]

Right, we didn't say the same thing after all. It seems we can't have an easy implementation of both "disable on a statement" and "any disable prevent all further wrong-import-position" so there's a decision to take here. And we might have to do the hard implementation, especially since I don't think isort or ruff deal with intertwined statements in imports (?).

[DanielNoord]

Current PR does:

The fix changes the message enablement check from using the first non-import node's line number to using the current import node's line number, ensuring proper pragma isolation.

Perhaps we can it to:

The fix changes the message enablement check from no longer working on non-import statements for this message. A pragma will disable from the current import statement until further statements, ensuring proper pragma isolation.

[Pierre-Sassoulas]

Instinctively I would say the fix need to be a lot more complex than that. If one line is disabled it means not only that the message shouldn't be raised for it but also that other line should not raise if the only issue for them it the disabled line.

i.e. the expected is

import a
import b
import c

import b  # [wrong-import-position]
import a  # [wrong-import-position]
import c 

import b  # pylint: disable=wrong-import-position
import a  
import c  

import b  # pylint: disable=wrong-import-position
import c   # [wrong-import-position]
import a   # [wrong-import-position]

Tbf, I don't think we should sweat other this, maybe we need to abandon the pylint disable system for this specific error message, and take the isort/ruff "I" disable into account, or drop the message entirely, or integrate ruff plus autofix and drop isort, or integrate both ruff and isort... I don't see a fix for this being super impactful in the mid term.

[DanielNoord]

I don't fully understand your comment, and I think taking into accounts other tools setting is bit too much work for @emmanuel-ferdman 😅

@emmanuel-ferdman Do you think we can fix your issue while still keeping the behaviour as being tested in the test? I agree that a disable on a non-import makes no sense, but perhaps we can fix it with a smaller scope?

[Pierre-Sassoulas]

What I mean is that the effort to fix this at all is not worth it as we have ruff and isort for reordering import, having working disable in pylint or coordinating multiple tools that check this message does not bring a lot of value imo. (Maybe we can even remove the check entirely for 4.0)

[emmanuel-ferdman]

@DanielNoord @Pierre-Sassoulas Thanks for the feedback 🙌
From issue #10589 and seeing that other pylint rules are line-based, I assumed this was a bug. But I understand that wrong-import-position might be different since it's about import ordering patterns. What should we do here? I'm happy to try and implement whatever you think is best.

[Pierre-Sassoulas]

Sorry for marking an issue as ready for PR when clearly it wasn't well specified @emmanuel-ferdman. Or thank you for making the fix and making us realize what actually fixing this bug imply. I think we should close because there's probably better use for your time. Right now I'm more and more convinced that we should either drop isort and wrong-import-position (import time at startup could make this a big change for users) or add ruff (using ruff and isort as optional dependencies instead of isort as mandatory dependency would open a world of possibilities once the wrong-import-position proof of concept is validated). In all case this mean this fix is not going to be used much. Need some other maintainers opinion about it and it's probably an idea for pylint 5.0 anyway (that I should open an issue for if Daniel don't think it's trash).

[DanielNoord]

I agree with removing the dependency on isort.

It fails logical to let those concerns be dealt with by a separate tool with its own configuration and/or ignores. Wondering what other maintainers think.

[ruck94301]

@Pierre-Sassoulas, you gave 4 examples above (importing a, b, & c).
Those seem like wrong-import-order C0411, not wrong-import-position C0413.
I mean, I think they are actually not the expected. But if you change to the pragma and expected messages to wrong-import-order, yes that's the expected.

The original bug report is about wrong-import-position, which maybe doesn't require sorting & other complexities?

At any rate, thanks for taking my report seriously, I appreciate that! :)

[Pierre-Sassoulas]

Ha, indeed you're right, my bad. Well the fix LGTM as is after a second take. I'm going to create an issue for the unrelated discussion.

[ruck94301]

Sorry to risk additional confusion. I will try hard to be clear.
The issue in this bug is regarding C0413 wrong-import-position, not C0411 wrong-import-order. That has already been clarified.

However, during the above conversation, I agreed with the expectations of a different condition, C0411 wrong-import-order, as shown with importing a, b, & c in different orders.

import os  # [wrong-import-order]
import logging  # [wrong-import-order]
import sys  

which seemed reasonable. But OTOH, this also seems reasonable to the user:

import os
import logging  # [wrong-import-order]
import sys  

So, which is actually implemented? Easy to check...
Neither. C0411 (in pylint 3.3.7) doesn't seem to verify the sort order of the libraries within the three "families"-- at least the standard libraries family. Because I'm seeing no C0411 on the following.

import os
import logging
import sys  

I DO get C0411 when I, for example, import a third-party lib in the wrong place.

import os
import requests
import logging  # [wrong-import-order]
import sys  # [wrong-import-order]

Maybe C0411 doesn't attempt to validate the "alphabetical" sort order recommendation of PEP 8, and concerns itself only with whether the three sets of imports are correctly organized per PEP 8 like (1) standard lib imports, (2) related 3rd party lib imports (3) Local application/library specific imports.
So even though I cannot see how this is relevant to C0413, I wanted to clarify C0411 existing behavior in 3.3.7. Hope this helps. LMK if you think I'm misunderstanding something.
Best Regards,
John