[pylint] Implement useless-exception-statement (W0133) (#10176)

## Summary

This review contains a new rule for handling `useless exception
statements` (`PLW0133`). Is it based on the following pylint's rule:
[W0133](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/pointless-exception-statement.html)


Note: this rule does not cover the case if an error is a custom
exception class.

See: [Rule request](https://github.com/astral-sh/ruff/issues/10145)

## Test Plan

```bash
cargo test & manually
```
This commit is contained in:
Mikko Leppänen 2024-03-01 04:37:16 +02:00 committed by GitHub
parent cbafae022d
commit 8e0a70cfa3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 418 additions and 1 deletions

View file

@ -0,0 +1,116 @@
# Test case 1: Useless exception statement
from abc import ABC, abstractmethod
from contextlib import suppress
def func():
AssertionError("This is an assertion error") # PLW0133
# Test case 2: Useless exception statement in try-except block
def func():
try:
Exception("This is an exception") # PLW0133
except Exception as err:
pass
# Test case 3: Useless exception statement in if statement
def func():
if True:
RuntimeError("This is an exception") # PLW0133
# Test case 4: Useless exception statement in class
def func():
class Class:
def __init__(self):
TypeError("This is an exception") # PLW0133
# Test case 5: Useless exception statement in function
def func():
def inner():
IndexError("This is an exception") # PLW0133
inner()
# Test case 6: Useless exception statement in while loop
def func():
while True:
KeyError("This is an exception") # PLW0133
# Test case 7: Useless exception statement in abstract class
def func():
class Class(ABC):
@abstractmethod
def method(self):
NotImplementedError("This is an exception") # PLW0133
# Test case 8: Useless exception statement inside context manager
def func():
with suppress(AttributeError):
AttributeError("This is an exception") # PLW0133
# Test case 9: Useless exception statement in parentheses
def func():
(RuntimeError("This is an exception")) # PLW0133
# Test case 10: Useless exception statement in continuation
def func():
x = 1; (RuntimeError("This is an exception")); y = 2 # PLW0133
# Non-violation test cases: PLW0133
# Test case 1: Used exception statement in try-except block
def func():
raise Exception("This is an exception") # OK
# Test case 2: Used exception statement in if statement
def func():
if True:
raise ValueError("This is an exception") # OK
# Test case 3: Used exception statement in class
def func():
class Class:
def __init__(self):
raise TypeError("This is an exception") # OK
# Test case 4: Exception statement used in list comprehension
def func():
[ValueError("This is an exception") for i in range(10)] # OK
# Test case 5: Exception statement used when initializing a dictionary
def func():
{i: TypeError("This is an exception") for i in range(10)} # OK
# Test case 6: Exception statement used in function
def func():
def inner():
raise IndexError("This is an exception") # OK
inner()
# Test case 7: Exception statement used in variable assignment
def func():
err = KeyError("This is an exception") # OK
# Test case 8: Exception statement inside context manager
def func():
with suppress(AttributeError):
raise AttributeError("This is an exception") # OK

View file

@ -1609,7 +1609,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
refurb::rules::delete_full_slice(checker, delete);
}
}
Stmt::Expr(ast::StmtExpr { value, range: _ }) => {
Stmt::Expr(expr @ ast::StmtExpr { value, range: _ }) => {
if checker.enabled(Rule::UselessComparison) {
flake8_bugbear::rules::useless_comparison(checker, value);
}
@ -1632,6 +1632,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::RepeatedAppend) {
refurb::rules::repeated_append(checker, stmt);
}
if checker.enabled(Rule::UselessExceptionStatement) {
pylint::rules::useless_exception_statement(checker, expr);
}
}
_ => {}
}

View file

@ -293,6 +293,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "W0127") => (RuleGroup::Stable, rules::pylint::rules::SelfAssigningVariable),
(Pylint, "W0129") => (RuleGroup::Stable, rules::pylint::rules::AssertOnStringLiteral),
(Pylint, "W0131") => (RuleGroup::Stable, rules::pylint::rules::NamedExprWithoutContext),
(Pylint, "W0133") => (RuleGroup::Preview, rules::pylint::rules::UselessExceptionStatement),
(Pylint, "W0245") => (RuleGroup::Preview, rules::pylint::rules::SuperWithoutBrackets),
(Pylint, "W0406") => (RuleGroup::Stable, rules::pylint::rules::ImportSelf),
(Pylint, "W0602") => (RuleGroup::Stable, rules::pylint::rules::GlobalVariableNotAssigned),

