Skip to content

Commit

Permalink
ruff_linter: permit asyncio.TimeoutError as an unsafe fix for <3.11
Browse files Browse the repository at this point in the history
This commit tweaks the TimeoutErrorAlias lint to suggest
asyncio.TimeoutError in Python <3.11, but only as an unsafe fix. Namely,
in <3.11, asyncio.TimeoutError is not an alias and thus the
transformation could change the semantics of the program.

In the case where there is a tuple containing both safe and unsafe
fixes, I decided to keep it as a single suggested fix and made it
overall unsafe. So for example, if Ruff sees this code in Python 3.10:

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

Then it will suggest this as an unsafe fix:

    try:
      pass
    except TimeoutError:
      pass

It could, though, suggest this as a safe fix:

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

since socket.timeout became an alias of TimeoutError in Python 3.10.

I opted not to go this route because it wasn't obvious to me that it was
worth it.

Fixes #8565
  • Loading branch information
BurntSushi committed Nov 9, 2023
1 parent 7043846 commit ba22f95
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 46 deletions.
17 changes: 17 additions & 0 deletions crates/ruff_diagnostics/src/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Expand Down
103 changes: 61 additions & 42 deletions crates/ruff_linter/src/rules/pyupgrade/rules/timeout_error_alias.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -57,26 +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(),
[""] | ["socket", "timeout"] | ["asyncio", "TimeoutError"]
)
/// Return an appropriate `Applicability` if an [`Expr`] is an alias of
/// `TimeoutError`.
fn alias_applicability(checker: &mut Checker, expr: &Expr) -> Option<Applicability> {
// 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 {
// 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!(
target_version >= PythonVersion::Py310,
"lint should only be used for Python 3.10+",
);
matches!(call_path.as_slice(), [""] | ["socket", "timeout"])
}
})
Some(Applicability::Unsafe)
};
}
None
}

/// Return `true` if an [`Expr`] is `TimeoutError`.
Expand All @@ -87,24 +95,27 @@ 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),
},
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.
Expand Down Expand Up @@ -145,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);
}
Expand All @@ -162,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);
}
}
_ => {}
Expand All @@ -185,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);
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,26 @@
---
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:
Expand All @@ -21,6 +41,26 @@ UP041.py:10:8: UP041 [*] Replace aliased errors with `TimeoutError`
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:
Expand Down Expand Up @@ -51,12 +91,12 @@ UP041.py:27:8: UP041 [*] Replace aliased errors with `TimeoutError`
|
= help: Replace with builtin `TimeoutError`

Safe fix
Unsafe fix
24 24 |
25 25 | try:
26 26 | pass
27 |-except (asyncio.TimeoutError, socket.timeout,):
27 |+except (TimeoutError, asyncio.TimeoutError):
27 |+except TimeoutError:
28 28 | pass
29 29 |
30 30 | # Should be kept in parentheses (because multiple)
Expand All @@ -71,14 +111,34 @@ UP041.py:34:8: UP041 [*] Replace aliased errors with `TimeoutError`
|
= help: Replace with builtin `TimeoutError`

Safe fix
Unsafe fix
31 31 |
32 32 | try:
33 33 | pass
34 |-except (asyncio.TimeoutError, socket.timeout, KeyError, TimeoutError):
34 |+except (asyncio.TimeoutError, 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


0 comments on commit ba22f95

Please sign in to comment.