B006 (mutable-argument-default) no longer offers autofix even with --unsafe-fixes

[cjolowicz]

Hello 👋

B006 (mutable-argument-default) no longer offers an autofix even when --unsafe-fixes is passed.

Use the example from ruff rule B006 to reproduce this:

# /tmp/bad.py
def add_to_list(item, some_list=[]):
    some_list.append(item)
    return some_list

At HEAD (df807ff) and v0.1.1:

❯ cargo run -p ruff_cli -- check --select B --unsafe-fixes /tmp/bad.py --no-cache
/tmp/bad.py:1:33: B006 Do not use mutable data structures for argument defaults
Found 1 error.

I've bisected this to 22e1874, but that may only be where the issue was surfaced in the CLI.

The output is slightly different in that commit, there's an additional line:

/tmp/bad.py:1:33: B006 Do not use mutable data structures for argument defaults
Found 1 error.
[*] 0 fixable with the --fix option.

If I omit --unsafe-fixes there's a hint about a hidden fix:

❯ cargo run -p ruff_cli -- check --select B /tmp/bad2.py --no-cache
/tmp/bad.py:1:33: B006 Do not use mutable data structures for argument defaults
Found 1 error.
1 hidden fix can be enabled with the `--unsafe-fixes` option.

(I don't see this hint in 0.1.1 or HEAD, but I haven't bisected where it went away.)

In the parent of the bisected commit and in v0.0.292, I get the expected output:

/tmp/bad.py:1:33: B006 [*] Do not use mutable data structures for argument defaults
Found 1 error.
[*] 1 potentially fixable with the --fix option.

(I noticed after the fact that I ran Ruff from its repository which has Ruff config in pyproject.toml, so that could have skewed results a little. But the repro should still be valid.)

[cjolowicz]

FWIW the fix is deemed unapplicable in FixableStatistics::try_from

ruff/crates/ruff_cli/src/printer.rs

Line 521 in df807ff

// Do not include unapplicable fixes at other levels that do not provide an opt-in

Here's what the diagnostics look like:

Diagnostics {
    messages: [
        Message {
            kind: DiagnosticKind {
                name: "MutableArgumentDefault",
                body: "Do not use mutable data structures for argument defaults",
                suggestion: Some("Replace with `None`; initialize within function")
            },
            range: 32..34,
            fix: Some(Fix {
                edits: [Edit { range: 32..34, content: Some("None") },
                        Edit { range: 37..37, content: Some("    if some_list is None:\n        some_list = []\n") }],
                applicability: Display,
                isolation_level: NonOverlapping
            }),
            file: SourceFile {
                name: "/tmp/bad.py",
                code: "def add_to_list(item, some_list=[]):\n    some_list.append(item)\n    return some_list\n" },
            noqa_offset: 32 }],
    fixed: FixMap({}),
    imports: ImportMap { module_to_imports: {} },
    notebook_indexes: {}
}

[cjolowicz]

I think I understand what's going on now.

#6131 added the fix as a "manual edit", a concept which later became applicability: Display. This means that the fix should only be displayed to the user but never applied by Ruff. Before 22e1874 (the bisected commit), applicability levels weren't respected, which explains why the fix was available before 0.1.0 and then went away.

So I suppose this issue is a feature request, not a bug:

• Can we move the fix from display to unsafe (or even safe)?

I haven't found discussion of this (but may have missed it).

[cjolowicz]

For context, I see three rules with display edits in the codebase, which are likely all affected by the change:

• mutable-argument-default (flake8-bugbear)
• lambda-assignment (pycodestyle)
• commented-out-code (eradicate)

These rules are all marked as fixable in the docs, which is prone to create confusion? (Did for me.)

[charliermarsh]

I support upgrading this to unsafe -- we could probably increase all of those to unsafe now that it's supported on the CLI.

[charliermarsh]

(But also, we probably shouldn't mark "display" rules as fixable in the docs.)

[zanieb]

(But also, we probably shouldn't mark "display" rules as fixable in the docs.)

We do not know applicability levels of fixes when generating the documentation. Perhaps FixAvailability needs a maximum applicability field.

I'll need to look at those specific rules but I presume they were categorized as display-only for good reason?

Note after #7352 these fixes will be displayed instead of entirely hidden.

[cjolowicz]

I opened #8108 to promote mutable-argument-default to unsafe.

I'll need to look at those specific rules but I presume they were categorized as display-only for good reason?

I found some discussion of this, after all:

• [flake8-bugbear] add autofix for B006 mutable_argument_default #4880 (comment)
• [flake8-bugbear] add autofix for B006 mutable_argument_default #4880 (comment)

To my understanding, the rule was left in manual because of an edge case where the newline between the function signature and the body is escaped with a backslash (second link above). But it seems like there was some question whether this edge case justified making the fix display-only.

cc @qdegraaf @MichaReiser

(As a reminder for myself, the reason the fix isn't safe is because mutable argument defaults can be used intentionally, e.g. for caching.)

[zanieb]

It looks like it was marked as display/manual because it could result in invalid syntax if a continuation is used at the end of a function definition? Seems like a weird enough edge-case to just be unsafe.