View file

@ -177,6 +177,10 @@ mod tests {
Rule::UnnecessaryDictIndexLookup,
Path::new("unnecessary_dict_index_lookup.py")
)]
#[test_case(
Rule::UselessExceptionStatement,
Path::new("useless_exception_statement.py")
)]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(

View file

@ -79,6 +79,7 @@ pub(crate) use unnecessary_lambda::*;
pub(crate) use unnecessary_list_index_lookup::*;
pub(crate) use unspecified_encoding::*;
pub(crate) use useless_else_on_loop::*;
pub(crate) use useless_exception_statement::*;
pub(crate) use useless_import_alias::*;
pub(crate) use useless_return::*;
pub(crate) use useless_with_lock::*;
@ -166,6 +167,7 @@ mod unnecessary_lambda;
mod unnecessary_list_index_lookup;
mod unspecified_encoding;
mod useless_else_on_loop;
mod useless_exception_statement;
mod useless_import_alias;
mod useless_return;
mod useless_with_lock;

View file

@ -0,0 +1,95 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for an exception that is not raised.
///
/// ## Why is this bad?
/// It's unnecessary to create an exception without raising it. For example,
/// `ValueError("...")` on its own will have no effect (unlike
/// `raise ValueError("...")`) and is likely a mistake.
///
/// ## Example
/// ```python
/// ValueError("...")
/// ```
///
/// Use instead:
/// ```python
/// raise ValueError("...")
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as converting a useless exception
/// statement to a `raise` statement will change the program's behavior.
#[violation]
pub struct UselessExceptionStatement;
impl Violation for UselessExceptionStatement {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
format!("Missing `raise` statement on exception")
}
fn fix_title(&self) -> Option<String> {
Some(format!("Add `raise` keyword"))
}
}
/// PLW0133
pub(crate) fn useless_exception_statement(checker: &mut Checker, expr: &ast::StmtExpr) {
let Expr::Call(ast::ExprCall { func, .. }) = expr.value.as_ref() else {
return;
};
if is_builtin_exception(func, checker.semantic()) {
let mut diagnostic = Diagnostic::new(UselessExceptionStatement, expr.range());
diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
"raise ".to_string(),
expr.start(),
)));
checker.diagnostics.push(diagnostic);
}
}
/// Returns `true` if the given expression is a builtin exception.
///
/// See: <https://docs.python.org/3/library/exceptions.html#exception-hierarchy>
fn is_builtin_exception(expr: &Expr, semantic: &SemanticModel) -> bool {
return semantic.resolve_call_path(expr).is_some_and(|call_path| {
matches!(
call_path.as_slice(),
[
"",
"SystemExit"
| "Exception"
| "ArithmeticError"
| "AssertionError"
| "AttributeError"
| "BufferError"
| "EOFError"
| "ImportError"
| "LookupError"
| "IndexError"
| "KeyError"
| "MemoryError"
| "NameError"
| "ReferenceError"
| "RuntimeError"
| "NotImplementedError"
| "StopIteration"
| "SyntaxError"
| "SystemError"
| "TypeError"
| "ValueError"
]
)
});
}

View file

