Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing mutable references #353

Open
Pyriphlegethon opened this issue Sep 28, 2015 · 17 comments

Comments

@Pyriphlegethon
Copy link
Contributor

@Pyriphlegethon Pyriphlegethon commented Sep 28, 2015

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

This comment has been minimized.

Copy link
Member

@Manishearth Manishearth commented Sep 28, 2015

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. 😄

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

You're welcome! Glad you enjoyed it!

@llogiq

This comment has been minimized.

Copy link
Collaborator

@llogiq llogiq commented Sep 28, 2015

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 comment was marked as outdated.

Copy link
Member

@Manishearth Manishearth commented Sep 28, 2015

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

This comment has been minimized.

Copy link
Contributor

@birkenfeld birkenfeld commented Sep 28, 2015

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 😄)

@Manishearth

This comment was marked as outdated.

Copy link
Member

@Manishearth Manishearth commented Sep 28, 2015

Oh, this is two lints, right.

@Pyriphlegethon

This comment has been minimized.

Copy link
Contributor Author

@Pyriphlegethon Pyriphlegethon commented Sep 28, 2015

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

This comment was marked as outdated.

Copy link
Contributor

@birkenfeld birkenfeld commented Sep 28, 2015

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

@Pyriphlegethon

This comment was marked as outdated.

Copy link
Contributor Author

@Pyriphlegethon Pyriphlegethon commented Sep 28, 2015

@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

This comment was marked as outdated.

Copy link
Contributor

@birkenfeld birkenfeld commented Sep 28, 2015

Great! We'll be happy to help.

@Pyriphlegethon

This comment was marked as outdated.

Copy link
Contributor Author

@Pyriphlegethon Pyriphlegethon commented Sep 28, 2015

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 😃

@Manishearth

This comment was marked as outdated.

Copy link
Member

@Manishearth Manishearth commented Sep 28, 2015

@Pyriphlegethon

This comment was marked as outdated.

Copy link
Contributor Author

@Pyriphlegethon Pyriphlegethon commented Sep 29, 2015

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

This comment has been minimized.

Copy link
Collaborator

@oli-obk oli-obk commented Feb 22, 2017

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

This comment has been minimized.

Copy link
Member

@RalfJung RalfJung commented Sep 13, 2017

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

This comment was marked as resolved.

Copy link
Collaborator

@oli-obk oli-obk commented Sep 13, 2017

self_.as_ptr() as *mut T

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

@RalfJung

This comment was marked as resolved.

Copy link
Member

@RalfJung RalfJung commented Sep 13, 2017

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

@L0uisc

This comment has been minimized.

Copy link

@L0uisc L0uisc commented Oct 17, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.