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

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

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,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
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,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
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,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 @@ -164,6 +165,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
Original file line number Diff line number Diff line change
@@ -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"
]
)
});
}