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

Add pyupgrade UP041 to replace TimeoutError aliases #8476

Merged
merged 2 commits into from
Nov 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyupgrade/UP041.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import asyncio, socket
# These should be fixed
try:
pass
except asyncio.TimeoutError:
pass

try:
pass
except socket.timeout:
pass

# Should NOT be in parentheses when replaced

try:
pass
except (asyncio.TimeoutError,):
pass

try:
pass
except (socket.timeout,):
pass

try:
pass
except (asyncio.TimeoutError, socket.timeout,):
pass

# Should be kept in parentheses (because multiple)

try:
pass
except (asyncio.TimeoutError, socket.timeout, KeyError, TimeoutError):
pass

# First should change, second should not

from .mmap import error
try:
pass
except (asyncio.TimeoutError, error):
pass

# These should not change

from foo import error

try:
pass
except (TimeoutError, error):
pass

try:
pass
except:
pass

try:
pass
except TimeoutError:
pass


try:
pass
except (TimeoutError, KeyError):
pass
5 changes: 5 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,11 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::OSErrorAlias) {
pyupgrade::rules::os_error_alias_call(checker, func);
}
if checker.enabled(Rule::TimeoutErrorAlias) {
if checker.settings.target_version >= PythonVersion::Py310 {
pyupgrade::rules::timeout_error_alias_call(checker, func);
}
}
if checker.enabled(Rule::NonPEP604Isinstance) {
if checker.settings.target_version >= PythonVersion::Py310 {
pyupgrade::rules::use_pep604_isinstance(checker, expr, func, args);
Expand Down
12 changes: 12 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
pyupgrade::rules::os_error_alias_raise(checker, item);
}
}
if checker.enabled(Rule::TimeoutErrorAlias) {
if checker.settings.target_version >= PythonVersion::Py310 {
if let Some(item) = exc {
pyupgrade::rules::timeout_error_alias_raise(checker, item);
}
}
}
if checker.enabled(Rule::RaiseVanillaClass) {
if let Some(expr) = exc {
tryceratops::rules::raise_vanilla_class(checker, expr);
Expand Down Expand Up @@ -1304,6 +1311,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::OSErrorAlias) {
pyupgrade::rules::os_error_alias_handlers(checker, handlers);
}
if checker.enabled(Rule::TimeoutErrorAlias) {
if checker.settings.target_version >= PythonVersion::Py310 {
pyupgrade::rules::timeout_error_alias_handlers(checker, handlers);
}
}
if checker.enabled(Rule::PytestAssertInExcept) {
flake8_pytest_style::rules::assert_in_exception_handler(checker, handlers);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pyupgrade, "038") => (RuleGroup::Stable, rules::pyupgrade::rules::NonPEP604Isinstance),
(Pyupgrade, "039") => (RuleGroup::Stable, rules::pyupgrade::rules::UnnecessaryClassParentheses),
(Pyupgrade, "040") => (RuleGroup::Stable, rules::pyupgrade::rules::NonPEP695TypeAlias),
(Pyupgrade, "041") => (RuleGroup::Preview, rules::pyupgrade::rules::TimeoutErrorAlias),

// pydocstyle
(Pydocstyle, "100") => (RuleGroup::Stable, rules::pydocstyle::rules::UndocumentedPublicModule),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pyupgrade/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ mod tests {
#[test_case(Rule::ReplaceStdoutStderr, Path::new("UP022.py"))]
#[test_case(Rule::ReplaceUniversalNewlines, Path::new("UP021.py"))]
#[test_case(Rule::SuperCallWithParameters, Path::new("UP008.py"))]
#[test_case(Rule::TimeoutErrorAlias, Path::new("UP041.py"))]
#[test_case(Rule::TypeOfPrimitive, Path::new("UP003.py"))]
#[test_case(Rule::TypingTextStrAlias, Path::new("UP019.py"))]
#[test_case(Rule::UTF8EncodingDeclaration, Path::new("UP009_0.py"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pyupgrade/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub(crate) use redundant_open_modes::*;
pub(crate) use replace_stdout_stderr::*;
pub(crate) use replace_universal_newlines::*;
pub(crate) use super_call_with_parameters::*;
pub(crate) use timeout_error_alias::*;
pub(crate) use type_of_primitive::*;
pub(crate) use typing_text_str_alias::*;
pub(crate) use unicode_kind_prefix::*;
Expand Down Expand Up @@ -59,6 +60,7 @@ mod redundant_open_modes;
mod replace_stdout_stderr;
mod replace_universal_newlines;
mod super_call_with_parameters;
mod timeout_error_alias;
mod type_of_primitive;
mod typing_text_str_alias;
mod unicode_kind_prefix;
Expand Down
192 changes: 192 additions & 0 deletions crates/ruff_linter/src/rules/pyupgrade/rules/timeout_error_alias.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
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_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::compose_call_path;
use ruff_python_semantic::SemanticModel;

use crate::checkers::ast::Checker;
use crate::settings::types::PythonVersion;

/// ## What it does
/// Checks for uses of exceptions that alias `TimeoutError`.
///
/// ## Why is this bad?
/// `TimeoutError` is the builtin error type used for exceptions when a system
/// function timed out at the system level.
///
/// In Python 3.10, `socket.timeout` was aliased to `TimeoutError`. In Python
/// 3.11, `asyncio.TimeoutError` was aliased to `TimeoutError`.
///
/// These aliases remain in place for compatibility with older versions of
/// Python, but may be removed in future versions.
///
/// Prefer using `TimeoutError` directly, as it is more idiomatic and future-proof.
///
/// ## Example
/// ```python
/// raise asyncio.TimeoutError
/// ```
///
/// Use instead:
/// ```python
/// raise TimeoutError
/// ```
///
/// ## References
/// - [Python documentation: `TimeoutError`](https://docs.python.org/3/library/exceptions.html#TimeoutError)
#[violation]
pub struct TimeoutErrorAlias {
name: Option<String>,
}

impl AlwaysFixableViolation for TimeoutErrorAlias {
#[derive_message_formats]
fn message(&self) -> String {
format!("Replace aliased errors with `TimeoutError`")
}

fn fix_title(&self) -> String {
let TimeoutErrorAlias { name } = self;
match name {
None => "Replace with builtin `TimeoutError`".to_string(),
Some(name) => format!("Replace `{name}` with builtin `TimeoutError`"),
}
}
}

/// 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"])
} else {
matches!(
call_path.as_slice(),
[""] | ["asyncio", "TimeoutError"] | ["socket", "timeout"]
)
}
})
}

