Skip to content

Commit

Permalink
[pylint] Include builtin warnings in useless-exception-statement (`…
Browse files Browse the repository at this point in the history
…PLW0133`) (#10394)

## Summary

Closes #10392.
  • Loading branch information
charliermarsh committed Mar 13, 2024
1 parent 4db5c29 commit 3243906
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 76 deletions.
@@ -1,8 +1,8 @@
# Test case 1: Useless exception statement
from abc import ABC, abstractmethod
from contextlib import suppress


# Test case 1: Useless exception statement
def func():
AssertionError("This is an assertion error") # PLW0133

Expand Down Expand Up @@ -66,6 +66,11 @@ def func():
x = 1; (RuntimeError("This is an exception")); y = 2 # PLW0133


# Test case 11: Useless warning statement
def func():
UserWarning("This is an assertion error") # PLW0133


# Non-violation test cases: PLW0133


Expand Down
Expand Up @@ -2,6 +2,7 @@ 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_python_stdlib::builtins;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand All @@ -14,6 +15,10 @@ use crate::checkers::ast::Checker;
/// `ValueError("...")` on its own will have no effect (unlike
/// `raise ValueError("...")`) and is likely a mistake.
///
/// ## Known problems
/// This rule only detects built-in exceptions, like `ValueError`, and does
/// not catch user-defined exceptions.
///
/// ## Example
/// ```python
/// ValueError("...")
Expand Down Expand Up @@ -60,38 +65,8 @@ pub(crate) fn useless_exception_statement(checker: &mut Checker, expr: &ast::Stm
}

/// 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
semantic
.resolve_qualified_name(expr)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
[
"",
"SystemExit"
| "Exception"
| "ArithmeticError"
| "AssertionError"
| "AttributeError"
| "BufferError"
| "EOFError"
| "ImportError"
| "LookupError"
| "IndexError"
| "KeyError"
| "MemoryError"
| "NameError"
| "ReferenceError"
| "RuntimeError"
| "NotImplementedError"
| "StopIteration"
| "SyntaxError"
| "SystemError"
| "TypeError"
| "ValueError"
]
)
});
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["", name] if builtins::is_exception(name)))
}
Expand Up @@ -3,6 +3,7 @@ source: crates/ruff_linter/src/rules/pylint/mod.rs
---
useless_exception_statement.py:7:5: PLW0133 [*] Missing `raise` statement on exception
|
5 | # Test case 1: Useless exception statement
6 | def func():
7 | AssertionError("This is an assertion error") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
Expand All @@ -11,7 +12,7 @@ useless_exception_statement.py:7:5: PLW0133 [*] Missing `raise` statement on exc

Unsafe fix
4 4 |
5 5 |
5 5 | # Test case 1: Useless exception statement
6 6 | def func():
7 |- AssertionError("This is an assertion error") # PLW0133
7 |+ raise AssertionError("This is an assertion error") # PLW0133
Expand Down Expand Up @@ -192,4 +193,23 @@ useless_exception_statement.py:66:12: PLW0133 [*] Missing `raise` statement on e
66 |+ x = 1; raise (RuntimeError("This is an exception")); y = 2 # PLW0133
67 67 |
68 68 |
69 69 | # Non-violation test cases: PLW0133
69 69 | # Test case 11: Useless warning statement

useless_exception_statement.py:71:5: PLW0133 [*] Missing `raise` statement on exception
|
69 | # Test case 11: Useless warning statement
70 | def func():
71 | UserWarning("This is an assertion error") # PLW0133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0133
|
= help: Add `raise` keyword

Unsafe fix
68 68 |
69 69 | # Test case 11: Useless warning statement
70 70 | def func():
71 |- UserWarning("This is an assertion error") # PLW0133
71 |+ raise UserWarning("This is an assertion error") # PLW0133
72 72 |
73 73 |
74 74 | # Non-violation test cases: PLW0133
@@ -1,7 +1,9 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::map_callable;
use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor};
use ruff_python_ast::{self as ast, Expr, Stmt, StmtIf};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -96,11 +98,32 @@ fn check_type_check_test(checker: &mut Checker, test: &Expr) -> bool {
}
}

