Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

F401 - update documentation and deprecate ignore_init_module_imports #11436

Merged
merged 20 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e78fee6
docs for updated F401 behavior #11168,#11314
plredmond May 15, 2024
c9ac09b
cargo insta review -- accept doc changes
plredmond May 15, 2024
fb1ac97
wording improvements in after zanie's comments
plredmond May 15, 2024
7706561
cargo insta review -- accept documentation change
plredmond May 15, 2024
36b4331
add deprecation warning for option lint.ignore_init_module_imports in…
plredmond May 20, 2024
63a9676
add module member name field to UnusedImport violation; redundant-ali…
plredmond May 20, 2024
f656ca4
cargo insta review -- accept change in diff for incorrect fix title
plredmond May 20, 2024
1bfa7b1
restore deprecated behavior for f401 when linter.ignore_init_module_i…
plredmond May 20, 2024
4f0b79e
tests for f401 in stable w/o deprecated option
plredmond May 20, 2024
3202de8
tests for f401 in stable w/ deprecated option: emit unsafe fixes to r…
plredmond May 20, 2024
97d1526
add deprecation message for `ignore-init-module-imports` in settings …
plredmond May 20, 2024
8d3ffbd
cargo insta review -- accept changed documentation
plredmond May 20, 2024
249d848
cargo dev generate-all
plredmond May 20, 2024
b6871c9
tweak boolean condition
plredmond May 20, 2024
715da2e
correct documentation formatting
plredmond May 20, 2024
21fe25e
cargo insta review -- accept documentation change
plredmond May 20, 2024
26f80c1
emit the deprecation warning if the option is set, regardless of valu…
plredmond May 21, 2024
2be489f
improve wording of deprecation warning by using zanie's suggestion
plredmond May 21, 2024
943ee25
list only the option, not information about it, in the docstring of a…
plredmond May 21, 2024
6028415
cargo insta review -- accept change in documentation
plredmond May 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Comment on lines +49 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good that ruff has different handling of first vs third party imports, but changing behavior based on the name of the module (containing the import) I find unintuitive and I argue is unpythonic.

By that, I mean: if I have mod/__init__.py, is the fix any more or less safe than if I instead named it mod.py? No — the __init__.py is just an implementation detail and in either case, the module's interface changes with any import changes.

It's not a universally-accepted convention that __init__.py should store your entire public API.
For example, it's hinders import times for users of a large package, if the entire public API is placed in __init__.py, because then any import of a submodule will trigger an import __init__.py and thus an import of every module, if everything is reexported.

At most, we have the typing docs (Typing documentation: interface conventions) because there's nothing I can find on python.org that discusses this. But given the typing docs guidance, I would find it slightly easier to teach that:

  • third party imports: always safe to remove
  • first party imports: unsafe to remove if contained within the same package (similar to relative import convention from typing spec). Safe otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for mono-repos, there may be dozens or hundreds of first party top-level packages. Therefore this rule should base its decisions on whether an import is intra-package (e.g. a relative import) rather than first vs third party packages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback @Hnasar — would you mind opening a new issue to discuss this preview behavior? I don't think this pull request is a great place for it since it's just updating the docs.


## 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,
Copy link
Contributor Author

@plredmond plredmond May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field is required to make fix titles follow the "old" behavior in __init__.py files. When we finally remove the option ignore_init_module_imports then this field ignore_init_module_imports can be deleted too.

},
}

Expand All @@ -46,12 +47,29 @@ enum UnusedImportContext {
/// from module import member as member
plredmond marked this conversation as resolved.
Show resolved Hide resolved
/// ```
///
/// 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();
Copy link
Contributor Author

@plredmond plredmond May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three lines look fishy because I'd originally replaced this

    let fix_init = !checker.settings.ignore_init_module_imports;

with

    let fix_init = checker.settings.preview.is_enabled();

which was perhaps lazy. In this PR I'm restoring the old behavior, and so gave checker.settings.preview.is_enabled(); its own name.

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 {
Copy link
Contributor Author

@plredmond plredmond May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section looks like a large change but isn't. I only added preview_mode to the disjunction. Turn on -w to see the diff w/o whitespace changes.

(
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
Loading