Skip to content

Commit

Permalink
Mark PYI025 fix as safe in more cases for stub files (#10547)
Browse files Browse the repository at this point in the history
## Summary

The fix for PYI025 is currently marked as unsafe in non-global scopes
for both `.py` and `.pyi` files, on the grounds that all global-scope
symbols in Python are implicitly exported from the module, so changing
the name of something in the global scope could break other modules that
import the module we're fixing. Unlike in `.py` files, however, imported
symbols are never implicitly re-exported from stub files. Symbols are
only understood by static analysis tools as being re-exported from stubs
if they are marked as explicit re-exports, which take three forms:

```py
from foo import *  # all symbols from foo are re-exported from the stub

# the "redundant" alias marks it as an explicit re-export
# (note that the alias needs to be identical to the symbol's "actual" name
# in order for it to be a re-export)
from bar import barrr as barrr

# inclusion in __all__ also marks it as an explicit re-export,
# just like in `.py` files
from baz import bazzz
__all__ = ["bazzz"]
```

This is [specc'd in PEP
484](https://peps.python.org/pep-0484/#stub-files), and means that we
can mark the fix for PYI025 as safe in more cases for `.pyi` files.

## Test Plan

`cargo test`. An existing test case goes from being an unsafe fix to a
safe fix in a `.pyi` fixture. I also added a new fixture so we have
coverage of global-scope imports that are marked as re-exports using
"redundant" `from collections.abc import Set as Set` aliases.
  • Loading branch information
AlexWaygood committed Mar 24, 2024
1 parent 7cc40d5 commit 021f0bd
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 20 deletions.
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 021f0bd

Please sign in to comment.