Rust-clippy: Passing mutable references

[Pyriphlegethon]

This issue already has a lint, only the second one still needs fixing

Passing a mutable reference to a function that takes an immutable reference:

fn takes_an_immutable_reference(a: &i32) -> bool {
    *a > 10
}

fn main() {
    let mut five = 5;
    println!("{}", takes_an_immutable_reference(&mut five));
}

If I'd find this code in somebody's code base I'd think that the function will mutate the argument and I'd have to make a copy if I want to preserve the original value. This lint should only be applied if the argument is actually passed as &mut, the following code should be ok:

fn takes_an_immutable_reference(a: &i32) -> bool {
    *a > 10
}

fn main() {
    let mut five = 5;
    let five_ref = &mut five;
    println!("{}", takes_an_immutable_reference(five_ref));
}

Taking a mutable reference and not mutating it:

fn takes_a_mutable_reference(a: &mut i32) -> bool {
     *a > 10
}

The rust compiler will complain whenever it sees a mutable binding that is not mutated. Strangely enough, it won't complain about mutable arguments that are never mutated. I guess this might be a hard problem?

Thanks for rust-clippy! It's an awesome tool!

[Manishearth]

I like this idea. Though I'm surprised the compiler doesn't complain here.

If I'd find this code in somebody's code base I'd think that the function will mutate the argument and I'd have to make a copy if I want to preserve the original value.

It's stuff like this which makes me love Rust. :smile:

Thanks for rust-clippy! It's an awesome tool!

You're welcome! Glad you enjoyed it!

[llogiq]

We should tread carefully here. The mut is part of the interface, so implementing a trait may well require it. Also there may be situations in which we want to rule out other aliases despite not mutating an instance.

The former can be detected, the latter probably not, short of a mind-reading extension to clippy.

[Manishearth]

This isn't about &mut Foo types in parameter lists, it's about foo(x, y, &mut z) where &mut z is not necessary.

[birkenfeld]

The other lint is about taking a mutable reference and not mutating it:

This may be problematic though, as @llogiq says. At least trait impls should be excluded, and for pub items the public API would change. (Although IIUC taking &T instead of &mut T wouldn't lead to compile errors, but only clippy warnings :smile:)

[Manishearth]

Oh, this is two lints, right.

[Pyriphlegethon]

Yeah, didn't know that a trait impl couldn't strengthen the constraints for its arguments. That really seems like it should be allowed.
The easy fix would be to not check any trait impls like @birkenfeld said.

[birkenfeld]

So @Pyriphlegethon, are you interested in working on these lints? (just asking to avoid duplicated work, there's no obligation :smile:)

[Pyriphlegethon]

@birkenfeld I'm already working on them, but it'll need some time to get used to the compiler internals. The first lint is not going to be very hard as soon as I figure out how to get the definition of a function I only have the path of. I'll ask if I need some help.

I assume the second lint can be done very similar to this.

[birkenfeld]

Great! We'll be happy to help.

[Pyriphlegethon]

So, here I am. I tried getting the type of the function that is called in order to verify that every mutable reference that is passed needs to be mutable. I think I found a way to get the type of the function. It seems very ugly but works:

impl LateLintPass for UnnecessaryMutPassed {
    fn check_expr(&mut self, cx: &LateContext, e: &Expr) {
        if let &ExprCall(ref fn_expr, ref arguments) = &e.node {
            println!("{:?}", cx.tcx.tables.borrow().node_types.get(&fn_expr.id).unwrap().sty);
        }
    }
}

This code will print out some weird enum that contains the necessary information. It is defined here, the important variant is this one.

The problem is that rustc doesn't want me to import this enum because it is allegedly private. That probably means that I'm doing things wrong, doesn't it? Is there a less ugly way to get the type of a function?
Also, I really need an IDE with a "Jump to definition" function :smiley:

[Manishearth]

Those enum variants are all public, we use them all over clippy.

http://manishearth.github.io/rust-internals-docs/rustc/middle/ty/enum.TypeVariants.html#variant.TyEnum

[Pyriphlegethon]

Yeah, that was strange. I had to import it from some other module that re-exported it, but at least I got the first lint to work. I will send the PR tomorrow after a bit of testing.

[oli-obk]

I'm gonna take this lint. I just spend 15 minutes chasing 5 functions in circles in miri to check why in the world they mutate an argument only to figure out that all of them could take a &T instead of a &mut T without any change in behaviour. Apparently I didn't get to this... And I ran into the same problem... again... in miri...

// increase this counter if you ran into this problem again and still didn't fix it: 3

[RalfJung]

The other lint is about taking a mutable reference and not mutating it:

This may be problematic though, as @llogiq says. At least trait impls should be excluded, and for pub items the public API would change. (Although IIUC taking &T instead of &mut T wouldn't lead to compile errors, but only clippy warnings 😄)

Not just that; when there is unsafe code around, the &mut may be necessary to preserve safety of the API. The following version of as_mut_slice compiles, but is wrong:

    pub fn as_mut_slice<T>(self_: &Vec<T>) -> &mut [T] {
        unsafe {
            from_raw_parts_mut(self_.as_ptr() as *mut T, self_.len())
        }
    }

[oli-obk]

self_.as_ptr() as *mut T

isn't that wrong in general, irrelevant of the mutability of self_?

[RalfJung]

Why should it be? The distinction between *const T and *mut T is just linting.

[L0uisc]

I came here from TWIR call for participation. Isn't it better to put the second part of the issue directly into rustc?