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

Remove F401 fix for __init__ imports by default and allow opt-in to unsafe fix #10365

Merged
merged 9 commits into from Mar 13, 2024
5 changes: 5 additions & 0 deletions crates/ruff/tests/snapshots/integration_test__rule_f401.snap
Expand Up @@ -34,6 +34,11 @@ marking it as unused, as in:
from module import member as member
```

## Fix safety

When `ignore_init_module_imports` is disabled, fixes can remove for unused imports in `__init__` files.
zanieb marked this conversation as resolved.
Show resolved Hide resolved
These fixes are considered unsafe because they can change the public interface.

## Example
```python
import numpy as np # unused import
Expand Down
Expand Up @@ -201,7 +201,7 @@ linter.allowed_confusables = []
linter.builtins = []
linter.dummy_variable_rgx = ^(_+|(_+[a-zA-Z0-9_]*[a-zA-Z0-9]+?))$
linter.external = []
linter.ignore_init_module_imports = false
linter.ignore_init_module_imports = true
linter.logger_objects = []
linter.namespace_packages = []
linter.src = [
Expand Down
13 changes: 13 additions & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Expand Up @@ -223,6 +223,19 @@ mod tests {
Ok(())
}

#[test]
fn init_unused_import_opt_in_to_fix() -> Result<()> {
let diagnostics = test_path(
Path::new("pyflakes/__init__.py"),
&LinterSettings {
ignore_init_module_imports: false,
..LinterSettings::for_rules(vec![Rule::UnusedImport])
},
)?;
assert_messages!(diagnostics);
Ok(())
}

#[test]
fn default_builtins() -> Result<()> {
let diagnostics = test_path(
Expand Down
38 changes: 28 additions & 10 deletions crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs
Expand Up @@ -3,7 +3,7 @@ use std::borrow::Cow;
use anyhow::Result;
use rustc_hash::FxHashMap;

use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation};
use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::{AnyImport, Exceptions, Imported, NodeId, Scope};
use ruff_text_size::{Ranged, TextRange};
Expand Down Expand Up @@ -37,6 +37,11 @@ enum UnusedImportContext {
/// from module import member as member
/// ```
///
/// ## 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.
///
/// ## Example
/// ```python
/// import numpy as np # unused import
Expand Down Expand Up @@ -90,7 +95,7 @@ impl Violation for UnusedImport {
}
Some(UnusedImportContext::Init) => {
format!(
"`{name}` imported but unused; consider adding to `__all__` or using a redundant alias"
"`{name}` imported but unused; consider removing, adding to `__all__`, or using a redundant alias"
Copy link
Member

Choose a reason for hiding this comment

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

I'd usually refer to this as an "explicit reexport" rather than "redundant alias", FWIW, but "redundant alias" is probably clearer for people unfamiliar with the idea

Copy link
Member Author

Choose a reason for hiding this comment

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

"redundant alias" is the term used in Pyright which is usually my reference for this

)
}
None => format!("`{name}` imported but unused"),
Expand Down Expand Up @@ -154,8 +159,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
}
}

let in_init =
checker.settings.ignore_init_module_imports && checker.path().ends_with("__init__.py");
let in_init = checker.path().ends_with("__init__.py");
let fix_init = !checker.settings.ignore_init_module_imports;

// Generate a diagnostic for every import, but share a fix across all imports within the same
// statement (excluding those that are ignored).
Expand All @@ -164,8 +169,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
exceptions.intersects(Exceptions::MODULE_NOT_FOUND_ERROR | Exceptions::IMPORT_ERROR);
let multiple = imports.len() > 1;

let fix = if !in_init && !in_except_handler {
fix_imports(checker, node_id, &imports).ok()
let fix = if (!in_init || fix_init) && !in_except_handler {
fix_imports(checker, node_id, &imports, in_init).ok()
} else {
None
};
Expand Down Expand Up @@ -243,7 +248,12 @@ impl Ranged for ImportBinding<'_> {
}

/// Generate a [`Fix`] to remove unused imports from a statement.
fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> Result<Fix> {
fn fix_imports(
checker: &Checker,
node_id: NodeId,
imports: &[ImportBinding],
in_init: bool,
) -> Result<Fix> {
let statement = checker.semantic().statement(node_id);
let parent = checker.semantic().parent_statement(node_id);

Expand All @@ -261,7 +271,15 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
checker.stylist(),
checker.indexer(),
)?;
Ok(Fix::safe_edit(edit).isolate(Checker::isolation(
checker.semantic().parent_statement_id(node_id),
)))
// It's unsafe to remove things from `__init__.py` because it can break public interfaces
let applicability = if in_init {
Applicability::Unsafe
} else {
Applicability::Safe
};
Ok(
Fix::applicable_edit(edit, applicability).isolate(Checker::isolation(
checker.semantic().parent_statement_id(node_id),
)),
)
}
@@ -1,17 +1,11 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
__init__.py:1:8: F401 [*] `os` imported but unused
__init__.py:1:8: F401 `os` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
1 | import os
| ^^ F401
2 |
3 | print(__path__)
|
= help: Remove unused import: `os`

ℹ Safe fix
1 |-import os
2 1 |
3 2 | print(__path__)
4 3 |
@@ -0,0 +1,17 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
__init__.py:1:8: F401 [*] `os` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
|
1 | import os
| ^^ F401
2 |
3 | print(__path__)
|
= help: Remove unused import: `os`

ℹ Unsafe fix
1 |-import os
2 1 |
3 2 | print(__path__)
4 3 |
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/settings/mod.rs
Expand Up @@ -383,7 +383,7 @@ impl LinterSettings {
dummy_variable_rgx: DUMMY_VARIABLE_RGX.clone(),

external: vec![],
ignore_init_module_imports: false,
ignore_init_module_imports: true,
logger_objects: vec![],
namespace_packages: vec![],

Expand Down
10 changes: 8 additions & 2 deletions crates/ruff_workspace/src/configuration.rs
Expand Up @@ -237,6 +237,7 @@ impl Configuration {
project_root: project_root.to_path_buf(),
},

#[allow(deprecated)]
linter: LinterSettings {
rules: lint.as_rule_table(lint_preview)?,
exclude: FilePatternSet::try_from_iter(lint.exclude.unwrap_or_default())?,
Expand All @@ -253,7 +254,7 @@ impl Configuration {
.dummy_variable_rgx
.unwrap_or_else(|| DUMMY_VARIABLE_RGX.clone()),
external: lint.external.unwrap_or_default(),
ignore_init_module_imports: lint.ignore_init_module_imports.unwrap_or_default(),
ignore_init_module_imports: lint.ignore_init_module_imports.unwrap_or(true),
line_length,
tab_size: self.indent_width.unwrap_or_default(),
namespace_packages: self.namespace_packages.unwrap_or_default(),
Expand Down Expand Up @@ -650,6 +651,10 @@ impl LintConfiguration {
.flatten()
.chain(options.common.extend_unfixable.into_iter().flatten())
.collect();

#[allow(deprecated)]
let ignore_init_module_imports = options.common.ignore_init_module_imports;

Ok(LintConfiguration {
exclude: options.exclude.map(|paths| {
paths
Expand Down Expand Up @@ -692,7 +697,7 @@ impl LintConfiguration {
})
.unwrap_or_default(),
external: options.common.external,
ignore_init_module_imports: options.common.ignore_init_module_imports,
ignore_init_module_imports,
explicit_preview_rules: options.common.explicit_preview_rules,
per_file_ignores: options.common.per_file_ignores.map(|per_file_ignores| {
per_file_ignores
Expand Down Expand Up @@ -1316,6 +1321,7 @@ fn warn_about_deprecated_top_level_lint_options(
used_options.push("extend-unsafe-fixes");
}

#[allow(deprecated)]
if top_level_options.ignore_init_module_imports.is_some() {
used_options.push("ignore-init-module-imports");
}
Expand Down
7 changes: 5 additions & 2 deletions crates/ruff_workspace/src/options.rs
Expand Up @@ -693,11 +693,14 @@ pub struct LintCommonOptions {
/// imports will still be flagged, but with a dedicated message suggesting
/// that the import is either added to the module's `__all__` symbol, or
/// re-exported with a redundant alias (e.g., `import os as os`).
///
/// This option is enabled by default, but you can opt-in to removal of imports
/// via an unsafe fix.
#[option(
default = "false",
default = "true",
value_type = "bool",
example = r#"
ignore-init-module-imports = true
ignore-init-module-imports = false
"#
)]
pub ignore_init_module_imports: Option<bool>,
Expand Down
4 changes: 2 additions & 2 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.