Skip to content

Commit

Permalink
F401 - update documentation and deprecate ignore_init_module_imports (
Browse files Browse the repository at this point in the history
#11436)

## Summary

* Update documentation for F401 following recent PRs
  * #11168
  * #11314
* Deprecate `ignore_init_module_imports`
* Add a deprecation pragma to the option and a "warn user once" message
when the option is used.
* Restore the old behavior for stable (non-preview) mode:
* When `ignore_init_module_imports` is set to `true` (default) there are
no `__init_.py` fixes (but we get nice fix titles!).
* When `ignore_init_module_imports` is set to `false` there are unsafe
`__init__.py` fixes to remove unused imports.
* When preview mode is enabled, it overrides
`ignore_init_module_imports`.
* Fixed a bug in fix titles where `import foo as bar` would recommend
reexporting `bar as bar`. It now says to reexport `foo as foo`. (In this
case we don't issue a fix, fwiw; it was just a fix title bug.)

## Test plan

Added new fixture tests that reuse the existing fixtures for
`__init__.py` files. Each of the three situations listed above has
fixture tests. The F401 "stable" tests cover:

> * When `ignore_init_module_imports` is set to `true` (default) there
are no `__init_.py` fixes (but we get nice fix titles!).

The F401 "deprecated option" tests cover:

> * When `ignore_init_module_imports` is set to `false` there are unsafe
`__init__.py` fixes to remove unused imports.

These complement existing "preview" tests that show the new behavior
which recommends fixes in `__init__.py` according to whether the import
is 1st party and other circumstances (for more on that behavior see:
#11314).
  • Loading branch information
plredmond authored May 21, 2024
1 parent 403f0dc commit 7225732
Show file tree
Hide file tree
Showing 19 changed files with 491 additions and 30 deletions.
23 changes: 21 additions & 2 deletions crates/ruff/tests/snapshots/integration_test__rule_f401.snap
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,29 @@ marking it as unused, as in:
from module import member as member
```

Alternatively, you can use `__all__` to declare a symbol as part of the module's
interface, as in:

```python
# __init__.py
import some_module

__all__ = [ "some_module"]
```

## Fix safety

When `ignore_init_module_imports` is disabled, fixes can remove for unused imports in `__init__` files.
These fixes are considered unsafe because they can change the public interface.
Fixes to remove unused imports are safe, except in `__init__.py` files.

Applying fixes to `__init__.py` files is currently in preview. The fix offered depends on the
type of the unused import. Ruff will suggest a safe fix to export first-party imports with
either a redundant alias or, if already present in the file, an `__all__` entry. If multiple
`__all__` declarations are present, Ruff will not offer a fix. Ruff will suggest an unsafe fix
to remove third-party and standard library imports -- the fix is unsafe because the module's
interface changes.

## Example

```python
import numpy as np # unused import

Expand All @@ -49,12 +66,14 @@ def area(radius):
```

Use instead:

```python
def area(radius):
return 3.14 * radius**2
```

To check the availability of a module, use `importlib.util.find_spec`:

```python
from importlib.util import find_spec

Expand Down
43 changes: 43 additions & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,49 @@ mod tests {
Ok(())
}

#[test_case(Rule::UnusedImport, Path::new("F401_24/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_25__all_nonempty/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_26__all_empty/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_27__all_mistyped/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_28__all_multiple/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_29__all_conditional/__init__.py"))]
fn f401_stable(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"{}_stable_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("pyflakes").join(path).as_path(),
&LinterSettings::for_rule(rule_code),
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::UnusedImport, Path::new("F401_24/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_25__all_nonempty/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_26__all_empty/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_27__all_mistyped/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_28__all_multiple/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_29__all_conditional/__init__.py"))]
fn f401_deprecated_option(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"{}_deprecated_option_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("pyflakes").join(path).as_path(),
&LinterSettings {
ignore_init_module_imports: false,
..LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test]
fn f841_dummy_variable_rgx() -> Result<()> {
let diagnostics = test_path(
Expand Down
80 changes: 54 additions & 26 deletions crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ enum UnusedImportContext {
Init {
first_party: bool,
dunder_all_count: usize,
ignore_init_module_imports: bool,
},
}

Expand All @@ -46,12 +47,29 @@ enum UnusedImportContext {
/// from module import member as member
/// ```
///
/// Alternatively, you can use `__all__` to declare a symbol as part of the module's
/// interface, as in:
///
/// ```python
/// # __init__.py
/// import some_module
///
/// __all__ = [ "some_module"]
/// ```
///
/// ## Fix safety
///
/// When `ignore_init_module_imports` is disabled, fixes can remove for unused imports in `__init__` files.
/// These fixes are considered unsafe because they can change the public interface.
/// Fixes to remove unused imports are safe, except in `__init__.py` files.
///
/// Applying fixes to `__init__.py` files is currently in preview. The fix offered depends on the
/// type of the unused import. Ruff will suggest a safe fix to export first-party imports with
/// either a redundant alias or, if already present in the file, an `__all__` entry. If multiple
/// `__all__` declarations are present, Ruff will not offer a fix. Ruff will suggest an unsafe fix
/// to remove third-party and standard library imports -- the fix is unsafe because the module's
/// interface changes.
///
/// ## Example
///
/// ```python
/// import numpy as np # unused import
///
Expand All @@ -61,12 +79,14 @@ enum UnusedImportContext {
/// ```
///
/// Use instead:
///
/// ```python
/// def area(radius):
/// return 3.14 * radius**2
/// ```
///
/// To check the availability of a module, use `importlib.util.find_spec`:
///
/// ```python
/// from importlib.util import find_spec
///
Expand All @@ -87,6 +107,8 @@ enum UnusedImportContext {
pub struct UnusedImport {
/// Qualified name of the import
name: String,
/// Unqualified name of the import
module: String,
/// Name of the import binding
binding: String,
context: Option<UnusedImportContext>,
Expand Down Expand Up @@ -117,6 +139,7 @@ impl Violation for UnusedImport {
fn fix_title(&self) -> Option<String> {
let UnusedImport {
name,
module,
binding,
multiple,
..
Expand All @@ -125,14 +148,14 @@ impl Violation for UnusedImport {
Some(UnusedImportContext::Init {
first_party: true,
dunder_all_count: 1,
ignore_init_module_imports: true,
}) => Some(format!("Add unused import `{binding}` to __all__")),

Some(UnusedImportContext::Init {
first_party: true,
dunder_all_count: 0,
}) => Some(format!(
"Use an explicit re-export: `{binding} as {binding}`"
)),
ignore_init_module_imports: true,
}) => Some(format!("Use an explicit re-export: `{module} as {module}`")),

_ => Some(if *multiple {
"Remove unused import".to_string()
Expand Down Expand Up @@ -244,7 +267,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
}

let in_init = checker.path().ends_with("__init__.py");
let fix_init = checker.settings.preview.is_enabled();
let fix_init = !checker.settings.ignore_init_module_imports;
let preview_mode = checker.settings.preview.is_enabled();
let dunder_all_exprs = find_dunder_all_exprs(checker.semantic());

// Generate a diagnostic for every import, but share fixes across all imports within the same
Expand Down Expand Up @@ -275,6 +299,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
checker,
),
dunder_all_count: dunder_all_exprs.len(),
ignore_init_module_imports: !fix_init,
})
} else {
None
Expand All @@ -288,30 +313,31 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
first_party: true,
..
})
)
) && preview_mode
});

// generate fixes that are shared across bindings in the statement
let (fix_remove, fix_reexport) = if (!in_init || fix_init) && !in_except_handler {
(
fix_by_removing_imports(
checker,
import_statement,
to_remove.iter().map(|(binding, _)| binding),
in_init,
)
.ok(),
fix_by_reexporting(
checker,
import_statement,
&to_reexport.iter().map(|(b, _)| b).collect::<Vec<_>>(),
&dunder_all_exprs,
let (fix_remove, fix_reexport) =
if (!in_init || fix_init || preview_mode) && !in_except_handler {
(
fix_by_removing_imports(
checker,
import_statement,
to_remove.iter().map(|(binding, _)| binding),
in_init,
)
.ok(),
fix_by_reexporting(
checker,
import_statement,
&to_reexport.iter().map(|(b, _)| b).collect::<Vec<_>>(),
&dunder_all_exprs,
)
.ok(),
)
.ok(),
)
} else {
(None, None)
};
} else {
(None, None)
};

for ((binding, context), fix) in iter::Iterator::chain(
iter::zip(to_remove, iter::repeat(fix_remove)),
Expand All @@ -320,6 +346,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
let mut diagnostic = Diagnostic::new(
UnusedImport {
name: binding.import.qualified_name().to_string(),
module: binding.import.member_name().to_string(),
binding: binding.name.to_string(),
context,
multiple,
Expand All @@ -344,6 +371,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
let mut diagnostic = Diagnostic::new(
UnusedImport {
name: binding.import.qualified_name().to_string(),
module: binding.import.member_name().to_string(),
binding: binding.name.to_string(),
context: None,
multiple: false,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
__init__.py:19:8: F401 [*] `sys` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
19 | import sys # F401: remove unused
| ^^^ F401
|
= help: Remove unused import: `sys`

Unsafe fix
16 16 | import argparse as argparse # Ok: is redundant alias
17 17 |
18 18 |
19 |-import sys # F401: remove unused
20 19 |
21 20 |
22 21 | # first-party

__init__.py:33:15: F401 [*] `.unused` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
33 | from . import unused # F401: change to redundant alias
| ^^^^^^ F401
|
= help: Remove unused import: `.unused`

Unsafe fix
30 30 | from . import aliased as aliased # Ok: is redundant alias
31 31 |
32 32 |
33 |-from . import unused # F401: change to redundant alias
34 33 |
35 34 |
36 35 | from . import renamed as bees # F401: no fix

__init__.py:36:26: F401 [*] `.renamed` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
36 | from . import renamed as bees # F401: no fix
| ^^^^ F401
|
= help: Remove unused import: `.renamed`

Unsafe fix
33 33 | from . import unused # F401: change to redundant alias
34 34 |
35 35 |
36 |-from . import renamed as bees # F401: no fix
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
__init__.py:19:8: F401 [*] `sys` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
19 | import sys # F401: remove unused
| ^^^ F401
|
= help: Remove unused import: `sys`

Unsafe fix
16 16 | import argparse # Ok: is exported in __all__
17 17 |
18 18 |
19 |-import sys # F401: remove unused
20 19 |
21 20 |
22 21 | # first-party

__init__.py:36:15: F401 [*] `.unused` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
36 | from . import unused # F401: add to __all__
| ^^^^^^ F401
|
= help: Remove unused import: `.unused`

Unsafe fix
33 33 | from . import exported # Ok: is exported in __all__
34 34 |
35 35 |
36 |-from . import unused # F401: add to __all__
37 36 |
38 37 |
39 38 | from . import renamed as bees # F401: add to __all__

__init__.py:39:26: F401 [*] `.renamed` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
39 | from . import renamed as bees # F401: add to __all__
| ^^^^ F401
|
= help: Remove unused import: `.renamed`

Unsafe fix
36 36 | from . import unused # F401: add to __all__
37 37 |
38 38 |
39 |-from . import renamed as bees # F401: add to __all__
40 39 |
41 40 |
42 41 | __all__ = ["argparse", "exported"]
Loading

0 comments on commit 7225732

Please sign in to comment.