From 2971655b280545583b2bedb7d1bdbbbbcaa733dd Mon Sep 17 00:00:00 2001 From: Philipp Thiel <153559399+Philipp-Thiel@users.noreply.github.com> Date: Wed, 17 Apr 2024 16:06:40 +0200 Subject: [PATCH] [`flake8-bugbear`] Treat `raise NotImplemented`-only bodies as stub functions (#10990) ## Summary As discussed in https://github.com/astral-sh/ruff/issues/10083#issuecomment-1969653610, 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 --- .../test/fixtures/flake8_bugbear/B006_8.py | 20 ++++ .../src/rules/flake8_bugbear/mod.rs | 1 + .../rules/mutable_argument_default.rs | 23 ++++- ...flake8_bugbear__tests__B006_B006_8.py.snap | 92 +++++++++++++++++++ 4 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_8.py create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_8.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_8.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_8.py new file mode 100644 index 0000000000000..8f948fa1cd510 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B006_8.py @@ -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 diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index 4049d4a0149eb..bbe8cd4d3aa61 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs @@ -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"))] diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index 39bcbad883ee9..ae17d50b423ae 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -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; @@ -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(), @@ -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, @@ -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)); } @@ -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: _ }) => { @@ -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, }) } diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_8.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_8.py.snap new file mode 100644 index 0000000000000..866643aa45474 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B006_B006_8.py.snap @@ -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