-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from all commits
511d8ea
a73b1e3
48cfd1b
9a2a464
1bc559e
77cc2e1
7690005
97ab765
a543a0a
e44383c
9f1190a
f7badb7
796fd0b
7a291b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Manual edits won't be applied when running There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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):
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 workThere was a problem hiding this comment.
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
toViolation
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 usingmanual_edit(s)
)There was a problem hiding this comment.
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.
Allows weird things like
To slip through. Is there a way to efficiently locate the range value of the
:
? I have found thelexer::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 aStmtFunctionDef
? Maybe afind_x_in_range
like util or something along those lines?There was a problem hiding this comment.
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 thecolon
. Then uselocator.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
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.There was a problem hiding this comment.
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
scenario without too much hassle and, if so, include it and only leave out the
scenario.
There was a problem hiding this comment.
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 forcontains_line_break()
so current implementation still tries to make: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?