diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP024_4.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP024_4.py new file mode 100644 index 0000000000000..2dc680e1249b1 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pyupgrade/UP024_4.py @@ -0,0 +1,10 @@ +import socket + +from kombu import Connection, exceptions + +try: + conn = Connection(settings.CELERY_BROKER_URL) + conn.ensure_connection(max_retries=2) + conn._close() +except (socket.error, exceptions.OperationalError): + return HttpResponseServerError("cache: cannot connect to broker.") diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 62b3eba6d3377..68732332c564f 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1505,7 +1505,7 @@ where } if self.settings.rules.enabled(&Rule::OSErrorAlias) { if let Some(item) = exc { - pyupgrade::rules::os_error_alias(self, &item); + pyupgrade::rules::os_error_alias_raise(self, item); } } if self.settings.rules.enabled(&Rule::RaiseVanillaClass) { @@ -1757,7 +1757,7 @@ where flake8_bugbear::rules::redundant_tuple_in_exception_handler(self, handlers); } if self.settings.rules.enabled(&Rule::OSErrorAlias) { - pyupgrade::rules::os_error_alias(self, &handlers); + pyupgrade::rules::os_error_alias_handlers(self, handlers); } if self.settings.rules.enabled(&Rule::AssertInExcept) { self.diagnostics.extend( @@ -2484,7 +2484,7 @@ where pyupgrade::rules::replace_stdout_stderr(self, expr, func, keywords); } if self.settings.rules.enabled(&Rule::OSErrorAlias) { - pyupgrade::rules::os_error_alias(self, &expr); + pyupgrade::rules::os_error_alias_call(self, func); } if self.settings.rules.enabled(&Rule::IsinstanceWithTuple) && self.settings.target_version >= PythonVersion::Py310 diff --git a/crates/ruff/src/rules/pyupgrade/mod.rs b/crates/ruff/src/rules/pyupgrade/mod.rs index 9bfcf7ffd8540..20477fdb836f5 100644 --- a/crates/ruff/src/rules/pyupgrade/mod.rs +++ b/crates/ruff/src/rules/pyupgrade/mod.rs @@ -45,6 +45,7 @@ mod tests { #[test_case(Rule::OSErrorAlias, Path::new("UP024_1.py"); "UP024_1")] #[test_case(Rule::OSErrorAlias, Path::new("UP024_2.py"); "UP024_2")] #[test_case(Rule::OSErrorAlias, Path::new("UP024_3.py"); "UP024_3")] + #[test_case(Rule::OSErrorAlias, Path::new("UP024_4.py"); "UP024_4")] #[test_case(Rule::RewriteUnicodeLiteral, Path::new("UP025.py"); "UP025")] #[test_case(Rule::RewriteMockImport, Path::new("UP026.py"); "UP026")] #[test_case(Rule::RewriteListComprehension, Path::new("UP027.py"); "UP027")] diff --git a/crates/ruff/src/rules/pyupgrade/rules/mod.rs b/crates/ruff/src/rules/pyupgrade/rules/mod.rs index 8e18ed986de29..26654fd4a18c9 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/mod.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/mod.rs @@ -16,7 +16,9 @@ pub(crate) use lru_cache_without_parameters::{ }; pub(crate) use native_literals::{native_literals, NativeLiterals}; pub(crate) use open_alias::{open_alias, OpenAlias}; -pub(crate) use os_error_alias::{os_error_alias, OSErrorAlias}; +pub(crate) use os_error_alias::{ + os_error_alias_call, os_error_alias_handlers, os_error_alias_raise, OSErrorAlias, +}; pub(crate) use outdated_version_block::{outdated_version_block, OutdatedVersionBlock}; pub(crate) use printf_string_formatting::{printf_string_formatting, PrintfStringFormatting}; pub(crate) use quoted_annotation::{quoted_annotation, QuotedAnnotation}; diff --git a/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs b/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs index 61310263c531a..d8aa5ccc7483e 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs @@ -1,9 +1,9 @@ -use itertools::Itertools; -use rustpython_parser::ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind}; +use rustpython_parser::ast::{Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::compose_call_path; +use ruff_python_ast::context::Context; +use ruff_python_ast::helpers::{compose_call_path, create_expr, unparse_expr}; use ruff_python_ast::types::Range; use crate::checkers::ast::Checker; @@ -29,218 +29,151 @@ impl AlwaysAutofixableViolation for OSErrorAlias { } } -const ERROR_NAMES: &[&str] = &["EnvironmentError", "IOError", "WindowsError"]; -const ERROR_MODULES: &[&str] = &["mmap", "select", "socket"]; - -fn corrected_name(checker: &Checker, original: &str) -> String { - if ERROR_NAMES.contains(&original) - && checker.ctx.is_builtin(original) - && checker.ctx.is_builtin("OSError") - { - "OSError".to_string() - } else { - original.to_string() - } +const ALIASES: &[(&str, &str)] = &[ + ("", "EnvironmentError"), + ("", "IOError"), + ("", "WindowsError"), + ("mmap", "error"), + ("select", "error"), + ("socket", "error"), +]; + +/// Return `true` if an [`Expr`] is an alias of `OSError`. +fn is_alias(context: &Context, expr: &Expr) -> bool { + context.resolve_call_path(expr).map_or(false, |call_path| { + ALIASES + .iter() + .any(|(module, member)| call_path.as_slice() == [*module, *member]) + }) } -fn get_before_replace(elts: &[Expr]) -> Vec { - elts.iter() - .map(|elt| { - if let ExprKind::Name { id, .. } = &elt.node { - id.to_string() - } else { - String::new() - } - }) - .collect() +/// Return `true` if an [`Expr`] is `OSError`. +fn is_os_error(context: &Context, expr: &Expr) -> bool { + context + .resolve_call_path(expr) + .map_or(false, |call_path| call_path.as_slice() == ["", "OSError"]) } -fn check_module(checker: &Checker, expr: &Expr) -> (Vec, Vec) { - let mut replacements: Vec = vec![]; - let mut before_replace: Vec = vec![]; - if let Some(call_path) = checker.ctx.resolve_call_path(expr) { - for module in ERROR_MODULES.iter() { - if call_path.as_slice() == [module, "error"] { - replacements.push("OSError".to_string()); - before_replace.push(format!("{module}.error")); - break; - } - } +/// Create a [`Diagnostic`] for a single target, like an [`ExprKind::Name`]. +fn atom_diagnostic(checker: &mut Checker, target: &Expr) { + let mut diagnostic = Diagnostic::new( + OSErrorAlias { + name: compose_call_path(target), + }, + Range::from(target), + ); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.amend(Fix::replacement( + "OSError".to_string(), + target.location, + target.end_location.unwrap(), + )); } - (replacements, before_replace) + checker.diagnostics.push(diagnostic); } -fn handle_name_or_attribute( - checker: &Checker, - item: &Expr, - replacements: &mut Vec, - before_replace: &mut Vec, -) { - match &item.node { - ExprKind::Name { id, .. } => { - let (temp_replacements, temp_before_replace) = check_module(checker, item); - replacements.extend(temp_replacements); - before_replace.extend(temp_before_replace); - if replacements.is_empty() { - let new_name = corrected_name(checker, id); - replacements.push(new_name); - before_replace.push(id.to_string()); - } - } - ExprKind::Attribute { .. } => { - let (temp_replacements, temp_before_replace) = check_module(checker, item); - replacements.extend(temp_replacements); - before_replace.extend(temp_before_replace); - } - _ => (), - } -} - -/// Handles one block of an except (use a loop if there are multiple blocks) -fn handle_except_block(checker: &mut Checker, handler: &Excepthandler) { - let ExcepthandlerKind::ExceptHandler { type_, .. } = &handler.node; - let Some(error_handlers) = type_.as_ref() else { - return; - }; - // The first part creates list of all the exceptions being caught, and - // what they should be changed to - let mut replacements: Vec = vec![]; - let mut before_replace: Vec = vec![]; - match &error_handlers.node { - ExprKind::Name { .. } | ExprKind::Attribute { .. } => { - handle_name_or_attribute( - checker, - error_handlers, - &mut replacements, - &mut before_replace, - ); - } - ExprKind::Tuple { elts, .. } => { - before_replace = get_before_replace(elts); - for elt in elts { - match &elt.node { - ExprKind::Name { id, .. } => { - let new_name = corrected_name(checker, id); - replacements.push(new_name); - } - ExprKind::Attribute { .. } => { - let (new_replacements, new_before_replace) = check_module(checker, elt); - replacements.extend(new_replacements); - before_replace.extend(new_before_replace); - } - _ => (), +/// Create a [`Diagnostic`] for a tuple of expressions. +fn tuple_diagnostic(checker: &mut Checker, target: &Expr, aliases: &[&Expr]) { + let mut diagnostic = Diagnostic::new(OSErrorAlias { name: None }, Range::from(target)); + if checker.patch(diagnostic.kind.rule()) { + let ExprKind::Tuple { elts, ..} = &target.node else { + unreachable!("expected ExprKind::Tuple"); + }; + + // Filter out any `OSErrors` aliases. + let mut remaining: Vec = elts + .iter() + .filter_map(|elt| { + if aliases.contains(&elt) { + None + } else { + Some(elt.clone()) } - } + }) + .collect(); + + // If `OSError` itself isn't already in the tuple, add it. + if elts.iter().all(|elt| !is_os_error(&checker.ctx, elt)) { + remaining.insert( + 0, + create_expr(ExprKind::Name { + id: "OSError".to_string(), + ctx: ExprContext::Load, + }), + ); } - _ => return, - } - replacements = replacements - .iter() - .unique() - .map(std::string::ToString::to_string) - .collect(); - before_replace = before_replace - .iter() - .filter(|x| !x.is_empty()) - .map(std::string::ToString::to_string) - .collect(); - - // This part checks if there are differences between what there is and - // what there should be. Where differences, the changes are applied - handle_making_changes(checker, error_handlers, &before_replace, &replacements); -} -fn handle_making_changes( - checker: &mut Checker, - target: &Expr, - before_replace: &[String], - replacements: &[String], -) { - if before_replace != replacements && !replacements.is_empty() { - let mut final_str: String; - if replacements.len() == 1 { - final_str = replacements.get(0).unwrap().to_string(); + if remaining.len() == 1 { + diagnostic.amend(Fix::replacement( + "OSError".to_string(), + target.location, + target.end_location.unwrap(), + )); } else { - final_str = replacements.join(", "); - final_str.insert(0, '('); - final_str.push(')'); - } - let mut diagnostic = Diagnostic::new( - OSErrorAlias { - name: compose_call_path(target), - }, - Range::from(target), - ); - if checker.patch(diagnostic.kind.rule()) { diagnostic.amend(Fix::replacement( - final_str, + format!( + "({})", + unparse_expr( + &create_expr(ExprKind::Tuple { + elts: remaining, + ctx: ExprContext::Load, + }), + checker.stylist, + ) + ), target.location, target.end_location.unwrap(), )); } - checker.diagnostics.push(diagnostic); - } -} - -pub trait OSErrorAliasChecker { - fn check_error(&self, checker: &mut Checker) - where - Self: Sized; -} - -impl OSErrorAliasChecker for &Vec { - fn check_error(&self, checker: &mut Checker) { - for handler in self.iter() { - handle_except_block(checker, handler); - } - } -} - -impl OSErrorAliasChecker for &Box { - fn check_error(&self, checker: &mut Checker) { - let mut replacements: Vec = vec![]; - let mut before_replace: Vec = vec![]; - match &self.node { - ExprKind::Name { .. } | ExprKind::Attribute { .. } => { - handle_name_or_attribute(checker, self, &mut replacements, &mut before_replace); - } - _ => return, - } - handle_making_changes(checker, self, &before_replace, &replacements); } + checker.diagnostics.push(diagnostic); } -impl OSErrorAliasChecker for &Expr { - fn check_error(&self, checker: &mut Checker) { - let mut replacements: Vec = vec![]; - let mut before_replace: Vec = vec![]; - let change_target: &Expr; - match &self.node { +/// UP024 +pub fn os_error_alias_handlers(checker: &mut Checker, handlers: &[Excepthandler]) { + for handler in handlers { + let ExcepthandlerKind::ExceptHandler { type_, .. } = &handler.node; + let Some(expr) = type_.as_ref() else { + continue; + }; + match &expr.node { ExprKind::Name { .. } | ExprKind::Attribute { .. } => { - change_target = self; - handle_name_or_attribute(checker, self, &mut replacements, &mut before_replace); + if is_alias(&checker.ctx, expr) { + atom_diagnostic(checker, expr); + } } - ExprKind::Call { func, .. } => { - change_target = func; - match &func.node { - ExprKind::Name { .. } | ExprKind::Attribute { .. } => { - handle_name_or_attribute( - checker, - func, - &mut replacements, - &mut before_replace, - ); + ExprKind::Tuple { elts, .. } => { + // List of aliases to replace with `OSError`. + let mut aliases: Vec<&Expr> = vec![]; + for elt in elts { + if is_alias(&checker.ctx, elt) { + aliases.push(elt); } - _ => return, + } + if !aliases.is_empty() { + tuple_diagnostic(checker, expr, &aliases); } } - _ => return, + _ => {} } - handle_making_changes(checker, change_target, &before_replace, &replacements); } } /// UP024 -pub fn os_error_alias(checker: &mut Checker, handlers: &U) { - handlers.check_error(checker); +pub fn os_error_alias_call(checker: &mut Checker, func: &Expr) { + if is_alias(&checker.ctx, func) { + atom_diagnostic(checker, func); + } +} + +/// UP024 +pub fn os_error_alias_raise(checker: &mut Checker, expr: &Expr) { + if matches!( + expr.node, + ExprKind::Name { .. } | ExprKind::Attribute { .. } + ) { + if is_alias(&checker.ctx, expr) { + atom_diagnostic(checker, expr); + } + } } diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap index 9134bc3ee4ced..96484c4ea2f5e 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap @@ -214,7 +214,7 @@ expression: diagnostics row: 58 column: 35 fix: - content: "(OSError, KeyError)" + content: "(KeyError, OSError)" location: row: 58 column: 7 diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_4.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_4.py.snap new file mode 100644 index 0000000000000..8783b93e1eacb --- /dev/null +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_4.py.snap @@ -0,0 +1,25 @@ +--- +source: crates/ruff/src/rules/pyupgrade/mod.rs +expression: diagnostics +--- +- kind: + name: OSErrorAlias + body: "Replace aliased errors with `OSError`" + suggestion: "Replace with builtin `OSError`" + fixable: true + location: + row: 9 + column: 7 + end_location: + row: 9 + column: 50 + fix: + content: "(OSError, exceptions.OperationalError)" + location: + row: 9 + column: 7 + end_location: + row: 9 + column: 50 + parent: ~ +