Skip to content

Commit

Permalink
Add pyupgrade UP041 to replace TimeoutError aliases (#8476)
Browse files Browse the repository at this point in the history
## Summary

Add UP041 to replace `TimeoutError` aliases:

* Python 3.10+: `socket.timeout`
* Python 3.11+: `asyncio.TimeoutError`

Re:

* https://github.com/asottile/pyupgrade#timeouterror-aliases
*
https://docs.python.org/3/library/asyncio-exceptions.html#asyncio.TimeoutError
* https://docs.python.org/3/library/socket.html#socket.timeout

Based on `os_error_alias.rs`.

## Test Plan

<!-- How was it tested? -->

By running:

```
cargo clippy --workspace --all-targets --all-features -- -D warnings  # Rust linting
RUFF_UPDATE_SCHEMA=1 cargo test  # Rust testing and updating ruff.schema.json
pre-commit run --all-files --show-diff-on-failure  # Rust and Python formatting, Markdown and Python linting, etc.
cargo insta review
```

And also running with different `--target-version` values:

```sh
cargo run -p ruff_cli -- check crates/ruff_linter/resources/test/fixtures/pyupgrade/UP041.py --no-cache --select UP041 --target-version py37 --diff
cargo run -p ruff_cli -- check crates/ruff_linter/resources/test/fixtures/pyupgrade/UP041.py --no-cache --select UP041 --target-version py310 --diff
cargo run -p ruff_cli -- check crates/ruff_linter/resources/test/fixtures/pyupgrade/UP041.py --no-cache --select UP041 --target-version py311 --diff
```
  • Loading branch information
hugovk authored Nov 3, 2023
1 parent 4982694 commit 65effc6
Show file tree
Hide file tree
Showing 9 changed files with 386 additions and 0 deletions.
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

0 comments on commit 65effc6

Please sign in to comment.