From b8c4736d2e755d43e9544f2045a21313b5e41902 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Tue, 12 Mar 2024 13:01:04 -0500 Subject: [PATCH] Allow an unsafe fix --- .../integration_test__rule_f401.snap | 5 +++ .../src/rules/pyflakes/rules/unused_import.rs | 34 ++++++++++++++----- ..._linter__rules__pyflakes__tests__init.snap | 8 ++++- ...sts__init_unused_import_opt_in_to_fix.snap | 8 ++++- ruff.schema.json | 4 +-- 5 files changed, 47 insertions(+), 12 deletions(-) diff --git a/crates/ruff/tests/snapshots/integration_test__rule_f401.snap b/crates/ruff/tests/snapshots/integration_test__rule_f401.snap index cad44c30080e6e..2c69bfa92bcd0c 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_linter/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs index a65d8bfe423648..dc2547424d67d7 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 @@ -155,7 +160,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut } let in_init = checker.path().ends_with("__init__.py"); - let fix_init = in_init && !checker.settings.ignore_init_module_imports; + 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 !fix_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 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 de4eaf9d3f490d..2805460c7ab279 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; consider adding to `__all__` or using a redundant alias +__init__.py:1:8: F401 [*] `os` imported but unused; consider adding to `__all__` or using a redundant alias | 1 | import os | ^^ F401 @@ -9,3 +9,9 @@ __init__.py:1:8: F401 `os` imported but unused; consider adding to `__all__` or 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/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 index de4eaf9d3f490d..2805460c7ab279 100644 --- 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 @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pyflakes/mod.rs --- -__init__.py:1:8: F401 `os` imported but unused; consider adding to `__all__` or using a redundant alias +__init__.py:1:8: F401 [*] `os` imported but unused; consider adding to `__all__` or using a redundant alias | 1 | import os | ^^ F401 @@ -9,3 +9,9 @@ __init__.py:1:8: F401 `os` imported but unused; consider adding to `__all__` or 3 | print(__path__) | = help: Remove unused import: `os` + +ℹ Unsafe fix +1 |-import os +2 1 | +3 2 | print(__path__) +4 3 | diff --git a/ruff.schema.json b/ruff.schema.json index 24a93bebb4b140..f6b56638bc5a17 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`).\n\nThis option is enabled by default, but you can opt-in to removal of 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`).\n\nThis option is enabled by default, but you can opt-in to removal of imports via an unsafe fix.", "deprecated": true, "type": [ "boolean", @@ -2076,7 +2076,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`).\n\nThis option is enabled by default, but you can opt-in to removal of 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`).\n\nThis option is enabled by default, but you can opt-in to removal of imports via an unsafe fix.", "type": [ "boolean", "null"