Skip to content
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.

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

enforce required imports even with useless alias #14287

Merged
merged 10 commits into from
Nov 14, 2024

Conversation

dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Nov 11, 2024

This PR handles a panic that occurs when applying unsafe fixes if a user inserts a required import (I002) that has a "useless alias" in it, like import numpy as numpy, and also selects PLC0414 (useless-import-alias)

In this case, the fixes alternate between adding the required import statement, then removing the alias, until the recursion limit is reached. See linked issue for an example.

Tests to confirm panic resolved:

$ echo 1 | cargo run --quiet -p ruff -- check --no-cache --isolated --select I002,PLC0414 --config 'lint.isort.required-imports = ["from module import this as this"]' - --unsafe-fixes --fix
from module import this as this
1
Found 1 error (1 fixed, 0 remaining).
$ echo 1 | cargo run --quiet -p ruff -- check --no-cache --isolated --select I002,PLC0414 --config 'lint.isort.required-imports = ["import numpy as numpy"]' - --unsafe-fixes --fix
import numpy as numpy
1
Found 1 error (1 fixed, 0 remaining).

Closes #14283

@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 11, 2024

Not so happy about the big diff in such a simple rule for an edge case... I welcome alternative suggestions!

Copy link
Contributor

github-actions bot commented Nov 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Could you expand the PR summary with details about what was causing the infinite loop or panic? It would help me review this PR and possibly suggest alternatives.

@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 12, 2024

Done! It also occurred to me that it might be better to keep the diagnostic but elide the fix for useless-import-alias. That would serve the purpose of warning the user that something was amiss, and they could override the warning by ignoring the rule.

Edit: Made this change. I did not change the "Fix availability" or documentation because it seems like this is a very rare (nonexistent?) exception, but let me know what you think.

@charliermarsh
Copy link
Member

Do you mind adding a dedicated test for this? Sorry!

@MichaReiser MichaReiser added bug Something isn't working fixes Related to suggested fixes for violations labels Nov 13, 2024
@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 14, 2024

Do you mind adding a dedicated test for this? Sorry!

Added!

@dylwil3 dylwil3 merged commit 8095ff0 into astral-sh:main Nov 14, 2024
20 checks passed
@dylwil3 dylwil3 deleted the isort-alias branch November 14, 2024 21:39
dylwil3 added a commit that referenced this pull request Nov 14, 2024
…s (PLC0414)` (#14345)

Follow-up to #14287 : when checking that `name` is the same as `as_name`
in `import name as as_name`, we do not need to first do an early return
if `'.'` is found in `name`.
@dylwil3 dylwil3 restored the isort-alias branch November 17, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infinite loop] PLC0414 conflicts with I002 if a useless alias is required
3 participants