Skip to content

[ruff] Prevent PYI025 from clashing with isort-required check #21115

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.

Sign up for GitHub

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

Closed
TaKO8Ki wants to merge 1 commit into from
+46 −0
Closed

[ruff] Prevent PYI025 from clashing with isort-required check #21115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions crates/ruff/tests/cli/lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3652,3 +3652,33 @@ fn supported_file_extensions_preview_enabled() -> Result<()> {
");
Ok(())
}

// Regression test for https://github.com/astral-sh/ruff/issues/20891
#[test]
fn required_import_skips_pyi025() {
assert_cmd_snapshot!(
Command::new(get_cargo_bin(BIN_NAME))
.arg("--isolated")
.arg("check")
.args(["--select", "F401,I002,PYI025"])
.arg("--config")
.arg(r#"lint.isort.required-imports=["from collections.abc import Set"]"#)
.arg("-")
.arg("--unsafe-fixes")
.arg("--no-cache")
.pass_stdin("1"),
@r"
success: false
exit_code: 1
----- stdout -----
I002 [*] Missing required import: `from collections.abc import Set`
--> -:1:1
help: Insert required import: `from collections.abc import Set`

Found 1 error.
[*] 1 fixable with the --fix option.

----- stderr -----
"
);
}
16 changes: 16 additions & 0 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{Stmt, StmtImportFrom};
use ruff_python_semantic::Imported;
use ruff_python_semantic::{Binding, BindingKind, Scope};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::renamer::Renamer;
use crate::rules::pyupgrade::rules::is_import_required_by_isort;
use crate::{Applicability, Fix, FixAvailability, Violation};

/// ## What it does
Expand Down Expand Up @@ -73,6 +75,20 @@ pub(crate) fn unaliased_collections_abc_set_import(checker: &Checker, binding: &
return;
}

// Skip if this import is required by isort to prevent infinite loops with I002 and F401
if let Some(stmt @ Stmt::ImportFrom(StmtImportFrom { names, .. })) =
binding.statement(checker.semantic())
&& names.iter().any(|alias| {
is_import_required_by_isort(
&checker.settings().isort.required_imports,
stmt.into(),
alias,
)
})
{
return;
}
Comment on lines +78 to +90
Copy link
Member

@AlexWaygood AlexWaygood Oct 29, 2025

Choose a reason for hiding this comment

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

I don't think this is the best fix. To me I think it would make more sense to reject this configuration entirely at an earlier stage. Fundamentally, it just doesn't make sense to tell Ruff that you simultaneously want from collections.abc import Set to always appear in every module, and also from collections.abc import Set to never appear in any module.

That implies that we'd want to edit this part of the code:

ruff/crates/ruff_workspace/src/configuration.rs

Lines 1614 to 1651 in aca8ba7

/// Detect conflicts between I002 (missing-required-import) and ICN001 (unconventional-import-alias)
fn conflicting_import_settings(
isort: &isort::settings::Settings,
flake8_import_conventions: &flake8_import_conventions::settings::Settings,
) -> Result<()> {
use std::fmt::Write;
let mut err_body = String::new();
for required_import in &isort.required_imports {
// Ex: `from foo import bar as baz` OR `import foo.bar as baz`
// - qualified name: `foo.bar`
// - bound name: `baz`
// - conflicts with: `{"foo.bar":"buzz"}`
// - does not conflict with either of
// - `{"bar":"buzz"}`
// - `{"foo.bar":"baz"}`
let qualified_name = required_import.qualified_name().to_string();
let bound_name = required_import.bound_name();
let Some(alias) = flake8_import_conventions.aliases.get(&qualified_name) else {
continue;
};
if alias != bound_name {
writeln!(err_body, " - `{qualified_name}` -> `{alias}`").unwrap();
}
}
if !err_body.is_empty() {
return Err(anyhow!(
"Required import specified in `lint.isort.required-imports` (I002) \
conflicts with the required import alias specified in either \
`lint.flake8-import-conventions.aliases` or \
`lint.flake8-import-conventions.extend-aliases` (ICN001):\
\n{err_body}\n\
Help: Remove the required import or alias from your configuration."
));
}
Ok(())
}

Copy link
Contributor

@ntBre ntBre Oct 29, 2025

Choose a reason for hiding this comment

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

I thought about that too. This seemed like a pretty niche issue, so I didn't feel too strongly, but this also makes sense to me.

Copy link
Member

@AlexWaygood AlexWaygood Oct 29, 2025

Choose a reason for hiding this comment

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

I generally have a pretty strong preference towards rejecting invalid configurations rather than silently accepting them and assuming we understand what the user actually wanted 😄

Copy link
Contributor

@ntBre ntBre Oct 29, 2025

Choose a reason for hiding this comment

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

We've done a bit of both with these conflicts, I think. #16802 is one example, but maybe that's different 🙂

Copy link
Contributor Author

@TaKO8Ki TaKO8Ki Oct 31, 2025

Choose a reason for hiding this comment

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

Ok. I will check it.


let mut diagnostic =
checker.report_diagnostic(UnaliasedCollectionsAbcSetImport, binding.range());
if checker.semantic().is_available("AbstractSet") {
Expand Down