From e2da33a45c7a4c76166bc0c11bd9c3f451f47caf Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Thu, 16 Jan 2025 10:45:24 -0500 Subject: [PATCH] [`unconventional-import-alias`] Fix infinite loop between ICN001 and I002 (`ICN001`) (#15480) ## Summary This fixes the infinite loop reported in #14389 by raising an error to the user about conflicting ICN001 (`unconventional-import-alias`) and I002 (`missing-required-import`) configuration options. ## Test Plan Added a CLI integration test reproducing the old behavior and then confirming the fix. Closes #14389 --------- Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> --- crates/ruff/tests/lint.rs | 14 +++++ ...port_convention_unused_aliased_import.snap | 31 ++++++++++ crates/ruff_python_semantic/src/imports.rs | 2 +- crates/ruff_workspace/src/configuration.rs | 58 +++++++++++++++---- 4 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 crates/ruff/tests/snapshots/lint__flake8_import_convention_unused_aliased_import.snap diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 76b680aea50936..c9b136bcf75239 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -2160,3 +2160,17 @@ fn flake8_import_convention_invalid_aliases_config_module_name() -> Result<()> { "#);}); Ok(()) } + +#[test] +fn flake8_import_convention_unused_aliased_import() { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .arg("--config") + .arg(r#"lint.isort.required-imports = ["import pandas"]"#) + .args(["--select", "I002,ICN001,F401"]) + .args(["--stdin-filename", "test.py"]) + .arg("--unsafe-fixes") + .arg("--fix") + .arg("-") + .pass_stdin("1")); +} diff --git a/crates/ruff/tests/snapshots/lint__flake8_import_convention_unused_aliased_import.snap b/crates/ruff/tests/snapshots/lint__flake8_import_convention_unused_aliased_import.snap new file mode 100644 index 00000000000000..95379dfd9b9e45 --- /dev/null +++ b/crates/ruff/tests/snapshots/lint__flake8_import_convention_unused_aliased_import.snap @@ -0,0 +1,31 @@ +--- +source: crates/ruff/tests/lint.rs +info: + program: ruff + args: + - check + - "--no-cache" + - "--output-format" + - concise + - "--config" + - "lint.isort.required-imports = [\"import pandas\"]" + - "--select" + - "I002,ICN001,F401" + - "--stdin-filename" + - test.py + - "--unsafe-fixes" + - "--fix" + - "-" + stdin: "1" +snapshot_kind: text +--- +success: false +exit_code: 2 +----- stdout ----- + +----- stderr ----- +ruff failed + Cause: 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): + - `pandas` -> `pd` + +Help: Remove the required import or alias from your configuration. diff --git a/crates/ruff_python_semantic/src/imports.rs b/crates/ruff_python_semantic/src/imports.rs index 6ef0725e6d4978..069956e371fb11 100644 --- a/crates/ruff_python_semantic/src/imports.rs +++ b/crates/ruff_python_semantic/src/imports.rs @@ -57,7 +57,7 @@ impl NameImport { } /// Returns the [`QualifiedName`] of the imported name (e.g., given `from foo import bar as baz`, returns `["foo", "bar"]`). - fn qualified_name(&self) -> QualifiedName { + pub fn qualified_name(&self) -> QualifiedName { match self { NameImport::Import(import) => QualifiedName::user_defined(&import.name.name), NameImport::ImportFrom(import_from) => collect_import_from_member( diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 22436db9c92aea..877445b7664fd6 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -25,7 +25,7 @@ use ruff_linter::line_width::{IndentWidth, LineLength}; use ruff_linter::registry::RuleNamespace; use ruff_linter::registry::{Rule, RuleSet, INCOMPATIBLE_CODES}; use ruff_linter::rule_selector::{PreviewOptions, Specificity}; -use ruff_linter::rules::pycodestyle; +use ruff_linter::rules::{flake8_import_conventions, isort, pycodestyle}; use ruff_linter::settings::fix_safety_table::FixSafetyTable; use ruff_linter::settings::rule_table::RuleTable; use ruff_linter::settings::types::{ @@ -232,6 +232,21 @@ impl Configuration { let line_length = self.line_length.unwrap_or_default(); + let rules = lint.as_rule_table(lint_preview)?; + + // LinterSettings validation + let isort = lint + .isort + .map(IsortOptions::try_into_settings) + .transpose()? + .unwrap_or_default(); + let flake8_import_conventions = lint + .flake8_import_conventions + .map(Flake8ImportConventionsOptions::into_settings) + .unwrap_or_default(); + + conflicting_import_settings(&isort, &flake8_import_conventions)?; + Ok(Settings { cache_dir: self .cache_dir @@ -259,7 +274,7 @@ impl Configuration { #[allow(deprecated)] linter: LinterSettings { - rules: lint.as_rule_table(lint_preview)?, + rules, exclude: FilePatternSet::try_from_iter(lint.exclude.unwrap_or_default())?, extension: self.extension.unwrap_or_default(), preview: lint_preview, @@ -341,10 +356,7 @@ impl Configuration { .flake8_implicit_str_concat .map(Flake8ImplicitStrConcatOptions::into_settings) .unwrap_or_default(), - flake8_import_conventions: lint - .flake8_import_conventions - .map(Flake8ImportConventionsOptions::into_settings) - .unwrap_or_default(), + flake8_import_conventions, flake8_pytest_style: lint .flake8_pytest_style .map(Flake8PytestStyleOptions::try_into_settings) @@ -374,11 +386,7 @@ impl Configuration { .flake8_gettext .map(Flake8GetTextOptions::into_settings) .unwrap_or_default(), - isort: lint - .isort - .map(IsortOptions::try_into_settings) - .transpose()? - .unwrap_or_default(), + isort, mccabe: lint .mccabe .map(McCabeOptions::into_settings) @@ -1553,6 +1561,34 @@ fn warn_about_deprecated_top_level_lint_options( ); } +/// 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 { + let name = required_import.qualified_name().to_string(); + if let Some(alias) = flake8_import_conventions.aliases.get(&name) { + writeln!(err_body, " - `{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(()) +} + #[cfg(test)] mod tests { use std::str::FromStr;