Skip to content

Commit 8095ff0

Browse files
dylwil3charliermarsh
dylwil3
and
charliermarsh
authored
enforce required imports even with useless alias (#14287)
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. Closes #14283 --------- Co-authored-by: Charlie Marsh <[email protected]>
1 parent 24cd592 commit 8095ff0

8 files changed

Lines changed: 173 additions & 12 deletions

File tree

‎crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
import this as this

‎crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this_from.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
from module import this as this

‎crates/ruff_linter/src/checkers/ast/analyze/statement.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1028,7 +1028,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
10281028
}
10291029
if !checker.source_type.is_stub() {
10301030
if checker.enabled(Rule::UselessImportAlias) {
1031-
pylint::rules::useless_import_alias(checker, alias);
1031+
pylint::rules::useless_import_from_alias(checker, alias, module, level);
10321032
}
10331033
}
10341034
}

‎crates/ruff_linter/src/rules/isort/mod.rs‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -855,6 +855,57 @@ mod tests {
855855
Ok(())
856856
}
857857

858+
#[test_case(Path::new("this_this_from.py"))]
859+
fn required_importfrom_with_useless_alias(path: &Path) -> Result<()> {
860+
let snapshot = format!(
861+
"required_importfrom_with_useless_alias_{}",
862+
path.to_string_lossy()
863+
);
864+
let diagnostics = test_path(
865+
Path::new("isort/required_imports").join(path).as_path(),
866+
&LinterSettings {
867+
src: vec![test_resource_path("fixtures/isort")],
868+
isort: super::settings::Settings {
869+
required_imports: BTreeSet::from_iter([NameImport::ImportFrom(
870+
MemberNameImport::alias(
871+
"module".to_string(),
872+
"this".to_string(),
873+
"this".to_string(),
874+
),
875+
)]),
876+
..super::settings::Settings::default()
877+
},
878+
..LinterSettings::for_rules([Rule::MissingRequiredImport, Rule::UselessImportAlias])
879+
},
880+
)?;
881+
882+
assert_messages!(snapshot, diagnostics);
883+
Ok(())
884+
}
885+
886+
#[test_case(Path::new("this_this.py"))]
887+
fn required_import_with_useless_alias(path: &Path) -> Result<()> {
888+
let snapshot = format!(
889+
"required_import_with_useless_alias_{}",
890+
path.to_string_lossy()
891+
);
892+
let diagnostics = test_path(
893+
Path::new("isort/required_imports").join(path).as_path(),
894+
&LinterSettings {
895+
src: vec![test_resource_path("fixtures/isort")],
896+
isort: super::settings::Settings {
897+
required_imports: BTreeSet::from_iter([NameImport::Import(
898+
ModuleNameImport::alias("this".to_string(), "this".to_string()),
899+
)]),
900+
..super::settings::Settings::default()
901+
},
902+
..LinterSettings::for_rules([Rule::MissingRequiredImport, Rule::UselessImportAlias])
903+
},
904+
)?;
905+
assert_messages!(snapshot, diagnostics);
906+
Ok(())
907+
}
908+
858909
#[test_case(Path::new("docstring.py"))]
859910
#[test_case(Path::new("docstring.pyi"))]
860911
#[test_case(Path::new("docstring_only.py"))]

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

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use crate::display_settings;
1313
use crate::rules::isort::categorize::KnownModules;
1414
use crate::rules::isort::ImportType;
1515
use ruff_macros::CacheKey;
16-
use ruff_python_semantic::NameImport;
16+
use ruff_python_semantic::{Alias, MemberNameImport, ModuleNameImport, NameImport};
1717

1818
use super::categorize::ImportSection;
1919

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

