From e151ca678c91cf495cc137af1599a65f7c4c404f Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Sun, 24 Mar 2024 14:31:06 +0000 Subject: [PATCH] Mark PYI025 fix as safe in more cases for stub files --- .../test/fixtures/flake8_pyi/PYI025_3.py | 6 ++ .../test/fixtures/flake8_pyi/PYI025_3.pyi | 6 ++ .../ruff_linter/src/rules/flake8_pyi/mod.rs | 2 + .../unaliased_collections_abc_set_import.rs | 60 +++++++++++++------ ...lake8_pyi__tests__PYI025_PYI025_1.pyi.snap | 2 +- ...flake8_pyi__tests__PYI025_PYI025_3.py.snap | 18 ++++++ ...lake8_pyi__tests__PYI025_PYI025_3.pyi.snap | 18 ++++++ 7 files changed, 92 insertions(+), 20 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.py create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.pyi create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.pyi.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.py new file mode 100644 index 0000000000000..af813a779dd44 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.py @@ -0,0 +1,6 @@ +""" +Tests for PYI025 where the import is marked as re-exported +through usage of a "redundant" `import Set as Set` alias +""" + +from collections.abc import Set as Set # PYI025 triggered but fix is not marked as safe diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.pyi new file mode 100644 index 0000000000000..af813a779dd44 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_3.pyi @@ -0,0 +1,6 @@ +""" +Tests for PYI025 where the import is marked as re-exported +through usage of a "redundant" `import Set as Set` alias +""" + +from collections.abc import Set as Set # PYI025 triggered but fix is not marked as safe diff --git a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs index 6f4166339e54c..004aacd280bfe 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs @@ -83,6 +83,8 @@ mod tests { #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_1.pyi"))] #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_2.py"))] #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_2.pyi"))] + #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_3.py"))] + #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_3.pyi"))] #[test_case(Rule::UnannotatedAssignmentInStub, Path::new("PYI052.py"))] #[test_case(Rule::UnannotatedAssignmentInStub, Path::new("PYI052.pyi"))] #[test_case(Rule::UnassignedSpecialVariableInStub, Path::new("PYI035.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs index 873df56106db7..ec8e40a22d7d7 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_semantic::Imported; -use ruff_python_semantic::{Binding, BindingKind}; +use ruff_python_semantic::{Binding, BindingKind, Scope}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -32,9 +32,14 @@ use crate::renamer::Renamer; /// /// ## Fix safety /// This rule's fix is marked as unsafe for `Set` imports defined at the -/// top-level of a module. Top-level symbols are implicitly exported by the +/// top-level of a `.py` module. Top-level symbols are implicitly exported by the /// module, and so renaming a top-level symbol may break downstream modules /// that import it. +/// +/// The same is not true for `.pyi` files, where imported symbols are only +/// re-exported if they are included in `__all__`, use a "redundant" +/// `import foo as foo` alias, or are imported via a `*` import. As such, the +/// fix is marked as safe in more cases for `.pyi` files. #[violation] pub struct UnaliasedCollectionsAbcSetImport; @@ -76,24 +81,41 @@ pub(crate) fn unaliased_collections_abc_set_import( let mut diagnostic = Diagnostic::new(UnaliasedCollectionsAbcSetImport, binding.range()); if checker.semantic().is_available("AbstractSet") { diagnostic.try_set_fix(|| { - let scope = &checker.semantic().scopes[binding.scope]; - let (edit, rest) = Renamer::rename( - name, - "AbstractSet", - scope, - checker.semantic(), - checker.stylist(), - )?; - Ok(Fix::applicable_edits( - edit, - rest, - if scope.kind.is_module() { - Applicability::Unsafe - } else { - Applicability::Safe - }, - )) + let semantic = checker.semantic(); + let scope = &semantic.scopes[binding.scope]; + let (edit, rest) = + Renamer::rename(name, "AbstractSet", scope, semantic, checker.stylist())?; + let applicability = determine_applicability(binding, scope, checker); + Ok(Fix::applicable_edits(edit, rest, applicability)) }); } Some(diagnostic) } + +fn determine_applicability(binding: &Binding, scope: &Scope, checker: &Checker) -> Applicability { + // If it's not in a module scope, the import can't be implicitly re-exported, + // so always mark it as safe + if !scope.kind.is_module() { + return Applicability::Safe; + } + // If it's not a stub and it's in the module scope, always mark the fix as unsafe + if !checker.source_type.is_stub() { + return Applicability::Unsafe; + } + // If the import was `from collections.abc import Set as Set`, + // it's being explicitly re-exported: mark the fix as unsafe + if binding.is_explicit_export() { + return Applicability::Unsafe; + } + // If it's included in `__all__`, mark the fix as unsafe + if binding.references().any(|reference| { + checker + .semantic() + .reference(reference) + .in_dunder_all_definition() + }) { + return Applicability::Unsafe; + } + // Anything else in a stub, and it's a safe fix: + Applicability::Safe +} diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap index 53bb3f37dc5c0..208551931e8c2 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap @@ -87,7 +87,7 @@ PYI025_1.pyi:33:29: PYI025 [*] Use `from collections.abc import Set as AbstractS | = help: Alias `Set` to `AbstractSet` -ℹ Unsafe fix +ℹ Safe fix 17 17 | else: 18 18 | Set = 1 19 19 | diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.py.snap new file mode 100644 index 0000000000000..008e68502946c --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.py.snap @@ -0,0 +1,18 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- +PYI025_3.py:6:36: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin + | +4 | """ +5 | +6 | from collections.abc import Set as Set # PYI025 triggered but fix is not marked as safe + | ^^^ PYI025 + | + = help: Alias `Set` to `AbstractSet` + +ℹ Unsafe fix +3 3 | through usage of a "redundant" `import Set as Set` alias +4 4 | """ +5 5 | +6 |-from collections.abc import Set as Set # PYI025 triggered but fix is not marked as safe + 6 |+from collections.abc import Set as AbstractSet # PYI025 triggered but fix is not marked as safe diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.pyi.snap new file mode 100644 index 0000000000000..ae54570974134 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_3.pyi.snap @@ -0,0 +1,18 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- +PYI025_3.pyi:6:36: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin + | +4 | """ +5 | +6 | from collections.abc import Set as Set # PYI025 triggered but fix is not marked as safe + | ^^^ PYI025 + | + = help: Alias `Set` to `AbstractSet` + +ℹ Unsafe fix +3 3 | through usage of a "redundant" `import Set as Set` alias +4 4 | """ +5 5 | +6 |-from collections.abc import Set as Set # PYI025 triggered but fix is not marked as safe + 6 |+from collections.abc import Set as AbstractSet # PYI025 triggered but fix is not marked as safe