-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Open
Labels
C-enhancementCategory: Enhancement of lints, like adding more cases or adding help messagesCategory: 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 onCall for participation: This a hard problem and requires more experience or effort to work on
Description
(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?
Metadata
Metadata
Assignees
Labels
C-enhancementCategory: Enhancement of lints, like adding more cases or adding help messagesCategory: 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 onCall for participation: This a hard problem and requires more experience or effort to work on
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
ebroto commentedon Nov 26, 2020
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.
iago-lito commentedon Nov 26, 2020
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 commentedon Nov 26, 2020
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.
xFrednet commentedon Dec 5, 2020
I could reproduce the suggestion with the full type specification in the Rust Playground. The message that this suggestion most likely originates from is:
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 commentedon Dec 7, 2020
@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:Run
consider changing this to be a mutable reference: `&mut Vec<u8>`
: simple type spec.Tools -> Clippy
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 withclippy
.xFrednet commentedon Dec 7, 2020
Ohhh yes you're completely right. I mixed up the output from
Run
andTools -> 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