78+
impl Settings {
79+
pub fn requires_module_import(&self, name: String, as_name: Option<String>) -> bool {
80+
self.required_imports
81+
.contains(&NameImport::Import(ModuleNameImport {
82+
name: Alias { name, as_name },
83+
}))
84+
}
85+
pub fn requires_member_import(
86+
&self,
87+
module: Option<String>,
88+
name: String,
89+
as_name: Option<String>,
90+
level: u32,
91+
) -> bool {
92+
self.required_imports
93+
.contains(&NameImport::ImportFrom(MemberNameImport {
94+
module,
95+
name: Alias { name, as_name },
96+
level,
97+
}))
98+
}
99+
}
100+
78101
impl Default for Settings {
79102
fn default() -> Self {
80103
Self {

‎crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_import_with_useless_alias_this_this.py.snap‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
source: crates/ruff_linter/src/rules/isort/mod.rs
3+
---
4+
this_this.py:1:8: PLC0414 Required import does not rename original package.
5+
|
6+
1 | import this as this
7+
| ^^^^^^^^^^^^ PLC0414
8+
|
9+
= help: Change required import or disable rule.

‎crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_importfrom_with_useless_alias_this_this_from.py.snap‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
source: crates/ruff_linter/src/rules/isort/mod.rs
3+
---
4+
this_this_from.py:1:20: PLC0414 Required import does not rename original package.
5+
|
6+
1 | from module import this as this
7+
| ^^^^^^^^^^^^ PLC0414
8+
|
9+
= help: Change required import or disable rule.

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

Lines changed: 77 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use ruff_python_ast::Alias;
22

3-
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
3+
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
44
use ruff_macros::{derive_message_formats, violation};
55
use ruff_text_size::Ranged;
66

@@ -28,16 +28,29 @@ use crate::checkers::ast::Checker;
2828
/// import numpy
2929
/// ```
3030
#[violation]
31-
pub struct UselessImportAlias;
31+
pub struct UselessImportAlias {
32+
required_import_conflict: bool,
33+
}
34+
35+
impl Violation for UselessImportAlias {
36+
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
3237

33-
impl AlwaysFixableViolation for UselessImportAlias {
3438
#[derive_message_formats]
3539
fn message(&self) -> String {
36-
"Import alias does not rename original package".to_string()
40+
#[allow(clippy::if_not_else)]
41+
if !self.required_import_conflict {
42+
"Import alias does not rename original package".to_string()
43+
} else {
44+
"Required import does not rename original package.".to_string()
45+
}
3746
}
3847

39-
fn fix_title(&self) -> String {
40-
"Remove import alias".to_string()
48+
fn fix_title(&self) -> Option<String> {
49+
if self.required_import_conflict {
50+
Some("Change required import or disable rule.".to_string())
51+
} else {
52+
Some("Remove import alias".to_string())
53+
}
4154
}
4255
}
4356

@@ -52,11 +65,65 @@ pub(crate) fn useless_import_alias(checker: &mut Checker, alias: &Alias) {
5265
if alias.name.as_str() != asname.as_str() {
5366
return;
5467
}
68+
// A required import with a useless alias causes an infinite loop.
69+
// See https://github.com/astral-sh/ruff/issues/14283
70+
let required_import_conflict = checker
71+
.settings
72+
.isort
73+
.requires_module_import(alias.name.to_string(), Some(asname.to_string()));
74+
let mut diagnostic = Diagnostic::new(
75+
UselessImportAlias {
76+
required_import_conflict,
77+
},
78+
alias.range(),
79+
);
80+
if !required_import_conflict {
81+
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
82+
asname.to_string(),
83+
alias.range(),
84+
)));
85+
}
86+
87+
checker.diagnostics.push(diagnostic);
88+
}
5589

56-
let mut diagnostic = Diagnostic::new(UselessImportAlias, alias.range());
57-
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
58-
asname.to_string(),
90+
/// PLC0414
91+
pub(crate) fn useless_import_from_alias(
92+
checker: &mut Checker,
93+
alias: &Alias,
94+
module: Option<&str>,
95+
level: u32,
96+
) {
97+
let Some(asname) = &alias.asname else {
98+
return;
99+
};
100+
if alias.name.contains('.') {
101+
return;
102+
}
103+
if alias.name.as_str() != asname.as_str() {
104+
return;
105+
}
106+
// A required import with a useless alias causes an infinite loop.
107+
// See https://github.com/astral-sh/ruff/issues/14283
108+
let required_import_conflict = checker.settings.isort.requires_member_import(
109+
module.map(str::to_string),
110+
alias.name.to_string(),
111+
Some(asname.to_string()),
112+
level,
113+
);
114+
let mut diagnostic = Diagnostic::new(
115+
UselessImportAlias {
116+
required_import_conflict,
117+
},
59118
alias.range(),
60-
)));
119+
);
120+
121+
if !required_import_conflict {
122+
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
123+
asname.to_string(),
124+
alias.range(),
125+
)));
126+
}
127+
61128
checker.diagnostics.push(diagnostic);
62129
}

0 commit comments

Comments
 (0)