fix: support block-level wrong-import-position pragma suppression #10590
Conversation
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10590 +/- ##
==========================================
+ Coverage 95.95% 95.98% +0.03%
==========================================
Files 176 176
Lines 19455 19561 +106
==========================================
+ Hits 18668 18776 +108
+ Misses 787 785 -2
π New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
left a comment
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
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.
|
@Pierre-Sassoulas Should it be fixed in the astroid library itself or using an astroid hook in pylint library? |
|
Astroid itself, we're the same maintainers and 99% of astroid user are also pylint users :) |
Signed-off-by: Emmanuel Ferdman <[email protected]>
emmanuel-ferdman
force-pushed
the
main
branch
from
28128d9 to
8cace41
Compare
emmanuel-ferdman
changed the title
Signed-off-by: Emmanuel Ferdman <[email protected]>
emmanuel-ferdman
force-pushed
the
main
branch
from
dd9aafb to
a7e937a
Compare
for more information, see https://pre-commit.ci
|
@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 π |
This comment has been minimized.
This comment has been minimized.
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
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 :)
| import json # pylint: disable=wrong-import-position | ||
|
|
||
| # Test that subsequent imports are not affected by the pragma above | ||
| import csv # [wrong-import-position] | ||
| import re # [wrong-import-position] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ha, yes, we reviewed independently but I think we said the same thing..
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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 (?).
There was a problem hiding this comment.
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.
pylint/checkers/imports.py
Outdated
| if self.linter.is_message_enabled( | ||
| "wrong-import-position", self._first_non_import_node.fromlineno | ||
| ): | ||
| if self.linter.is_message_enabled("wrong-import-position", node.fromlineno): |
There was a problem hiding this comment.
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 cimport 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Pierre-Sassoulas
added
the
Needs decision π
|
@Pierre-Sassoulas, you gave 4 examples above (importing a, b, & c). 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! :) |
|
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. |
Pierre-Sassoulas
added
Bug πͺ²
backport maintenance/4.0.x
and removed
Needs decision π
|
Sorry to risk additional confusion. I will try hard to be clear. 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. which seemed reasonable. But OTOH, this also seems reasonable to the user: So, which is actually implemented? Easy to check... I DO get C0411 when I, for example, import a third-party lib in the wrong place. 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. |
We're removing a dubious feature. Maybe we can avoid backporting in that case. It was added in #1337 without much discussion. |
Signed-off-by: Emmanuel Ferdman <[email protected]>
emmanuel-ferdman
changed the title
for more information, see https://pre-commit.ci
|
For now, I followed @Pierre-Sassoulasβs suggestion: a pragma on the non-import line that breaks the import block suppresses the following imports until the next non-import. Inline disables on the import line still work as before. Please let me know what you think. Example: import os
import sys
# First non-import with pragma - suppresses following imports
CONSTANT1 = "value1" # pylint: disable=wrong-import-position
import json
import csv
# Second non-import without pragma - imports after this should be flagged
CONSTANT2 = "value2"
import logging
# Third non-import with pragma - suppresses following imports again
CONSTANT3 = "value3" # pylint: disable=wrong-import-position
import pathlib
import random
# Fourth non-import without pragma - import after this should be flagged
CONSTANT4 = "value4"
import reOutput: |
Signed-off-by: Emmanuel Ferdman <[email protected]>
This comment has been minimized.
This comment has been minimized.
|
@Pierre-Sassoulas @jacobtylerwalls Sorry to follow up again, but I'm looking for feedback on new approch and guidance on the next step when you get a chance to look into this π |
Pierre-Sassoulas
left a comment
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
I like the current approach but I'm not sure we had an agreement about what to do as Jacob/Daniel liked/approved the previous one. It's a check I very rarely disable personally so I'm not going to die on any hill.
|
|
||
| # Pragma on non-import suppresses following imports until next non-import | ||
| CONSTANT_A = False # pylint: disable=wrong-import-position | ||
| import time |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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 timeThese two are not:
try:
import black
except ImportError:
HAS_BLACK = False
else:
HAS_BLACK = True
import timeSame with if/with/for/while:
if PY314:
import annotationlib
import time # consecutivedef 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 consecutiveDoes that seem sane?
There was a problem hiding this comment.
@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 :-/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@jacobtylerwalls Thanks for the detailed explanation! Before I implement, I want to confirm: @ruck94301 suggested a simpler approach - treat all control-flow blocks as non-imports regardless of content. Would you prefer that simpler approach, or should I implement your 'consecutive imports' logic that inspects block contents?
There was a problem hiding this comment.
I found @ruck94301's reminder to keep it not clever compelling.
Let's do this:
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.
Most important for me is just treating try and if similarly.
jacobtylerwalls
removed
the
Needs decision π
Signed-off-by: Emmanuel Ferdman <[email protected]>
emmanuel-ferdman
dismissed stale reviews from DanielNoord and Pierre-Sassoulas
via
13f6f12
This comment has been minimized.
This comment has been minimized.
|
@jacobtylerwalls Done. All control-flow blocks ( |
|
Thanks. Now, with the benefit of the primer result, I'm realizing that my advice about handling What do you recommend? |
Signed-off-by: Emmanuel Ferdman <[email protected]>
for more information, see https://pre-commit.ci
|
@jacobtylerwalls Yeah, I see what you mean. In Django, I see plenty of cases where we can't fix the code by moving the import, only with disable rule, for example: try:
import MySQLdb
except ImportError:
raise Error("Install mysql")
from MySQLdb.constants import x # Should this be flagged?I did some research on how other linters/formatters handle this. I tried to compare them against the different versions of this PR:
The table I got:
Legend - β = allowed, β = flagged, see test examples below. Import sorters:
Looks like every tool either allows imports after try/except, ignores them, or treats them as separate blocks. None flag them as errors. So I think we should exempt Test examples: try_with_importimport os
try:
import json
except ImportError:
pass
import sys # <- flagged?try_without_importimport os
try:
x = 1
except Exception:
pass
import sys # <- flagged?try_finallyimport os
try:
import json
finally:
pass
import sys # <- flagged?try_elseimport os
try:
import json
except ImportError:
pass
else:
x = 1
import sys # <- flagged?if_with_importimport os
if True:
import json
import sys # <- flagged?if_without_importimport os
if True:
x = 1
import sys # <- flagged?if TYPE_CHECKINGfrom typing import TYPE_CHECKING
if TYPE_CHECKING:
from collections import OrderedDict
import sys # <- flagged?elseimport os
if False:
pass
else:
x = 1
import sys # <- flagged?forimport os
for i in []:
pass
import sys # <- flagged?whileimport os
while False:
pass
import sys # <- flagged?withimport os
with open("/dev/null"):
pass
import sys # <- flagged?matchimport os
match 1:
case 1:
pass
import sys # <- flagged?classimport os
class Foo:
pass
import sys # <- flagged?defimport os
def foo():
pass
import sys # <- flagged?async defimport os
async def foo():
pass
import sys # <- flagged?decorated funcimport os
def decorator(f):
return f
@decorator
def foo():
pass
import sys # <- flagged?assignmentimport os
x = 1
import sys # <- flagged?funcallimport os
print("hello")
import sys # <- flagged? |
|
π€ Effect of this PR on checked open source code: π€ Effect on ansible: Details
Effect on pandas: Details
Effect on pytest: Details
Effect on poetry-core: Details
Effect on sentry: Details
Effect on black: Details
Effect on home-assistant: Details
This comment was generated for commit 771ac7c |
jacobtylerwalls
left a comment
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thanks, this really morphed from where we started, but looking at the primer that is a huge improvement.
@DanielNoord @Pierre-Sassoulas are you okay with where we took this?
There was a problem hiding this comment.
Can you remove this file and the other blank one?
| CONSTANT_C = 42 | ||
| import json # pylint: disable=wrong-import-position | ||
|
|
||
| CONSTANT_D = "test" # pylint: disable=wrong-import-position |
There was a problem hiding this comment.
Is this here to test two pragmas in a row? Might be nice to comment that.
|
|
||
| import base64 # [wrong-import-position] | ||
|
|
||
| try: # pylint: disable=wrong-import-position |
There was a problem hiding this comment.
These examples seem to follow different flows. I guess I'd expect to see a pattern of pragma, new statement, import statement. Just above this there is no new pragma before the while, with, and match examples, so I don't know what state is being tested.
| Allow ``wrong-import-position`` pragma on non-import lines to suppress following imports until the next non-import statement. | ||
|
|
||
| Closes #10589 |
There was a problem hiding this comment.
Now we can update this to reflect the larger change we are making.
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
Yeah, this LGTM pragma-handling wise!
5 participants
Type of Changes
Description
Previously, a
# pylint: disable=wrong-import-positionpragma 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:
Expected Output:
wrong-import-positionmessages (incorrect behavior)wrong-import-positionmessages for bothimport osandimport sys(correct behavior)Closes #10589