From 7b3ee2daff9681352ba66ed0536072efc5a4ad69 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 13 Mar 2024 12:58:25 -0500 Subject: [PATCH] Remove `F401` fix for `__init__` imports by default and allow opt-in to unsafe fix (#10365) Re-implementation of https://github.com/astral-sh/ruff/pull/5845 but instead of deprecating the option I toggle the default. Now users can _opt-in_ via the setting which will give them an unsafe fix to remove the import. Otherwise, we raise violations but do not offer a fix. The setting is a bit of a misnomer in either case, maybe we'll want to remove it still someday. As discussed there, I think the safe fix should be to import it as an alias. I'm not sure. We need support for offering multiple fixes for ideal behavior though? I think we should remove the fix entirely and consider it separately. Closes https://github.com/astral-sh/ruff/issues/5697 Supersedes https://github.com/astral-sh/ruff/pull/5845 --------- Co-authored-by: Alex Waygood --- .../integration_test__rule_f401.snap | 5 +++ ...ow_settings__display_default_settings.snap | 2 +- crates/ruff_linter/src/rules/pyflakes/mod.rs | 13 +++++++ .../src/rules/pyflakes/rules/unused_import.rs | 38 ++++++++++++++----- ..._linter__rules__pyflakes__tests__init.snap | 8 +--- ...sts__init_unused_import_opt_in_to_fix.snap | 17 +++++++++ crates/ruff_linter/src/settings/mod.rs | 2 +- crates/ruff_workspace/src/configuration.rs | 10 ++++- crates/ruff_workspace/src/options.rs | 7 +++- ruff.schema.json | 4 +- 10 files changed, 81 insertions(+), 25 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init_unused_import_opt_in_to_fix.snap diff --git a/crates/ruff/tests/snapshots/integration_test__rule_f401.snap b/crates/ruff/tests/snapshots/integration_test__rule_f401.snap index cad44c30080e6..2c69bfa92bcd0 100644 --- a/crates/ruff/tests/snapshots/integration_test__rule_f401.snap +++ b/crates/ruff/tests/snapshots/integration_test__rule_f401.snap @@ -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. +These fixes are considered unsafe because they can change the public interface. + ## Example ```python import numpy as np # unused import diff --git a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap index f04f78f8a7deb..0498d30f847c5 100644 --- a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap +++ b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap @@ -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 = [ diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index e913278245c49..aa08d9d32de65 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -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( diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs index 1499ad779128e..df446ff608e0c 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs @@ -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}; @@ -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 @@ -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" ) } None => format!("`{name}` imported but unused"), @@ -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). @@ -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 }; @@ -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 { +fn fix_imports( + checker: &Checker, + node_id: NodeId, + imports: &[ImportBinding], + in_init: bool, +) -> Result { let statement = checker.semantic().statement(node_id); let parent = checker.semantic().parent_statement(node_id); @@ -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), + )), + ) } diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init.snap index 7da8280b04883..3792cb39ddeaa 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init.snap @@ -1,7 +1,7 @@ --- 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 @@ -9,9 +9,3 @@ __init__.py:1:8: F401 [*] `os` imported but unused 3 | print(__path__) | = help: Remove unused import: `os` - -ℹ Safe fix -1 |-import os -2 1 | -3 2 | print(__path__) -4 3 | diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init_unused_import_opt_in_to_fix.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init_unused_import_opt_in_to_fix.snap new file mode 100644 index 0000000000000..f141588829c77 --- /dev/null +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__init_unused_import_opt_in_to_fix.snap @@ -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 | diff --git a/crates/ruff_linter/src/settings/mod.rs b/crates/ruff_linter/src/settings/mod.rs index cdedabc6c0cd4..99d5a481e8569 100644 --- a/crates/ruff_linter/src/settings/mod.rs +++ b/crates/ruff_linter/src/settings/mod.rs @@ -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![], diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index e6dfea2f37d83..5414515e3d795 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -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())?, @@ -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(), @@ -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 @@ -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 @@ -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"); } diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index d6679a4c920a2..da5445c692a0c 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -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, diff --git a/ruff.schema.json b/ruff.schema.json index d0f0f9a6394e0..7b65b8f158596 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -424,7 +424,7 @@ } }, "ignore-init-module-imports": { - "description": "Avoid automatically removing unused imports in `__init__.py` files. Such 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`).", + "description": "Avoid automatically removing unused imports in `__init__.py` files. Such 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`).\n\nThis option is enabled by default, but you can opt-in to removal of imports via an unsafe fix.", "deprecated": true, "type": [ "boolean", @@ -2079,7 +2079,7 @@ } }, "ignore-init-module-imports": { - "description": "Avoid automatically removing unused imports in `__init__.py` files. Such 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`).", + "description": "Avoid automatically removing unused imports in `__init__.py` files. Such 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`).\n\nThis option is enabled by default, but you can opt-in to removal of imports via an unsafe fix.", "type": [ "boolean", "null"