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

[flake8-bugbear] Treat raise NotImplemented-only bodies as stub functions #10990

Merged
merged 9 commits into from
Apr 17, 2024
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,20 @@
def foo(a: list = []):
raise NotImplementedError("")


def bar(a: dict = {}):
""" This one also has a docstring"""
raise NotImplementedError("and has some text in here")


def baz(a: list = []):
"""This one raises a different exception"""
raise IndexError()


def qux(a: list = []):
raise NotImplementedError


def quux(a: list = []):
raise NotImplemented

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To verify that we treat raise NotImplemented like raise NotImplementedError. (Just to be clear, these aren't examples, they're tests.)

1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ mod tests {
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_5.py"))]
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_6.py"))]
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_7.py"))]
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_8.py"))]
#[test_case(Rule::MutableArgumentDefault, Path::new("B006_B008.py"))]
#[test_case(Rule::NoExplicitStacklevel, Path::new("B028.py"))]
#[test_case(Rule::RaiseLiteral, Path::new("B016.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::is_docstring_stmt;
use ruff_python_ast::helpers::{is_docstring_stmt, map_callable};
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::{self as ast, Expr, Parameter, ParameterWithDefault, Stmt};
use ruff_python_ast::{self as ast, Expr, Parameter, ParameterWithDefault, Stmt, StmtRaise};
use ruff_python_codegen::{Generator, Stylist};
use ruff_python_index::Indexer;
use ruff_python_semantic::analyze::typing::{is_immutable_annotation, is_mutable_expr};
use ruff_python_semantic::SemanticModel;
use ruff_python_trivia::{indentation_at_offset, textwrap};
use ruff_source_file::Locator;
use ruff_text_size::Ranged;
Expand Down Expand Up @@ -118,6 +119,7 @@ pub(crate) fn mutable_argument_default(checker: &mut Checker, function_def: &ast
function_def,
parameter,
default,
checker.semantic(),
checker.locator(),
checker.stylist(),
checker.indexer(),
Expand All @@ -132,10 +134,12 @@ pub(crate) fn mutable_argument_default(checker: &mut Checker, function_def: &ast

/// Generate a [`Fix`] to move a mutable argument default initialization
/// into the function body.
#[allow(clippy::too_many_arguments)]
fn move_initialization(
function_def: &ast::StmtFunctionDef,
parameter: &Parameter,
default: &Expr,
semantic: &SemanticModel,
locator: &Locator,
stylist: &Stylist,
indexer: &Indexer,
Expand All @@ -153,7 +157,7 @@ fn move_initialization(
let default_edit = Edit::range_replacement("None".to_string(), default.range());

// If the function is a stub, this is the only necessary edit.
if is_stub(function_def) {
if is_stub(function_def, semantic) {
return Some(Fix::unsafe_edit(default_edit));
}

Expand Down Expand Up @@ -213,8 +217,8 @@ fn move_initialization(
/// Returns `true` if a function has an empty body, and is therefore a stub.
///
/// A function body is considered to be empty if it contains only `pass` statements, `...` literals,
/// and docstrings.
fn is_stub(function_def: &ast::StmtFunctionDef) -> bool {
/// `NotImplementedError` raises, or string literal statements (docstrings).
fn is_stub(function_def: &ast::StmtFunctionDef, semantic: &SemanticModel) -> bool {
function_def.body.iter().all(|stmt| match stmt {
Stmt::Pass(_) => true,
Stmt::Expr(ast::StmtExpr { value, range: _ }) => {
Expand All @@ -223,6 +227,15 @@ fn is_stub(function_def: &ast::StmtFunctionDef) -> bool {
Expr::StringLiteral(_) | Expr::EllipsisLiteral(_)
)
}
Stmt::Raise(StmtRaise {
range: _,
exc: exception,
cause: _,
}) => exception.as_ref().is_some_and(|exc| {
semantic
.resolve_builtin_symbol(map_callable(exc))
.is_some_and(|name| matches!(name, "NotImplementedError" | "NotImplemented"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to use the semantic model, and allow raise NotImplementedError (without argument) and raise NotImplemented.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you allow raise NotImplemented? NotImplemented is not an exception. It should never be raised. NotImplemented can be returned by comparison operators.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have separate rules to guard against raise NotImplemented. But if the entire function is raise NotImplemented, it's almost certainly intended to be raise NotImplementedError, right?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, fair enough!

}),
_ => false,
})
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---
B006_8.py:1:19: B006 [*] Do not use mutable data structures for argument defaults
|
1 | def foo(a: list = []):
| ^^ B006
2 | raise NotImplementedError("")
|
= help: Replace with `None`; initialize within function

ℹ Unsafe fix
1 |-def foo(a: list = []):
1 |+def foo(a: list = None):
2 2 | raise NotImplementedError("")
3 3 |
4 4 |

B006_8.py:5:19: B006 [*] Do not use mutable data structures for argument defaults
|
5 | def bar(a: dict = {}):
| ^^ B006
6 | """ This one also has a docstring"""
7 | raise NotImplementedError("and has some text in here")
|
= help: Replace with `None`; initialize within function

ℹ Unsafe fix
2 2 | raise NotImplementedError("")
3 3 |
4 4 |
5 |-def bar(a: dict = {}):
5 |+def bar(a: dict = None):
6 6 | """ This one also has a docstring"""
7 7 | raise NotImplementedError("and has some text in here")
8 8 |

B006_8.py:10:19: B006 [*] Do not use mutable data structures for argument defaults
|
10 | def baz(a: list = []):
| ^^ B006
11 | """This one raises a different exception"""
12 | raise IndexError()
|
= help: Replace with `None`; initialize within function

ℹ Unsafe fix
7 7 | raise NotImplementedError("and has some text in here")
8 8 |
9 9 |
10 |-def baz(a: list = []):
10 |+def baz(a: list = None):
11 11 | """This one raises a different exception"""
12 |+ if a is None:
13 |+ a = []
12 14 | raise IndexError()
13 15 |
14 16 |

B006_8.py:15:19: B006 [*] Do not use mutable data structures for argument defaults
|
15 | def qux(a: list = []):
| ^^ B006
16 | raise NotImplementedError
|
= help: Replace with `None`; initialize within function

ℹ Unsafe fix
12 12 | raise IndexError()
13 13 |
14 14 |
15 |-def qux(a: list = []):
15 |+def qux(a: list = None):
16 16 | raise NotImplementedError
17 17 |
18 18 |

B006_8.py:19:20: B006 [*] Do not use mutable data structures for argument defaults
|
19 | def quux(a: list = []):
| ^^ B006
20 | raise NotImplemented
|
= help: Replace with `None`; initialize within function

ℹ Unsafe fix
16 16 | raise NotImplementedError
17 17 |
18 18 |
19 |-def quux(a: list = []):
19 |+def quux(a: list = None):
20 20 | raise NotImplemented