diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.py index 5c8b8fa26b..e500a3f4a9 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.py +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.py @@ -1,19 +1,19 @@ -from collections.abc import Set as AbstractSet # Ok +def f(): + from collections.abc import Set as AbstractSet # Ok -from collections.abc import Set # Ok +def f(): + from collections.abc import Container, Sized, Set as AbstractSet, ValuesView # Ok -from collections.abc import ( - Container, - Sized, - Set, # Ok - ValuesView -) +def f(): + from collections.abc import Set # PYI025 -from collections.abc import ( - Container, - Sized, - Set as AbstractSet, # Ok - ValuesView -) + +def f(): + from collections.abc import Container, Sized, Set, ValuesView # PYI025 + + GLOBAL: Set[int] = set() + + class Class: + member: Set[int] diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.pyi index c12ccdffb5..bda9c0d083 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.pyi +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI025.pyi @@ -1,19 +1,30 @@ -from collections.abc import Set as AbstractSet # Ok +def f(): + from collections.abc import Set as AbstractSet # Ok +def f(): + from collections.abc import Container, Sized, Set as AbstractSet, ValuesView # Ok -from collections.abc import Set # PYI025 +def f(): + from collections.abc import Set # PYI025 +def f(): + from collections.abc import Container, Sized, Set, ValuesView # PYI025 -from collections.abc import ( - Container, - Sized, - Set, # PYI025 - ValuesView -) +def f(): + if True: + from collections.abc import Set + else: + Set = 1 -from collections.abc import ( - Container, - Sized, - Set as AbstractSet, - ValuesView # Ok -) + x: Set = set() + + x: Set + + del Set + + def f(): + print(Set) + + def Set(): + pass + print(Set) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index c5260823c9..d9b1b9d3d4 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -817,24 +817,28 @@ where BindingKind::SubmoduleImportation(SubmoduleImportation { qualified_name, }), - BindingFlags::empty(), + BindingFlags::EXTERNAL, ); } else { + let mut flags = BindingFlags::EXTERNAL; + if alias.asname.is_some() { + flags |= BindingFlags::ALIAS; + } + if alias + .asname + .as_ref() + .map_or(false, |asname| asname == &alias.name) + { + flags |= BindingFlags::EXPLICIT_EXPORT; + } + let name = alias.asname.as_ref().unwrap_or(&alias.name); let qualified_name = &alias.name; self.add_binding( name, alias.identifier(self.locator), BindingKind::Importation(Importation { qualified_name }), - if alias - .asname - .as_ref() - .map_or(false, |asname| asname == &alias.name) - { - BindingFlags::EXPLICIT_EXPORT - } else { - BindingFlags::empty() - }, + flags, ); if let Some(asname) = &alias.asname { @@ -1052,9 +1056,6 @@ where } if self.is_stub { - if self.enabled(Rule::UnaliasedCollectionsAbcSetImport) { - flake8_pyi::rules::unaliased_collections_abc_set_import(self, import_from); - } if self.enabled(Rule::FutureAnnotationsInStub) { flake8_pyi::rules::from_future_import(self, import_from); } @@ -1116,6 +1117,18 @@ where } } + let mut flags = BindingFlags::EXTERNAL; + if alias.asname.is_some() { + flags |= BindingFlags::ALIAS; + } + if alias + .asname + .as_ref() + .map_or(false, |asname| asname == &alias.name) + { + flags |= BindingFlags::EXPLICIT_EXPORT; + } + // Given `from foo import bar`, `name` would be "bar" and `qualified_name` would // be "foo.bar". Given `from foo import bar as baz`, `name` would be "baz" // and `qualified_name` would be "foo.bar". @@ -1126,15 +1139,7 @@ where name, alias.identifier(self.locator), BindingKind::FromImportation(FromImportation { qualified_name }), - if alias - .asname - .as_ref() - .map_or(false, |asname| asname == &alias.name) - { - BindingFlags::EXPLICIT_EXPORT - } else { - BindingFlags::empty() - }, + flags, ); } if self.enabled(Rule::RelativeImports) { @@ -4711,23 +4716,18 @@ impl<'a> Checker<'a> { } fn check_dead_scopes(&mut self) { - let enforce_typing_imports = !self.is_stub - && self.any_enabled(&[ - Rule::GlobalVariableNotAssigned, - Rule::RuntimeImportInTypeCheckingBlock, - Rule::TypingOnlyFirstPartyImport, - Rule::TypingOnlyThirdPartyImport, - Rule::TypingOnlyStandardLibraryImport, - ]); - - if !(enforce_typing_imports - || self.any_enabled(&[ - Rule::UnusedImport, - Rule::UndefinedLocalWithImportStarUsage, - Rule::RedefinedWhileUnused, - Rule::UndefinedExport, - ])) - { + if !self.any_enabled(&[ + Rule::UnusedImport, + Rule::GlobalVariableNotAssigned, + Rule::UndefinedLocalWithImportStarUsage, + Rule::RedefinedWhileUnused, + Rule::RuntimeImportInTypeCheckingBlock, + Rule::TypingOnlyFirstPartyImport, + Rule::TypingOnlyThirdPartyImport, + Rule::TypingOnlyStandardLibraryImport, + Rule::UndefinedExport, + Rule::UnaliasedCollectionsAbcSetImport, + ]) { return; } @@ -4757,6 +4757,16 @@ impl<'a> Checker<'a> { // Identify any valid runtime imports. If a module is imported at runtime, and // used at runtime, then by default, we avoid flagging any other // imports from that model as typing-only. + let enforce_typing_imports = if self.is_stub { + false + } else { + self.any_enabled(&[ + Rule::RuntimeImportInTypeCheckingBlock, + Rule::TypingOnlyFirstPartyImport, + Rule::TypingOnlyThirdPartyImport, + Rule::TypingOnlyStandardLibraryImport, + ]) + }; let runtime_imports: Vec> = if enforce_typing_imports { if self.settings.flake8_type_checking.strict { vec![] @@ -4907,6 +4917,16 @@ impl<'a> Checker<'a> { if self.enabled(Rule::UnusedImport) { pyflakes::rules::unused_import(self, scope, &mut diagnostics); } + + if self.is_stub { + if self.enabled(Rule::UnaliasedCollectionsAbcSetImport) { + flake8_pyi::rules::unaliased_collections_abc_set_import( + self, + scope, + &mut diagnostics, + ); + } + } } self.diagnostics.extend(diagnostics); } diff --git a/crates/ruff/src/importer/mod.rs b/crates/ruff/src/importer/mod.rs index b7a67141b6..6acab6e471 100644 --- a/crates/ruff/src/importer/mod.rs +++ b/crates/ruff/src/importer/mod.rs @@ -1,4 +1,7 @@ -//! Add and modify import statements to make module members available during fix execution. +//! Code modification struct to add and modify import statements. +//! +//! Enables rules to make module members available (that may be not yet be imported) during fix +//! execution. use std::error::Error; diff --git a/crates/ruff/src/lib.rs b/crates/ruff/src/lib.rs index 4cba40f704..f1d884303e 100644 --- a/crates/ruff/src/lib.rs +++ b/crates/ruff/src/lib.rs @@ -29,6 +29,7 @@ mod noqa; pub mod packaging; pub mod pyproject_toml; pub mod registry; +mod renamer; pub mod resolver; mod rule_redirects; mod rule_selector; diff --git a/crates/ruff/src/renamer.rs b/crates/ruff/src/renamer.rs new file mode 100644 index 0000000000..05ab127f2e --- /dev/null +++ b/crates/ruff/src/renamer.rs @@ -0,0 +1,176 @@ +//! Code modification struct to support symbol renaming within a scope. + +use anyhow::{anyhow, Result}; + +use ruff_diagnostics::Edit; +use ruff_python_semantic::{Binding, BindingKind, Scope, SemanticModel}; + +pub(crate) struct Renamer; + +impl Renamer { + /// Rename a symbol (from `name` to `target`) within a [`Scope`]. + /// + /// ## How it works + /// + /// The renaming algorithm is as follows: + /// + /// 1. Start with the first [`Binding`] in the scope, for the given name. For example, in the + /// following snippet, we'd start by examining the `x = 1` binding: + /// + /// ```python + /// if True: + /// x = 1 + /// print(x) + /// else: + /// x = 2 + /// print(x) + /// + /// print(x) + /// ``` + /// + /// 2. Rename the [`Binding`]. In most cases, this is a simple replacement. For example, + /// renaming `x` to `y` above would require replacing `x = 1` with `y = 1`. After the + /// first replacement in the snippet above, we'd have: + /// + /// ```python + /// if True: + /// y = 1 + /// print(x) + /// else: + /// x = 2 + /// print(x) + /// + /// print(x) + /// ``` + /// + /// Note that, when renaming imports, we need to instead rename (or add) an alias. For + /// example, to rename `pandas` to `pd`, we may need to rewrite `import pandas` to + /// `import pandas as pd`, rather than `import pd`. + /// + /// 3. Rename every reference to the [`Binding`]. For example, renaming the references to the + /// `x = 1` binding above would give us: + /// + /// ```python + /// if True: + /// y = 1 + /// print(y) + /// else: + /// x = 2 + /// print(x) + /// + /// print(x) + /// ``` + /// + /// 4. Rename every delayed annotation. (See [`SemanticModel::delayed_annotations`].) + /// + /// 5. Repeat the above process for every [`Binding`] in the scope with the given name. + /// After renaming the `x = 2` binding, we'd have: + /// + /// ```python + /// if True: + /// y = 1 + /// print(y) + /// else: + /// y = 2 + /// print(y) + /// + /// print(y) + /// ``` + /// + /// ## Limitations + /// + /// `global` and `nonlocal` declarations are not yet supported. + /// + /// `global` and `nonlocal` declarations add some additional complexity. If we're renaming a + /// name that's declared as `global` or `nonlocal` in a child scope, we need to rename the name + /// in that scope too, repeating the above process. + /// + /// If we're renaming a name that's declared as `global` or `nonlocal` in the current scope, + /// then we need to identify the scope in which the name is declared, and perform the rename + /// in that scope instead (which will in turn trigger the above process on the current scope). + pub(crate) fn rename( + name: &str, + target: &str, + scope: &Scope, + semantic: &SemanticModel, + ) -> Result<(Edit, Vec)> { + let mut edits = vec![]; + + // Iterate over every binding to the name in the scope. + for binding_id in scope.get_all(name) { + let binding = semantic.binding(binding_id); + + // Rename the binding. + if let Some(edit) = Renamer::rename_binding(binding, name, target) { + edits.push(edit); + + // Rename any delayed annotations. + if let Some(annotations) = semantic.delayed_annotations(binding_id) { + edits.extend(annotations.iter().filter_map(|annotation_id| { + let annotation = semantic.binding(*annotation_id); + Renamer::rename_binding(annotation, name, target) + })); + } + + // Rename the references to the binding. + edits.extend(binding.references().map(|reference_id| { + let reference = semantic.reference(reference_id); + Edit::range_replacement(target.to_string(), reference.range()) + })); + } + } + + // Deduplicate any edits. In some cases, a reference can be both a read _and_ a write. For + // example, `x += 1` is both a read of and a write to `x`. + edits.sort(); + edits.dedup(); + + let edit = edits + .pop() + .ok_or(anyhow!("Unable to rename any references to `{name}`"))?; + Ok((edit, edits)) + } + + /// Rename a [`Binding`] reference. + fn rename_binding(binding: &Binding, name: &str, target: &str) -> Option { + match &binding.kind { + BindingKind::Importation(_) | BindingKind::FromImportation(_) => { + if binding.is_alias() { + // Ex) Rename `import pandas as alias` to `import pandas as pd`. + Some(Edit::range_replacement(target.to_string(), binding.range)) + } else { + // Ex) Rename `import pandas` to `import pandas as pd`. + Some(Edit::range_replacement( + format!("{name} as {target}"), + binding.range, + )) + } + } + BindingKind::SubmoduleImportation(import) => { + // Ex) Rename `import pandas.core` to `import pandas as pd`. + let module_name = import.qualified_name.split('.').next().unwrap(); + Some(Edit::range_replacement( + format!("{module_name} as {target}"), + binding.range, + )) + } + // Avoid renaming builtins and other "special" bindings. + BindingKind::FutureImportation | BindingKind::Builtin | BindingKind::Export(_) => None, + // By default, replace the binding's name with the target name. + BindingKind::Annotation + | BindingKind::Argument + | BindingKind::NamedExprAssignment + | BindingKind::UnpackedAssignment + | BindingKind::Assignment + | BindingKind::LoopVar + | BindingKind::Global + | BindingKind::Nonlocal + | BindingKind::ClassDefinition + | BindingKind::FunctionDefinition + | BindingKind::Deletion + | BindingKind::UnboundException => { + Some(Edit::range_replacement(target.to_string(), binding.range)) + } + } + } +} diff --git a/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs b/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs index db17d9c272..07a3039083 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs @@ -1,9 +1,10 @@ -use rustpython_parser::ast::StmtImportFrom; - -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_semantic::{BindingKind, FromImportation, Scope}; use crate::checkers::ast::Checker; +use crate::registry::AsRule; +use crate::renamer::Renamer; /// ## What it does /// Checks for `from collections.abc import Set` imports that do not alias @@ -30,6 +31,8 @@ use crate::checkers::ast::Checker; pub struct UnaliasedCollectionsAbcSetImport; impl Violation for UnaliasedCollectionsAbcSetImport { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + #[derive_message_formats] fn message(&self) -> String { format!( @@ -43,20 +46,33 @@ impl Violation for UnaliasedCollectionsAbcSetImport { } /// PYI025 -pub(crate) fn unaliased_collections_abc_set_import(checker: &mut Checker, stmt: &StmtImportFrom) { - let Some(module_id) = &stmt.module else { - return; - }; - if module_id.as_str() != "collections.abc" { - return; - } - - for name in &stmt.names { - if name.name.as_str() == "Set" && name.asname.is_none() { - checker.diagnostics.push(Diagnostic::new( - UnaliasedCollectionsAbcSetImport, - name.range, - )); +pub(crate) fn unaliased_collections_abc_set_import( + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, +) { + for (name, binding_id) in scope.all_bindings() { + let binding = checker.semantic().binding(binding_id); + let BindingKind::FromImportation(FromImportation { qualified_name }) = &binding.kind else { + continue; + }; + if qualified_name.as_str() != "collections.abc.Set" { + continue; } + if name == "AbstractSet" { + continue; + } + + let mut diagnostic = Diagnostic::new(UnaliasedCollectionsAbcSetImport, binding.range); + if checker.patch(diagnostic.kind.rule()) { + if checker.semantic().is_available("AbstractSet") { + diagnostic.try_set_fix(|| { + let (edit, rest) = + Renamer::rename(name, "AbstractSet", scope, checker.semantic())?; + Ok(Fix::suggested_edits(edit, rest)) + }); + } + } + diagnostics.push(diagnostic); } } diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap index 5c0dc3649d..b3659c1544 100644 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap @@ -1,22 +1,81 @@ --- source: crates/ruff/src/rules/flake8_pyi/mod.rs --- -PYI025.pyi:4:29: PYI025 Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin - | -4 | from collections.abc import Set # PYI025 - | ^^^ PYI025 - | - = help: Alias `Set` to `AbstractSet` - -PYI025.pyi:10:5: PYI025 Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025.pyi:8:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | - 8 | Container, - 9 | Sized, -10 | Set, # PYI025 - | ^^^ PYI025 -11 | ValuesView -12 | ) + 7 | def f(): + 8 | from collections.abc import Set # PYI025 + | ^^^ PYI025 + 9 | +10 | def f(): | = help: Alias `Set` to `AbstractSet` +ℹ Suggested fix +5 5 | from collections.abc import Container, Sized, Set as AbstractSet, ValuesView # Ok +6 6 | +7 7 | def f(): +8 |- from collections.abc import Set # PYI025 + 8 |+ from collections.abc import Set as AbstractSet # PYI025 +9 9 | +10 10 | def f(): +11 11 | from collections.abc import Container, Sized, Set, ValuesView # PYI025 + +PYI025.pyi:11:51: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin + | +10 | def f(): +11 | from collections.abc import Container, Sized, Set, ValuesView # PYI025 + | ^^^ PYI025 +12 | +13 | def f(): + | + = help: Alias `Set` to `AbstractSet` + +ℹ Suggested fix +8 8 | from collections.abc import Set # PYI025 +9 9 | +10 10 | def f(): +11 |- from collections.abc import Container, Sized, Set, ValuesView # PYI025 + 11 |+ from collections.abc import Container, Sized, Set as AbstractSet, ValuesView # PYI025 +12 12 | +13 13 | def f(): +14 14 | if True: + +PYI025.pyi:15:37: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin + | +13 | def f(): +14 | if True: +15 | from collections.abc import Set + | ^^^ PYI025 +16 | else: +17 | Set = 1 + | + = help: Alias `Set` to `AbstractSet` + +ℹ Suggested fix +12 12 | +13 13 | def f(): +14 14 | if True: +15 |- from collections.abc import Set + 15 |+ from collections.abc import Set as AbstractSet +16 16 | else: +17 |- Set = 1 + 17 |+ AbstractSet = 1 +18 18 | +19 |- x: Set = set() + 19 |+ x: AbstractSet = set() +20 20 | +21 |- x: Set + 21 |+ x: AbstractSet +22 22 | +23 |- del Set + 23 |+ del AbstractSet +24 24 | +25 25 | def f(): +26 |- print(Set) + 26 |+ print(AbstractSet) +27 27 | +28 28 | def Set(): +29 29 | pass + diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index ea79278389..819782a3af 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -44,6 +44,18 @@ impl<'a> Binding<'a> { self.flags.contains(BindingFlags::EXPLICIT_EXPORT) } + /// Return `true` if this [`Binding`] represents an external symbol + /// (e.g., `FastAPI` in `from fastapi import FastAPI`). + pub const fn is_external(&self) -> bool { + self.flags.contains(BindingFlags::EXTERNAL) + } + + /// Return `true` if this [`Binding`] represents an aliased symbol + /// (e.g., `app` in `from fastapi import FastAPI as app`). + pub const fn is_alias(&self) -> bool { + self.flags.contains(BindingFlags::ALIAS) + } + /// Return `true` if this [`Binding`] represents an unbound variable /// (e.g., `x` in `x = 1; del x`). pub const fn is_unbound(&self) -> bool { @@ -161,9 +173,25 @@ bitflags! { /// /// For example, the binding could be `FastAPI` in: /// ```python - /// import FastAPI as FastAPI + /// from fastapi import FastAPI as FastAPI /// ``` const EXPLICIT_EXPORT = 1 << 0; + + /// The binding represents an external symbol, like an import or a builtin. + /// + /// For example, the binding could be `FastAPI` in: + /// ```python + /// from fastapi import FastAPI + /// ``` + const EXTERNAL = 1 << 1; + + /// The binding is an aliased symbol. + /// + /// For example, the binding could be `app` in: + /// ```python + /// from fastapi import FastAPI as app + /// ``` + const ALIAS = 1 << 2; } }