fix(unnecessary_mut_passed): retain parens around the arguments

[ada4a]

Fixes #15722

changelog: [unnecessary_mut_passed]: retain parens around the arguments

[rustbot]

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Alexendoo]

Instead of building up a suggestion string as it does currently it would be nice to span only the mut part of &mut and replace it with "" to improve the diagnostic

I'm happy to merge the PR as is though, just let me know either way

[ada4a]

I did try doing that:

Implementation

let span_to_remove = {
    let span_until_arg = argument.span.until(arg.span);
    let Some(ref_pos) =
        span_until_arg.with_source_text(cx, |src| src.find('&').expect("a reference must contain `&`"))
    else {
        return;
    };
    let lo = ref_pos + '&'.len_utf8();
    span_until_arg.with_lo(BytePos::from_usize(lo))
};

span_lint_and_then(
    cx,
    UNNECESSARY_MUT_PASSED,
    argument.span,
    format!("the {fn_kind} `{name}` doesn't need a mutable reference"),
    |diag| {
        diag.span_suggestion(span_to_remove, "remove this `mut`", String::new(), applicability);
    },
);

But:

• The diagnostic ended up looking just a bit too crowded imo?

  error: the method `takes_ref` doesn't need a mutable reference
    --> tests/ui/unnecessary_mut_passed.rs:188:25
     |
  LL |     my_struct.takes_ref((&mut 42));
     |                         ^^----^^^
     |                           |
     |                           help: remove this `mut`
  

• More importantly, diving into the source is inherently brittle I think. For example, if we add handling of &raw references in the future, the implementation above would start eating the raw part as well, which would of course be no good. That said, building the expression from scratch is not hugely better -- it will, in theory, destroy all the custom formatting and comments -- though to be fair one wouldn't really expect to see either of these in this context.

Given all that, I'm kind of undecided, maybe leaning a bit towards the "cut out the mut" approach -- WDYT?

[Alexendoo]

If you ever see a crowded diagnostic swap it out for a span_suggestion_verbose, long term that's intended to be the default (rust-lang/rust#141973)

Yeah it's unfortunate it can't be done solely with spans, but has_enclosing_paren also requires examing the source I guess. You could do .find("&mut") to guarantee you've found it

I wouldn't worry about a future addition of &raw handling as they'd have to adapt it either way

[ada4a]

long term that's intended to be the default (rust-lang/rust#141973)

Good to know!

You could do .find("&mut") to guarantee you've found it

I'm a bit worried that that would make us miss weirdly formatted things. I could add a debug assertion that the part after & does actually contain a mut, but I wish we could issue some kind of a warning about that even in release builds.

[ada4a]

Oh no, apparently the implementation above completely wrecks ui_test and/or rustfix:

error: failed to apply suggestions for tests/ui/unnecessary_mut_passed.rs with rustfix
could not replace range 1..1584, maybe parts of it were already replaced?
Add //@no-rustfix to the test file to ignore rustfix suggestions

full stderr:
<JSON representation of the error, for some bizarre reason>

At first I thought that that was due to the test cases with multiple suggestions on the same line, like (&mut my_struct).takes_ref(&mut 42);, but even removing that doesn't help.

Any ideas?...

[Alexendoo]

I'm a bit worried that that would make us miss weirdly formatted things

I thought &mut was one token but yeah sure you could look for mut instead since & mut is valid apparently

I could add a debug assertion that the part after & does actually contain a mut, but I wish we could issue some kind of a warning about that even in release builds.

We don't want to panic when the source doesn't seem to make sense because it often doesn't in the presence of proc macros that use set_span, checking for the actual mut token does ensure that we don't lint in those cases also

1..1584 sounds like a pretty large range, I would guess something went wrong with a span calculation

[ada4a]

We don't want to panic when the source doesn't seem to make sense because it often doesn't in the presence of proc macros that use set_span, checking for the actual mut token does ensure that we don't lint in those cases also

Makes sense

1..1584 sounds like a pretty large range, I would guess something went wrong with a span calculation

Not only is the span wrong, but it isn't even on the right file?... Here's the JSON representation of the very first suggestion: (EDIT2: figured it out, see below)

{
  "message": "remove this `mut`",
  "code": null,
  "level": "help",
  "spans": [
    {
      "file_name": "/home/ada4a/dev/minor/clippy/tests/clippy.toml",
      "byte_start": 1,
      "byte_end": 1524,
      "line_start": 1,
      "line_end": 57,
      "column_start": 2,
      "column_end": 20,
      "is_primary": true,
      "text": [],
      "label": null,
      "suggested_replacement": "",
      "suggestion_applicability": "MachineApplicable",
      "expansion": null
    }
  ],
  "children": [],
  "rendered": null
}

Huh?...

EDIT: the diagnostic span is at least in the right file though:

{
  "$message_type": "diagnostic",
  "message": "the function `takes_ref` doesn't need a mutable reference",
  "code": {
    "code": "clippy::unnecessary_mut_passed",
    "explanation": null
  },
  "level": "error",
  "spans": [
    {
      "file_name": "tests/ui/unnecessary_mut_passed.rs",
      "byte_start": 1420,
      "byte_end": 1427,
      "line_start": 57,
      "line_end": 57,
      "column_start": 15,
      "column_end": 22,
      "is_primary": true,
      "text": [
        {
          "text": " takes_ref(&mut 42);",
          "highlight_start": 15,
          "highlight_end": 22
        }
      ],
      "label": null,
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "expansion": null
    }
  ],
  // snip

[ada4a]

So the reason for the bug was that I was using span.with_lo(1), thinking that 1 is the relative position in the span, but it of course was the absolute position (..in another file -- this part I still don't understand) -- the solution was to use span.split_at(1).1 instead

(Sorry for all the spam, just writing this down so that I hopefully remember it for the future debugging)

[github-actions]

Lintcheck changes for 9cb82d7

Lint	Added	Removed	Changed
clippy::unnecessary_mut_passed	0	0	1

This comment will be updated if you push new changes

[Alexendoo]

Thanks!

View changes since this review