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] add autofix for B006 mutable_argument_default #4880

Closed
wants to merge 14 commits into from
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

Copy link
Member

Choose a reason for hiding this comment

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

another test case i'd like to see is a multi-line default value

Copy link
Member

@charliermarsh charliermarsh Jul 7, 2023

Choose a reason for hiding this comment

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

We also need to handle same-line functions (we should just avoid trying to fix these):

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add both and have already set fix to manual. Can also change to not suggest at all for same-line funcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So multiline arg seems to do fine, but same-line func creates defunct Python. It's best to not fix these indeed. Is there a quick way to check if a function is one line, or its body is on the same line as its def?

Copy link
Member

Choose a reason for hiding this comment

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

locator.contains_line_break between the colon (:) and the first body statement should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for single-line func and changed from AlwaysAutoFixableViolation to Violation and sometimes fixable (also already changed the fixes to manual as per request in another comment so unsure what type of Violation the rule should now be to be fair, as so far it appears to be the only rule using manual_edit(s))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm my heuristic is not covering all cases in the end.

checker
                    .locator
                    .contains_line_break(TextRange::new(arguments.range().end(), body[0].start()))

Allows weird things like

def single_line_func_wrong(value = {}

                           )\
    : ...

To slip through. Is there a way to efficiently locate the range value of the :? I have found the lexer::lex but that would require getting a string representation of the function def. Is that the way to go, or is there a better way to locate single characters in parts of a StmtFunctionDef? Maybe a find_x_in_range like util or something along those lines?

Copy link
Member

Choose a reason for hiding this comment

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

Using the lexer when creating a diagnostic can be useful, but it is preferred not to when possible. But I don't see how you can get away with not using the lexer here (we wrote a more lightweight lexer in the formatter, because getting to tokens is such a common task, maybe it's time to make it available to ruff too). You can get the string representation of the function def by using locator.slice(def.range()). You can then start lexing after the last argument, find the colon. Then use locator.contains_line_break(TextRange::new(colon.end(), first_statement.start()

Which raises an interesting question. Is this valid? Is this a suite or a single statement body

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

Unrelated: @konstin a use case where StmtSuite would be handy... It would tell us right away whether it is a single statement or a suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hyperclear instructions and context. I'll see if I can spot and autofix the

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

scenario without too much hassle and, if so, include it and only leave out the

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

scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've managed to exclude def single_line_func_wrong(value = {}): ... based on your instructions. The other scenario you raise is tricky though. The lexer does not do anything with the \ and it counts as a line break for contains_line_break() so current implementation still tries to make:

def single_line_func_wrong(value = None):\
    if value is None:
        value = {}

of it, which is broken. Is there a way to discern \ line breaks from other line breaks? Or do we want to go a different route here?


def multiline_arg_wrong(value={

}):
...

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


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

Expand Down
8 changes: 5 additions & 3 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ where
returns,
args,
body,
range,
..
})
| Stmt::AsyncFunctionDef(ast::StmtAsyncFunctionDef {
Expand All @@ -351,8 +352,12 @@ where
returns,
args,
body,
range,
..
}) => {
if self.enabled(Rule::MutableArgumentDefault) {
flake8_bugbear::rules::mutable_argument_default(self, args, body, *range);
}
if self.enabled(Rule::DjangoNonLeadingReceiverDecorator) {
flake8_django::rules::non_leading_receiver_decorator(self, decorator_list);
}
Expand Down Expand Up @@ -4010,9 +4015,6 @@ where
}

fn visit_arguments(&mut self, arguments: &'b Arguments) {
if self.enabled(Rule::MutableArgumentDefault) {
flake8_bugbear::rules::mutable_argument_default(self, arguments);
}
if self.enabled(Rule::FunctionCallInDefaultArgument) {
flake8_bugbear::rules::function_call_argument_default(self, arguments);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
use rustpython_parser::ast::{ArgWithDefault, Arguments, Ranged};
use rustpython_parser::ast::{ArgWithDefault, Arguments, Ranged, Stmt};

use ruff_diagnostics::{Diagnostic, Violation};
use crate::registry::AsRule;
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::source_code::Locator;
use ruff_python_ast::whitespace::indentation_at_offset;
use ruff_python_semantic::analyze::typing::{is_immutable_annotation, is_mutable_expr};
use ruff_text_size::TextRange;
use rustpython_parser::lexer::lex_starts_at;
use rustpython_parser::{Mode, Tok};

use crate::checkers::ast::Checker;

Expand Down Expand Up @@ -50,14 +57,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")
format!("Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None`")
}
fn autofix_title(&self) -> Option<String> {
Some(format!(
"Do not use mutable data structures for argument defaults"
))
Comment on lines -55 to +69
Copy link
Member

Choose a reason for hiding this comment

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

Should this be reversed as in the new message (the one with "Replace...") would be the autofix title?

Copy link
Contributor Author

@qdegraaf qdegraaf Jul 20, 2023

Choose a reason for hiding this comment

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

Yeah that was the case until: #4880 (comment) but that was more of a length argument. Agree that the phrasing now is potentially confusing. Can find a way to get the best of both worlds.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting! I think editors would be using the violation message to display instead of the autofix title and I believe that's the top level key in the diagnostic data sent by the LSP server as well. \cc @MichaReiser

}
}

/// B006
pub(crate) fn mutable_argument_default(checker: &mut Checker, arguments: &Arguments) {
pub(crate) fn mutable_argument_default(
checker: &mut Checker,
arguments: &Arguments,
body: &[Stmt],
qdegraaf marked this conversation as resolved.
Show resolved Hide resolved
func_range: TextRange,
) {
// Scan in reverse order to right-align zip().
for ArgWithDefault {
def,
Expand All @@ -78,9 +97,53 @@ pub(crate) fn mutable_argument_default(checker: &mut Checker, arguments: &Argume
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(checker.locator, body[0].start()).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", def.arg.as_str()).as_str());
qdegraaf marked this conversation as resolved.
Show resolved Hide resolved
check_lines.push_str(indentation);
check_lines.push_str(checker.stylist.indentation());
check_lines.push_str(
format!(
"{} = {}",
def.arg.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]));
Copy link
Member

Choose a reason for hiding this comment

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

Manual edits won't be applied when running --fix in the future and are intended to be used for fixes that are known to contain syntax errors. Do you expect the fixes to contain syntax errors? If not, consider using suggested instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently only syntax errors I am aware of that can occur is in the case of weirdly formatted single line funcs. See also: #4880 (comment) if that can be fixed it can go to suggested AFAIAC.

}
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