diff --git a/crates/ruff_diagnostics/src/fix.rs b/crates/ruff_diagnostics/src/fix.rs index 751258508f7183..81bab65cf12a78 100644 --- a/crates/ruff_diagnostics/src/fix.rs +++ b/crates/ruff_diagnostics/src/fix.rs @@ -23,6 +23,23 @@ pub enum Applicability { Safe, } +impl Applicability { + /// Returns the "less safe" of the two applicabilities. + /// + /// This is useful in contexts where there are multiple transformations in + /// a single fix, and the fix should be labeled as the least safe of the + /// bunch. + #[must_use] + pub fn degrade(self, other: Applicability) -> Applicability { + match (self, other) { + (Applicability::DisplayOnly, _) => self, + (Applicability::Unsafe, Applicability::DisplayOnly) => other, + (Applicability::Unsafe, _) => self, + (Applicability::Safe, _) => other, + } + } +} + /// Indicates the level of isolation required to apply a fix. #[derive(Default, Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] diff --git a/crates/ruff_linter/src/rules/pyupgrade/mod.rs b/crates/ruff_linter/src/rules/pyupgrade/mod.rs index 987e82608e03b5..194bcef73fa3f9 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/mod.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/mod.rs @@ -96,6 +96,19 @@ mod tests { Ok(()) } + #[test] + fn async_timeout_error_alias_not_applied_py310() -> Result<()> { + let diagnostics = test_path( + Path::new("pyupgrade/UP041.py"), + &settings::LinterSettings { + target_version: PythonVersion::Py310, + ..settings::LinterSettings::for_rule(Rule::TimeoutErrorAlias) + }, + )?; + assert_messages!(diagnostics); + Ok(()) + } + #[test] fn non_pep695_type_alias_not_applied_py311() -> Result<()> { let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/timeout_error_alias.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/timeout_error_alias.rs index 1bb999f96d6516..938cddb77846b1 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/timeout_error_alias.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/timeout_error_alias.rs @@ -2,7 +2,7 @@ use ruff_python_ast::{self as ast, ExceptHandler, Expr, ExprContext}; use ruff_text_size::{Ranged, TextRange}; use crate::fix::edits::pad; -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::call_path::compose_call_path; use ruff_python_semantic::SemanticModel; @@ -57,18 +57,34 @@ impl AlwaysFixableViolation for TimeoutErrorAlias { } } -/// Return `true` if an [`Expr`] is an alias of `TimeoutError`. -fn is_alias(expr: &Expr, semantic: &SemanticModel, target_version: PythonVersion) -> bool { - semantic.resolve_call_path(expr).is_some_and(|call_path| { - if target_version >= PythonVersion::Py311 { - matches!(call_path.as_slice(), [""] | ["asyncio", "TimeoutError"]) +/// Return an appropriate `Applicability` if an [`Expr`] is an alias of +/// `TimeoutError`. +fn alias_applicability(checker: &mut Checker, expr: &Expr) -> Option { + // N.B. This lint is only invoked for Python 3.10+. We assume + // as much here since otherwise socket.timeout would be an unsafe + // fix in Python <3.10. We add an assert to make this assumption + // explicit. + assert!( + checker.settings.target_version >= PythonVersion::Py310, + "lint should only be used for Python 3.10+", + ); + + let call_path = checker.semantic().resolve_call_path(expr)?; + if matches!(call_path.as_slice(), [""] | ["socket", "timeout"]) { + // Since we assume 3.10+ here and socket.timeout is an alias + // of TimeoutError in 3.10+, it's always safe to change it to + // TimeoutError. + return Some(Applicability::Safe); + } + if matches!(call_path.as_slice(), [""] | ["asyncio", "TimeoutError"]) { + // asyncio.TimeoutError only became an alias of TimeoutError in 3.11+. + return if checker.settings.target_version >= PythonVersion::Py311 { + Some(Applicability::Safe) } else { - matches!( - call_path.as_slice(), - [""] | ["asyncio", "TimeoutError"] | ["socket", "timeout"] - ) - } - }) + Some(Applicability::Unsafe) + }; + } + None } /// Return `true` if an [`Expr`] is `TimeoutError`. @@ -79,7 +95,7 @@ fn is_timeout_error(expr: &Expr, semantic: &SemanticModel) -> bool { } /// Create a [`Diagnostic`] for a single target, like an [`Expr::Name`]. -fn atom_diagnostic(checker: &mut Checker, target: &Expr) { +fn atom_diagnostic(checker: &mut Checker, app: Applicability, target: &Expr) { let mut diagnostic = Diagnostic::new( TimeoutErrorAlias { name: compose_call_path(target), @@ -87,16 +103,19 @@ fn atom_diagnostic(checker: &mut Checker, target: &Expr) { target.range(), ); if checker.semantic().is_builtin("TimeoutError") { - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - "TimeoutError".to_string(), - target.range(), - ))); + let edit = Edit::range_replacement("TimeoutError".to_string(), target.range()); + diagnostic.set_fix(Fix::applicable_edit(edit, app)); } checker.diagnostics.push(diagnostic); } /// Create a [`Diagnostic`] for a tuple of expressions. -fn tuple_diagnostic(checker: &mut Checker, tuple: &ast::ExprTuple, aliases: &[&Expr]) { +fn tuple_diagnostic( + checker: &mut Checker, + app: Applicability, + tuple: &ast::ExprTuple, + aliases: &[&Expr], +) { let mut diagnostic = Diagnostic::new(TimeoutErrorAlias { name: None }, tuple.range()); if checker.semantic().is_builtin("TimeoutError") { // Filter out any `TimeoutErrors` aliases. @@ -137,10 +156,11 @@ fn tuple_diagnostic(checker: &mut Checker, tuple: &ast::ExprTuple, aliases: &[&E format!("({})", checker.generator().expr(&node.into())) }; - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + let edit = Edit::range_replacement( pad(content, tuple.range(), checker.locator()), tuple.range(), - ))); + ); + diagnostic.set_fix(Fix::applicable_edit(edit, app)); } checker.diagnostics.push(diagnostic); } @@ -154,20 +174,24 @@ pub(crate) fn timeout_error_alias_handlers(checker: &mut Checker, handlers: &[Ex }; match expr.as_ref() { Expr::Name(_) | Expr::Attribute(_) => { - if is_alias(expr, checker.semantic(), checker.settings.target_version) { - atom_diagnostic(checker, expr); - } + let Some(app) = alias_applicability(checker, expr) else { + continue; + }; + atom_diagnostic(checker, app, expr); } Expr::Tuple(tuple) => { // List of aliases to replace with `TimeoutError`. let mut aliases: Vec<&Expr> = vec![]; + let mut app = Applicability::Safe; for elt in &tuple.elts { - if is_alias(elt, checker.semantic(), checker.settings.target_version) { - aliases.push(elt); - } + let Some(eltapp) = alias_applicability(checker, elt) else { + continue; + }; + aliases.push(elt); + app = app.degrade(eltapp); } if !aliases.is_empty() { - tuple_diagnostic(checker, tuple, &aliases); + tuple_diagnostic(checker, app, tuple, &aliases); } } _ => {} @@ -177,16 +201,19 @@ pub(crate) fn timeout_error_alias_handlers(checker: &mut Checker, handlers: &[Ex /// UP041 pub(crate) fn timeout_error_alias_call(checker: &mut Checker, func: &Expr) { - if is_alias(func, checker.semantic(), checker.settings.target_version) { - atom_diagnostic(checker, func); - } + let Some(app) = alias_applicability(checker, func) else { + return; + }; + atom_diagnostic(checker, app, func); } /// UP041 pub(crate) fn timeout_error_alias_raise(checker: &mut Checker, expr: &Expr) { - if matches!(expr, Expr::Name(_) | Expr::Attribute(_)) { - if is_alias(expr, checker.semantic(), checker.settings.target_version) { - atom_diagnostic(checker, expr); - } + if !matches!(expr, Expr::Name(_) | Expr::Attribute(_)) { + return; } + let Some(app) = alias_applicability(checker, expr) else { + return; + }; + atom_diagnostic(checker, app, expr); } diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP041.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP041.py.snap index 8b0c7b90e2298f..68f66dc6de38fd 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP041.py.snap +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP041.py.snap @@ -21,6 +21,26 @@ UP041.py:5:8: UP041 [*] Replace aliased errors with `TimeoutError` 7 7 | 8 8 | try: +UP041.py:10:8: UP041 [*] Replace aliased errors with `TimeoutError` + | + 8 | try: + 9 | pass +10 | except socket.timeout: + | ^^^^^^^^^^^^^^ UP041 +11 | pass + | + = help: Replace `socket.timeout` with builtin `TimeoutError` + +ℹ Safe fix +7 7 | +8 8 | try: +9 9 | pass +10 |-except socket.timeout: + 10 |+except TimeoutError: +11 11 | pass +12 12 | +13 13 | # Should NOT be in parentheses when replaced + UP041.py:17:8: UP041 [*] Replace aliased errors with `TimeoutError` | 15 | try: @@ -41,6 +61,26 @@ UP041.py:17:8: UP041 [*] Replace aliased errors with `TimeoutError` 19 19 | 20 20 | try: +UP041.py:22:8: UP041 [*] Replace aliased errors with `TimeoutError` + | +20 | try: +21 | pass +22 | except (socket.timeout,): + | ^^^^^^^^^^^^^^^^^ UP041 +23 | pass + | + = help: Replace with builtin `TimeoutError` + +ℹ Safe fix +19 19 | +20 20 | try: +21 21 | pass +22 |-except (socket.timeout,): + 22 |+except TimeoutError: +23 23 | pass +24 24 | +25 25 | try: + UP041.py:27:8: UP041 [*] Replace aliased errors with `TimeoutError` | 25 | try: @@ -56,7 +96,7 @@ UP041.py:27:8: UP041 [*] Replace aliased errors with `TimeoutError` 25 25 | try: 26 26 | pass 27 |-except (asyncio.TimeoutError, socket.timeout,): - 27 |+except (TimeoutError, socket.timeout): + 27 |+except TimeoutError: 28 28 | pass 29 29 | 30 30 | # Should be kept in parentheses (because multiple) @@ -76,7 +116,7 @@ UP041.py:34:8: UP041 [*] Replace aliased errors with `TimeoutError` 32 32 | try: 33 33 | pass 34 |-except (asyncio.TimeoutError, socket.timeout, KeyError, TimeoutError): - 34 |+except (socket.timeout, KeyError, TimeoutError): + 34 |+except (KeyError, TimeoutError): 35 35 | pass 36 36 | 37 37 | # First should change, second should not diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__async_timeout_error_alias_not_applied_py310.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__async_timeout_error_alias_not_applied_py310.snap new file mode 100644 index 00000000000000..14d06aeb3df00e --- /dev/null +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__async_timeout_error_alias_not_applied_py310.snap @@ -0,0 +1,144 @@ +--- +source: crates/ruff_linter/src/rules/pyupgrade/mod.rs +--- +UP041.py:5:8: UP041 [*] Replace aliased errors with `TimeoutError` + | +3 | try: +4 | pass +5 | except asyncio.TimeoutError: + | ^^^^^^^^^^^^^^^^^^^^ UP041 +6 | pass + | + = help: Replace `asyncio.TimeoutError` with builtin `TimeoutError` + +ℹ Unsafe fix +2 2 | # These should be fixed +3 3 | try: +4 4 | pass +5 |-except asyncio.TimeoutError: + 5 |+except TimeoutError: +6 6 | pass +7 7 | +8 8 | try: + +UP041.py:10:8: UP041 [*] Replace aliased errors with `TimeoutError` + | + 8 | try: + 9 | pass +10 | except socket.timeout: + | ^^^^^^^^^^^^^^ UP041 +11 | pass + | + = help: Replace `socket.timeout` with builtin `TimeoutError` + +ℹ Safe fix +7 7 | +8 8 | try: +9 9 | pass +10 |-except socket.timeout: + 10 |+except TimeoutError: +11 11 | pass +12 12 | +13 13 | # Should NOT be in parentheses when replaced + +UP041.py:17:8: UP041 [*] Replace aliased errors with `TimeoutError` + | +15 | try: +16 | pass +17 | except (asyncio.TimeoutError,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP041 +18 | pass + | + = help: Replace with builtin `TimeoutError` + +ℹ Unsafe fix +14 14 | +15 15 | try: +16 16 | pass +17 |-except (asyncio.TimeoutError,): + 17 |+except TimeoutError: +18 18 | pass +19 19 | +20 20 | try: + +UP041.py:22:8: UP041 [*] Replace aliased errors with `TimeoutError` + | +20 | try: +21 | pass +22 | except (socket.timeout,): + | ^^^^^^^^^^^^^^^^^ UP041 +23 | pass + | + = help: Replace with builtin `TimeoutError` + +ℹ Safe fix +19 19 | +20 20 | try: +21 21 | pass +22 |-except (socket.timeout,): + 22 |+except TimeoutError: +23 23 | pass +24 24 | +25 25 | try: + +UP041.py:27:8: UP041 [*] Replace aliased errors with `TimeoutError` + | +25 | try: +26 | pass +27 | except (asyncio.TimeoutError, socket.timeout,): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP041 +28 | pass + | + = help: Replace with builtin `TimeoutError` + +ℹ Unsafe fix +24 24 | +25 25 | try: +26 26 | pass +27 |-except (asyncio.TimeoutError, socket.timeout,): + 27 |+except TimeoutError: +28 28 | pass +29 29 | +30 30 | # Should be kept in parentheses (because multiple) + +UP041.py:34:8: UP041 [*] Replace aliased errors with `TimeoutError` + | +32 | try: +33 | pass +34 | except (asyncio.TimeoutError, socket.timeout, KeyError, TimeoutError): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP041 +35 | pass + | + = help: Replace with builtin `TimeoutError` + +ℹ Unsafe fix +31 31 | +32 32 | try: +33 33 | pass +34 |-except (asyncio.TimeoutError, socket.timeout, KeyError, TimeoutError): + 34 |+except (KeyError, TimeoutError): +35 35 | pass +36 36 | +37 37 | # First should change, second should not + +UP041.py:42:8: UP041 [*] Replace aliased errors with `TimeoutError` + | +40 | try: +41 | pass +42 | except (asyncio.TimeoutError, error): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP041 +43 | pass + | + = help: Replace with builtin `TimeoutError` + +ℹ Unsafe fix +39 39 | from .mmap import error +40 40 | try: +41 41 | pass +42 |-except (asyncio.TimeoutError, error): + 42 |+except (TimeoutError, error): +43 43 | pass +44 44 | +45 45 | # These should not change + +