Skip to content

Commit

Permalink
[flake8-bugbear] Add autofix for B006 (#6131)
Browse files Browse the repository at this point in the history
## Summary

Reopening of #4880 

One open TODO as described in:
#4880 (comment)

FYI @charliermarsh seeing as you commented you wanted to do final review
and merge. @konstin @dhruvmanila @MichaReiser as previous reviewers.

# Old Description
## Summary

Adds an autofix for B006 turning mutable argument defaults into None and
setting their original value back in the function body if still `None`
at runtime like so:
```python
def before(x=[]):
    pass
    
def after(x=None):
    if x is None:
        x = []
    pass
```

## Test Plan

Added an extra test case to existing fixture with more indentation.
Checked results for all old examples.

NOTE: Also adapted the jupyter notebook test as this checked for B006 as
well.

## Issue link

Closes: #4693

---------

Co-authored-by: konstin <[email protected]>
  • Loading branch information
qdegraaf and konstin authored Aug 10, 2023
1 parent 4811af0 commit 50dab9c
Show file tree
Hide file tree
Showing 6 changed files with 449 additions and 120 deletions.
14 changes: 14 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_bugbear/B006_B008.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,20 @@ def this_is_also_wrong(value={}):
...


class Foo:
@staticmethod
def this_is_also_wrong_and_more_indented(value={}):
pass


def multiline_arg_wrong(value={

}):
...

def single_line_func_wrong(value = {}): ...


def and_this(value=set()):
...

Expand Down
3 changes: 0 additions & 3 deletions crates/ruff/src/checkers/ast/analyze/parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@ use crate::rules::{flake8_bugbear, flake8_pyi, ruff};

/// Run lint rules over a [`Parameters`] syntax node.
pub(crate) fn parameters(parameters: &Parameters, checker: &mut Checker) {
if checker.enabled(Rule::MutableArgumentDefault) {
flake8_bugbear::rules::mutable_argument_default(checker, parameters);
}
if checker.enabled(Rule::FunctionCallInDefaultArgument) {
flake8_bugbear::rules::function_call_in_argument_default(checker, parameters);
}
Expand Down
5 changes: 4 additions & 1 deletion crates/ruff/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
parameters,
body,
type_params,
range: _,
range,
}) => {
if checker.enabled(Rule::DjangoNonLeadingReceiverDecorator) {
flake8_django::rules::non_leading_receiver_decorator(checker, decorator_list);
Expand Down Expand Up @@ -204,6 +204,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::CachedInstanceMethod) {
flake8_bugbear::rules::cached_instance_method(checker, decorator_list);
}
if checker.enabled(Rule::MutableArgumentDefault) {
flake8_bugbear::rules::mutable_argument_default(checker, parameters, body, *range);
}
if checker.any_enabled(&[
Rule::UnnecessaryReturnNone,
Rule::ImplicitReturnValue,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
use ruff_python_ast::{ParameterWithDefault, Parameters, Ranged};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::docstrings::leading_space;
use ruff_python_ast::{ParameterWithDefault, Parameters, Ranged, Stmt};
use ruff_python_parser::lexer::lex_starts_at;
use ruff_python_parser::{Mode, Tok};
use ruff_python_semantic::analyze::typing::{is_immutable_annotation, is_mutable_expr};
use ruff_python_trivia::indentation_at_offset;
use ruff_source_file::Locator;
use ruff_text_size::TextRange;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;

/// ## What it does
/// Checks for uses of mutable objects as function argument defaults.
Expand Down Expand Up @@ -50,14 +56,26 @@ use crate::checkers::ast::Checker;
pub struct MutableArgumentDefault;

impl Violation for MutableArgumentDefault {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Do not use mutable data structures for argument defaults")
}
fn autofix_title(&self) -> Option<String> {
Some(format!(
"Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None`"
))
}
}

/// B006
pub(crate) fn mutable_argument_default(checker: &mut Checker, parameters: &Parameters) {
pub(crate) fn mutable_argument_default(
checker: &mut Checker,
parameters: &Parameters,
body: &[Stmt],
func_range: TextRange,
) {
// Scan in reverse order to right-align zip().
for ParameterWithDefault {
parameter,
Expand All @@ -79,9 +97,53 @@ pub(crate) fn mutable_argument_default(checker: &mut Checker, parameters: &Param
.as_ref()
.is_some_and(|expr| is_immutable_annotation(expr, checker.semantic()))
{
checker
.diagnostics
.push(Diagnostic::new(MutableArgumentDefault, default.range()));
let mut diagnostic = Diagnostic::new(MutableArgumentDefault, default.range());

// If the function body is on the same line as the function def, do not fix
if checker.patch(diagnostic.kind.rule())
&& !is_single_line(checker.locator(), func_range, body)
{
// Set the default arg value to None
let arg_edit = Edit::range_replacement("None".to_string(), default.range());

// Add conditional check to set the default arg to its original value if still None
let mut check_lines = String::new();
let indentation =
indentation_at_offset(body[0].start(), checker.locator()).unwrap_or_default();
let indentation = leading_space(indentation);
// body[0].start() starts at correct indentation so we do need to add indentation
// before pushing the if statement
check_lines.push_str(format!("if {} is None:\n", parameter.name.as_str()).as_str());
check_lines.push_str(indentation);
check_lines.push_str(checker.stylist().indentation());
check_lines.push_str(
format!(
"{} = {}",
parameter.name.as_str(),
checker.generator().expr(default),
)
.as_str(),
);
check_lines.push_str(&checker.stylist().line_ending());
check_lines.push_str(indentation);
let check_edit = Edit::insertion(check_lines, body[0].start());

diagnostic.set_fix(Fix::manual_edits(arg_edit, [check_edit]));
}
checker.diagnostics.push(diagnostic);
}
}
}

fn is_single_line(locator: &Locator, func_range: TextRange, body: &[Stmt]) -> bool {
let arg_string = locator.slice(func_range);
for (tok, range) in lex_starts_at(arg_string, Mode::Module, func_range.start()).flatten() {
match tok {
Tok::Colon => {
return !locator.contains_line_break(TextRange::new(range.end(), body[0].start()));
}
_ => continue,
}
}
false
}
Loading

0 comments on commit 50dab9c

Please sign in to comment.