/// Returns `true` if `exc` is a reference to a builtin exception.
fn is_builtin_exception(checker: &mut Checker, exc: &Expr) -> bool {
return checker
.semantic()
.resolve_qualified_name(exc)
fn check_raise(checker: &mut Checker, exc: &Expr, item: &Stmt) {
if is_builtin_exception(exc, checker.semantic()) {
checker
.diagnostics
.push(Diagnostic::new(TypeCheckWithoutTypeError, item.range()));
}
}

/// Search the body of an if-condition for raises.
fn check_body(checker: &mut Checker, body: &[Stmt]) {
for item in body {
if has_control_flow(item) {
return;
}
if let Stmt::Raise(ast::StmtRaise { exc: Some(exc), .. }) = &item {
check_raise(checker, exc, item);
}
}
}

/// Returns `true` if the given expression is a builtin exception.
///
/// This function only matches to a subset of the builtin exceptions, and omits `TypeError`.
fn is_builtin_exception(expr: &Expr, semantic: &SemanticModel) -> bool {
semantic
.resolve_qualified_name(map_callable(expr))
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
Expand All @@ -123,42 +146,7 @@ fn is_builtin_exception(checker: &mut Checker, exc: &Expr) -> bool {
| "ValueError"
]
)
});
}

/// Returns `true` if an [`Expr`] is a reference to a builtin exception.
fn check_raise_type(checker: &mut Checker, exc: &Expr) -> bool {
match exc {
Expr::Name(_) => is_builtin_exception(checker, exc),
Expr::Call(ast::ExprCall { func, .. }) => {
if let Expr::Name(_) = func.as_ref() {
is_builtin_exception(checker, func)
} else {
false
}
}
_ => false,
}
}

fn check_raise(checker: &mut Checker, exc: &Expr, item: &Stmt) {
if check_raise_type(checker, exc) {
checker
.diagnostics
.push(Diagnostic::new(TypeCheckWithoutTypeError, item.range()));
}
}

/// Search the body of an if-condition for raises.
fn check_body(checker: &mut Checker, body: &[Stmt]) {
for item in body {
if has_control_flow(item) {
return;
}
if let Stmt::Raise(ast::StmtRaise { exc: Some(exc), .. }) = &item {
check_raise(checker, exc, item);
}
}
})
}

/// TRY004
Expand Down
76 changes: 76 additions & 0 deletions crates/ruff_python_stdlib/src/builtins.rs
Expand Up @@ -368,3 +368,79 @@ pub fn is_ipython_builtin(name: &str) -> bool {
// Constructed by converting the `IPYTHON_BUILTINS` slice to a `match` expression.
matches!(name, "__IPYTHON__" | "display" | "get_ipython")
}

/// Returns `true` if the given name is that of a builtin exception.
///
/// See: <https://docs.python.org/3/library/exceptions.html#exception-hierarchy>
pub fn is_exception(name: &str) -> bool {
matches!(
name,
"BaseException"
| "BaseExceptionGroup"
| "GeneratorExit"
| "KeyboardInterrupt"
| "SystemExit"
| "Exception"
| "ArithmeticError"
| "FloatingPointError"
| "OverflowError"
| "ZeroDivisionError"
| "AssertionError"
| "AttributeError"
| "BufferError"
| "EOFError"
| "ExceptionGroup"
| "ImportError"
| "ModuleNotFoundError"
| "LookupError"
| "IndexError"
| "KeyError"
| "MemoryError"
| "NameError"
| "UnboundLocalError"
| "OSError"
| "BlockingIOError"
| "ChildProcessError"
| "ConnectionError"
| "BrokenPipeError"
| "ConnectionAbortedError"
| "ConnectionRefusedError"
| "ConnectionResetError"
| "FileExistsError"
| "FileNotFoundError"
| "InterruptedError"
| "IsADirectoryError"
| "NotADirectoryError"
| "PermissionError"
| "ProcessLookupError"
| "TimeoutError"
| "ReferenceError"
| "RuntimeError"
| "NotImplementedError"
| "RecursionError"
| "StopAsyncIteration"
| "StopIteration"
| "SyntaxError"
| "IndentationError"
| "TabError"
| "SystemError"
| "TypeError"
| "ValueError"
| "UnicodeError"
| "UnicodeDecodeError"
| "UnicodeEncodeError"
| "UnicodeTranslateError"
| "Warning"
| "BytesWarning"
| "DeprecationWarning"
| "EncodingWarning"
| "FutureWarning"
| "ImportWarning"
| "PendingDeprecationWarning"
| "ResourceWarning"
| "RuntimeWarning"
| "SyntaxWarning"
| "UnicodeWarning"
| "UserWarning"
)
}

0 comments on commit 3243906

Please sign in to comment.