Skip to content

Don't prefix full path specification when suggesting changing & to &mut. #6385

@iago-lito

Description

@iago-lito

(moved (again) from rust-lang/rust#79405)

With the following code:

fn f(vec: &Vec<usize>) {
    vec.push(5);
}

I get a suggestion to "change this to a mutable reference", but if I do, I get:

fn f(vec: &mut std::vec::Vec<usize>) {
    vec.push(5);
}

That's a bit overkill, right? I would rather expect

fn f(vec: &mut Vec<usize>) {
    vec.push(5);
}

It actually becomes worse. I've decided to file this bug report after my argument:

shells: &LocatedShells,

was changed to

shells: &mut topology::located_slice::LocatedBoxedSlice<std::collections::HashMap<usize, shell::Shell, std::hash::BuildHasherDefault<fnv::FnvHasher>>>,

instead of just

shells: &mut LocatedShells,

That's definitely overkill, so I end up changing the & to &mut by hand.
Is this something expected? Or something I can configure not to happen?

Activity

ebroto

ebroto commented on Nov 26, 2020

@ebroto
Member

Clippy does not have the ability to add imports, so we generally suggest the fully-qualified path to avoid generating compile errors.

That being said, if I'm not mistaken rustc used to have a similar problem which was solved somehow (e.g. Vec is not fully-qualified anymore if not needed) so we could check how this was solved upstream.

If we manage to use the same solution, it should be applied generally as this lint is not the only case where we do that.

added
E-mediumCall for participation: Medium difficulty level problem and requires some initial experience.
C-enhancementCategory: Enhancement of lints, like adding more cases or adding help messages
on Nov 26, 2020
iago-lito

iago-lito commented on Nov 26, 2020

@iago-lito
Author

Well, I'm curious about the process here, because I don't understand why the ability to add imports interferes :(

My understanding is that types are checked before mutability. If this is correct, then this lint appearing already means that the type is correctly qualified, so there is no need to query the current namespace, and just changing & to &mut should never generate a compile error, right? What am I missing?

ebroto

ebroto commented on Nov 26, 2020

@ebroto
Member

Oh my bad, ignore me, I confused this case with the case where we suggest something that may not be on scope.

This should be doable.

removed
E-mediumCall for participation: Medium difficulty level problem and requires some initial experience.
on Nov 26, 2020
xFrednet

xFrednet commented on Dec 5, 2020

@xFrednet
Member

I could reproduce the suggestion with the full type specification in the Rust Playground. The message that this suggestion most likely originates from is:

error[E0596]: cannot borrow `*me` as mutable, as it is behind a `&` reference
 --> src/main.rs:3:5
  |
2 | fn not_using_mut(me: &Vec<u8>) {
  |                      -------- help: consider changing this to be a mutable reference: `&mut Vec<u8>`
3 |     me.push(4);
  |     ^^ `me` is a `&` reference, so the data it refers to cannot be borrowed as mutable

error: aborting due to previous error

I couldn't find the message in the clippy code base. However, I found it in the rustc code in diagnostics/mutability_errors.rs line 390. So, I would think that this actually comes from the rust compiler. (I've also commented this on the rust issue)

iago-lito

iago-lito commented on Dec 7, 2020

@iago-lito
Author

@xFrednet Well, I'm confused, since I don't actually read the full type spec in your playground, or in your snippet, I read &mut Vec<u8> where full type spec would be &mut std::vec::Vec<u8>.

This said, I can reproduce in your playground if I eventually click clippy, so I think the problem does belong to this repo, here's what happens to me:

  • navigate to your playground
  • click Run
    • -> error appears: consider changing this to be a mutable reference: `&mut Vec<u8>` : simple type spec.
  • click Tools -> Clippy
    • -> error appears: consider changing this to be a mutable reference: `&mut std::vec::Vec<u8>` : full type spec.

So I think the problem does not occur with rustc while it does occur with clippy.

xFrednet

xFrednet commented on Dec 7, 2020

@xFrednet
Member

Ohhh yes you're completely right. I mixed up the output from Run and Tools -> Clippy because they looked so similar. Thank you for pointing that out @iago-lito!

Than it's most likely a problem with Clippy or with the combination of the two. I'll try to look into this in a week or two. Someone else can also take this issue if it sounds interesting :)

Update: I didn't look further into this. This issue can be taken up if someone is interested

added
E-hardCall for participation: This a hard problem and requires more experience or effort to work on
on Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-enhancementCategory: Enhancement of lints, like adding more cases or adding help messagesE-hardCall for participation: This a hard problem and requires more experience or effort to work on

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @ebroto@camsteffen@iago-lito@xFrednet

      Issue actions

        Don't prefix full path specification when suggesting changing & to &mut. · Issue #6385 · rust-lang/rust-clippy