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

Implementations of traits for &mut T yields surprises about mutability #12381

Closed
chris-morgan opened this issue Feb 19, 2014 · 2 comments
Closed

Comments

@chris-morgan
Copy link
Member

Here are a couple of minimal example of the situation:

trait Trait {
    fn b(&mut self) {}
}
struct Struct;
impl<'a> Trait for &'a mut Struct {}

#[allow(dead_code)]
fn foo(mut a: &mut Struct) {
    a.b();
}
fn main() {}
trait TraitA {}
trait TraitB {
    fn b(&mut self) {}
}

impl<A: TraitA> TraitB for A {}
impl<'a> TraitA for &'a mut TraitA {}

#[allow(dead_code)]
fn foo(mut a: &mut TraitA) {
    a.b();
}
fn main() {}

Now why was mut required on a in both cases? Because otherwise, the call to a.b() won't work: error: cannot borrow immutable argument a as mutable.

The reason why this is so is that the trait implementation is on &mut T, and so that (&mut T) is an opaque type for the purposes of the trait implementation, having no special mutability as it would otherwise have. Then, when calling the b method, it will take a second borrow &mut (&mut T). But in order to do that, it not having special knowledge of the fact that &mut T is considered mutable already, it requires you to place it in a mutable slot.

The real-life case where I hit this a couple of weeks ago and where someone else in IRC hit it today is implementing additional methods on Writer or Reader and then using the trait objects:

trait ReaderUtils: Reader {
    fn read_foo(&mut self) -> Foo {
        unimplemented!();
    }
}
impl<R: Reader> ReaderUtils for R {}

fn unfoo(mut r: &mut Reader) {
    let _ = r.read_foo();
}

In this case, all of a sudden read_foo is a second-class citizen, behaving differently from the standard Reader methods. If you don't use such a method, mut r will warn you about unnecessary mutability. If you do use such a method, then all of a sudden you must use mut r.

(I'm not sure if there is a real use case for implementing traits on &mut T other than trait objects, but it's possible. It's more the trait objects that I care about.)

This is not incorrect behaviour—it is consistent; it is merely surprising behaviour. Thus, if it can be improved, it would be good. I'm not sure whether or not it can be improved, because I suspect that any special detection of a &mut T implementation as mutable would just defer the problem to the next level of generic bounds.

@nikomatsakis
Copy link
Contributor

We discussed this for some time and eventually decided it wasn't worth having another kind of pointer to handle this sort of situation (which is essentially what winds up being needed -- well, that or implicitly considering all locals to be mutable, which was my preferred solution for a time). I'm going to close.

@chris-morgan
Copy link
Member Author

I'm not surprised by the decision here; it's what I expected. I was mostly just filing it so that it has been reported, and for future reference when other people strike this strange behaviour.

pcwalton added a commit to pcwalton/rust that referenced this issue Aug 14, 2014
by-reference upvars.

This partially implements RFC 38. A snapshot will be needed to turn this
on, because stage0 cannot yet parse the keyword.

Part of rust-lang#12381.
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Mar 21, 2024
Have the lint trigger even if `Self` has generic lifetime parameters.

```rs
impl<'a> Foo<'a> {
    type Item = Foo<'a>; // Can be replaced with Self

    fn new() -> Self {
        Foo { // No lifetime, but they are inferred to be that of Self
              // Can be replaced as well
            ...
        }
    }

    // Don't replace `Foo<'b>`, the lifetime is different!
    fn eq<'b>(self, other: Foo<'b>) -> bool {
        ..
    }
```

Fixes rust-lang#12381
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Mar 21, 2024
[`use_self`]: Make it aware of lifetimes

Have the lint trigger even if `Self` has generic lifetime parameters.

```rs
impl<'a> Foo<'a> {
    type Item = Foo<'a>; // Can be replaced with Self

    fn new() -> Self {
        Foo { // No lifetime, but they are inferred to be that of Self
              // Can be replaced as well
            ...
        }
    }

    // Don't replace `Foo<'b>`, the lifetime is different!
    fn eq<'b>(self, other: Foo<'b>) -> bool {
        ..
    }
```

Fixes rust-lang#12381

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`use_self`]: Have the lint trigger even if `Self` has generic lifetime parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants