Skip to content

Commit

Permalink
[pylint] Implement useless-exception-statement (W0133) (astral-…
Browse files Browse the repository at this point in the history
…sh#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](astral-sh#10145)

## Test Plan

```bash
cargo test & manually
```
  • Loading branch information
mikeleppane authored and nkxxll committed Mar 4, 2024
1 parent ea8836a commit 70fdeb2
Show file tree
Hide file tree
Showing 8 changed files with 418 additions and 1 deletion.
@@ -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
5 changes: 4 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Expand Up @@ -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);
}
Expand All @@ -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);
}
}
_ => {}
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Expand Up @@ -294,6 +294,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),
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Expand Up @@ -80,6 +80,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::*;
Expand Down Expand Up @@ -168,6 +169,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;
Expand Down
@@ -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"
]
)
});
}

0 comments on commit 70fdeb2

Please sign in to comment.