@ -0,0 +1,195 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
useless_exception_statement.py:7:5: PLW0133 [*] Missing `raise` statement on exception
|
6 | def func():
7 | AssertionError("This is an assertion error") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
4 4 |
5 5 |
6 6 | def func():
7 |- AssertionError("This is an assertion error") # PLW0133
7 |+ raise AssertionError("This is an assertion error") # PLW0133
8 8 |
9 9 |
10 10 | # Test case 2: Useless exception statement in try-except block
useless_exception_statement.py:13:9: PLW0133 [*] Missing `raise` statement on exception
|
11 | def func():
12 | try:
13 | Exception("This is an exception") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
14 | except Exception as err:
15 | pass
|
= help: Add `raise` keyword
Unsafe fix
10 10 | # Test case 2: Useless exception statement in try-except block
11 11 | def func():
12 12 | try:
13 |- Exception("This is an exception") # PLW0133
13 |+ raise Exception("This is an exception") # PLW0133
14 14 | except Exception as err:
15 15 | pass
16 16 |
useless_exception_statement.py:21:9: PLW0133 [*] Missing `raise` statement on exception
|
19 | def func():
20 | if True:
21 | RuntimeError("This is an exception") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
18 18 | # Test case 3: Useless exception statement in if statement
19 19 | def func():
20 20 | if True:
21 |- RuntimeError("This is an exception") # PLW0133
21 |+ raise RuntimeError("This is an exception") # PLW0133
22 22 |
23 23 |
24 24 | # Test case 4: Useless exception statement in class
useless_exception_statement.py:28:13: PLW0133 [*] Missing `raise` statement on exception
|
26 | class Class:
27 | def __init__(self):
28 | TypeError("This is an exception") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
25 25 | def func():
26 26 | class Class:
27 27 | def __init__(self):
28 |- TypeError("This is an exception") # PLW0133
28 |+ raise TypeError("This is an exception") # PLW0133
29 29 |
30 30 |
31 31 | # Test case 5: Useless exception statement in function
useless_exception_statement.py:34:9: PLW0133 [*] Missing `raise` statement on exception
|
32 | def func():
33 | def inner():
34 | IndexError("This is an exception") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
35 |
36 | inner()
|
= help: Add `raise` keyword
Unsafe fix
31 31 | # Test case 5: Useless exception statement in function
32 32 | def func():
33 33 | def inner():
34 |- IndexError("This is an exception") # PLW0133
34 |+ raise IndexError("This is an exception") # PLW0133
35 35 |
36 36 | inner()
37 37 |
useless_exception_statement.py:42:9: PLW0133 [*] Missing `raise` statement on exception
|
40 | def func():
41 | while True:
42 | KeyError("This is an exception") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
39 39 | # Test case 6: Useless exception statement in while loop
40 40 | def func():
41 41 | while True:
42 |- KeyError("This is an exception") # PLW0133
42 |+ raise KeyError("This is an exception") # PLW0133
43 43 |
44 44 |
45 45 | # Test case 7: Useless exception statement in abstract class
useless_exception_statement.py:50:13: PLW0133 [*] Missing `raise` statement on exception
|
48 | @abstractmethod
49 | def method(self):
50 | NotImplementedError("This is an exception") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
47 47 | class Class(ABC):
48 48 | @abstractmethod
49 49 | def method(self):
50 |- NotImplementedError("This is an exception") # PLW0133
50 |+ raise NotImplementedError("This is an exception") # PLW0133
51 51 |
52 52 |
53 53 | # Test case 8: Useless exception statement inside context manager
useless_exception_statement.py:56:9: PLW0133 [*] Missing `raise` statement on exception
|
54 | def func():
55 | with suppress(AttributeError):
56 | AttributeError("This is an exception") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
53 53 | # Test case 8: Useless exception statement inside context manager
54 54 | def func():
55 55 | with suppress(AttributeError):
56 |- AttributeError("This is an exception") # PLW0133
56 |+ raise AttributeError("This is an exception") # PLW0133
57 57 |
58 58 |
59 59 | # Test case 9: Useless exception statement in parentheses
useless_exception_statement.py:61:5: PLW0133 [*] Missing `raise` statement on exception
|
59 | # Test case 9: Useless exception statement in parentheses
60 | def func():
61 | (RuntimeError("This is an exception")) # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
58 58 |
59 59 | # Test case 9: Useless exception statement in parentheses
60 60 | def func():
61 |- (RuntimeError("This is an exception")) # PLW0133
61 |+ raise (RuntimeError("This is an exception")) # PLW0133
62 62 |
63 63 |
64 64 | # Test case 10: Useless exception statement in continuation
useless_exception_statement.py:66:12: PLW0133 [*] Missing `raise` statement on exception
|
64 | # Test case 10: Useless exception statement in continuation
65 | def func():
66 | x = 1; (RuntimeError("This is an exception")); y = 2 # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword
Unsafe fix
63 63 |
64 64 | # Test case 10: Useless exception statement in continuation
65 65 | def func():
66 |- x = 1; (RuntimeError("This is an exception")); y = 2 # PLW0133
66 |+ x = 1; raise (RuntimeError("This is an exception")); y = 2 # PLW0133
67 67 |
68 68 |
69 69 | # Non-violation test cases: PLW0133

1
ruff.schema.json generated
View file

@ -3314,6 +3314,7 @@
"PLW0129",
"PLW013",
"PLW0131",
"PLW0133",
"PLW02",
"PLW024",
"PLW0245",