Skip to content

Commit

Permalink
[flake8-bugbear] Treat raise NotImplemented-only bodies as stub f…
Browse files Browse the repository at this point in the history
…unctions (#10990)

## Summary

As discussed in
#10083 (comment),
stubs detection now also covers the case where the function body raises
NotImplementedError and does nothing else.

## Test Plan

Tests for the relevant cases were added in B006_8.py
  • Loading branch information
Philipp-Thiel committed Apr 17, 2024
1 parent f48a794 commit 2971655
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 5 deletions.
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
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"))
}),
_ => 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

0 comments on commit 2971655

Please sign in to comment.