enforce required imports even with useless alias #14287

dylwil3 · 2024-11-11T22:29:56Z

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 · 2024-11-11T22:30:36Z

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

github-actions · 2024-11-11T22:43:35Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser

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 · 2024-11-12T12:23:34Z

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 · 2024-11-12T21:01:30Z

crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs

+    let Some(asname) = &alias.asname else {
+        return;
+    };
+    if alias.name.contains('.') {


Why this part?

I don't know... This seems to have been there since the original implementation: https://github.com/harupy/ruff/blob/d765fbe5ddca1fa7b735256ebb303799a2e7aa95/src/pylint/plugins.rs#L16

I don't think it's possible to have an alias with a dot in it... maybe it's supposed to be an early return? Is checking for a dot faster than comparing two strings? Certainly can't be that much of a speed up... Anyway, happy to remove it.

Thanks. Do you mind removing it (here and in the linked location) in a separate PR?

Would it make sense to implement helper methods on the isort settings to test if it is a required import to avoid duplicating the logic?

Ok! Refactored to use new helper methods on isort settings, and I will follow up after this PR is merged to get rid of the check for "."

charliermarsh · 2024-11-13T03:43:40Z

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

MichaReiser · 2024-11-13T08:57:14Z

crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs

+    // While the fix is specified as always available,
+    // the presence of a user-specified required import (I002)
+    // with a useless alias causes an infinite loop. So
+    // in this case we keep the diagnostic but omit the fix.
+    // See https://github.com/astral-sh/ruff/issues/14283


I don't think our testing framework will accept that an always fixable fix isn't always fixable, which makes sense.

Of course you're right! Changed to "Sometimes" with special message/fix title emitted in case of conflict.

dylwil3 · 2024-11-14T05:45:50Z

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

Added!

MichaReiser · 2024-11-14T06:47:00Z

crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs

+    if checker
+        .settings
+        .isort
+        .requires_module_import(alias.name.to_string(), Some(asname.to_string()))
+    {
+        let diagnostic = Diagnostic::new(
+            UselessImportAlias {
+                required_import_conflict: true,
+            },
+            alias.range(),
+        );
+        checker.diagnostics.push(diagnostic);
+        return;
+    }
+
+    let mut diagnostic = Diagnostic::new(
+        UselessImportAlias {
+            required_import_conflict: false,
+        },
+        alias.range(),
+    );
+    diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
+        asname.to_string(),
+        alias.range(),
+    )));
+    checker.diagnostics.push(diagnostic);


Nit: To reduce the amount of repeated code

Suggested change

if checker

.settings

.isort

.requires_module_import(alias.name.to_string(), Some(asname.to_string()))

{

let diagnostic = Diagnostic::new(

UselessImportAlias {

required_import_conflict: true,

},

alias.range(),

);

checker.diagnostics.push(diagnostic);

return;

}

let mut diagnostic = Diagnostic::new(

UselessImportAlias {

required_import_conflict: false,

},

alias.range(),

);

diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(

asname.to_string(),

alias.range(),

)));

checker.diagnostics.push(diagnostic);

let mut diagnostic = Diagnostic::new(

UselessImportAlias {

required_import_conflict: true,

},

alias.range(),

);

if !checker

.settings

.isort

.requires_module_import(alias.name.to_string(), Some(asname.to_string()))

{

diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(

asname.to_string(),

alias.range(),

)));

}

checker.diagnostics.push(diagnostic);

MichaReiser · 2024-11-14T06:47:50Z

crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs

+    let Some(asname) = &alias.asname else {
+        return;
+    };
+    if alias.name.contains('.') {
+        return;
+    }
+    if alias.name.as_str() != asname.as_str() {
+        return;
+    }


Is it possible to move this as well into requires_member_import?

MichaReiser · 2024-11-14T06:48:26Z

crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs

+    if checker.settings.isort.requires_member_import(
+        module.map(str::to_string),
+        alias.name.to_string(),
+        Some(asname.to_string()),
+        level,
+    ) {
+        let diagnostic = Diagnostic::new(
+            UselessImportAlias {
+                required_import_conflict: true,
+            },
+            alias.range(),
+        );
+        checker.diagnostics.push(diagnostic);
+        return;
+    }

-    let mut diagnostic = Diagnostic::new(UselessImportAlias, alias.range());
+    let mut diagnostic = Diagnostic::new(
+        UselessImportAlias {
+            required_import_conflict: false,
+        },
+        alias.range(),
+    );


Nit: Same as above, let's invert the check so that we don't need to duplicate the diagnostic creation

MichaReiser · 2024-11-14T06:49:14Z

crates/ruff_linter/src/rules/isort/settings.rs

@@ -75,6 +75,29 @@ pub struct Settings {
    pub length_sort_straight: bool,
 }

+impl Settings {
+    pub fn requires_module_import(&self, name: String, as_name: Option<String>) -> bool {


Can we use these new methods in the isort implementation as well? It would help to ensure that both implementations stay in sync

enforce required imports even with useless alias

5cff5eb

MichaReiser reviewed Nov 12, 2024

View reviewed changes

dylwil3 added 2 commits November 12, 2024 07:06

omit fix not diagnostic

559d7da

add explanatory comment

77d4f3a

charliermarsh reviewed Nov 12, 2024

View reviewed changes

Rename

86d6655

MichaReiser added bug Something isn't working fixes Related to suggested fixes for violations labels Nov 13, 2024

MichaReiser reviewed Nov 13, 2024

View reviewed changes

dylwil3 added 4 commits November 13, 2024 23:40

add tests and fixtures

8186121

add convenience methods to isort settings

aa0d0f0

refactor rule, change fix avilability and message

6c944e1

snapshots

0560612

allow clippy violation so that message preview in docs looks nicer

5717d3b

MichaReiser approved these changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enforce required imports even with useless alias #14287

enforce required imports even with useless alias #14287

dylwil3 commented Nov 11, 2024 •

edited

Loading

dylwil3 commented Nov 11, 2024

github-actions bot commented Nov 11, 2024 •

edited

Loading

MichaReiser left a comment

dylwil3 commented Nov 12, 2024 •

edited

Loading

charliermarsh Nov 12, 2024

dylwil3 Nov 12, 2024

charliermarsh Nov 13, 2024

MichaReiser Nov 13, 2024

dylwil3 Nov 14, 2024

charliermarsh commented Nov 13, 2024

MichaReiser Nov 13, 2024

dylwil3 Nov 14, 2024

dylwil3 commented Nov 14, 2024

MichaReiser Nov 14, 2024

MichaReiser Nov 14, 2024

MichaReiser Nov 14, 2024

MichaReiser Nov 14, 2024

enforce required imports even with useless alias #14287

Are you sure you want to change the base?

enforce required imports even with useless alias #14287

Conversation

dylwil3 commented Nov 11, 2024 • edited Loading

Tests to confirm panic resolved:

dylwil3 commented Nov 11, 2024

github-actions bot commented Nov 11, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

MichaReiser left a comment

Choose a reason for hiding this comment

dylwil3 commented Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylwil3 commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylwil3 commented Nov 11, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

`ruff-ecosystem` results

dylwil3 commented Nov 12, 2024 •

edited

Loading