/// Return `true` if an [`Expr`] is `TimeoutError`.
fn is_timeout_error(expr: &Expr, semantic: &SemanticModel) -> bool {
semantic
.resolve_call_path(expr)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["", "TimeoutError"]))
}

/// Create a [`Diagnostic`] for a single target, like an [`Expr::Name`].
fn atom_diagnostic(checker: &mut Checker, target: &Expr) {
let mut diagnostic = Diagnostic::new(
TimeoutErrorAlias {
name: compose_call_path(target),
},
target.range(),
);
if checker.semantic().is_builtin("TimeoutError") {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
"TimeoutError".to_string(),
target.range(),
)));
}
checker.diagnostics.push(diagnostic);
}

/// Create a [`Diagnostic`] for a tuple of expressions.
fn tuple_diagnostic(checker: &mut Checker, 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.
let mut remaining: Vec<Expr> = tuple
.elts
.iter()
.filter_map(|elt| {
if aliases.contains(&elt) {
None
} else {
Some(elt.clone())
}
})
.collect();

// If `TimeoutError` itself isn't already in the tuple, add it.
if tuple
.elts
.iter()
.all(|elt| !is_timeout_error(elt, checker.semantic()))
{
let node = ast::ExprName {
id: "TimeoutError".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
};
remaining.insert(0, node.into());
}

let content = if remaining.len() == 1 {
"TimeoutError".to_string()
} else {
let node = ast::ExprTuple {
elts: remaining,
ctx: ExprContext::Load,
range: TextRange::default(),
};
format!("({})", checker.generator().expr(&node.into()))
};

diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
pad(content, tuple.range(), checker.locator()),
tuple.range(),
)));
}
checker.diagnostics.push(diagnostic);
}

/// UP041
pub(crate) fn timeout_error_alias_handlers(checker: &mut Checker, handlers: &[ExceptHandler]) {
for handler in handlers {
let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { type_, .. }) = handler;
let Some(expr) = type_.as_ref() else {
continue;
};
match expr.as_ref() {
Expr::Name(_) | Expr::Attribute(_) => {
if is_alias(expr, checker.semantic(), checker.settings.target_version) {
atom_diagnostic(checker, expr);
}
}
Expr::Tuple(tuple) => {
// List of aliases to replace with `TimeoutError`.
let mut aliases: Vec<&Expr> = vec![];
for elt in &tuple.elts {
if is_alias(elt, checker.semantic(), checker.settings.target_version) {
aliases.push(elt);
}
}
if !aliases.is_empty() {
tuple_diagnostic(checker, tuple, &aliases);
}
}
_ => {}
}
}
}

/// 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);
}
}

/// 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);
}
}
}
Loading