improve error message when resolution via Deref actually required DerefMut

[pnkfelix]

If a type B implements Deref to get out &A, then auto-deref based method resolution of instances of B will include methods of A that require &mut self, even when there is no DerefMut that would be necessary for such auto-deref to work.

We still reject the code, with the message "cannot borrow immutable borrowed content as mutable"

What I would prefer is for the message to say something like:

error: cannot do mutable borrow via Deref alone
note: would need (at least) impl DerefMut for A

• (i included some weasel language in the above that might be better editted out of a good error report.)

(I think @nikomatsakis is well aware of this problem; he's mentioned the issue about how the auto-deref code needs to use a conservative guess during method resolution and rely on later compiler stages to reject the method lookup.)

Here is an example of code exhibiting the problem playpen:

use std::ops::Deref;

struct A(i32);
struct B(A);

impl A {
    fn yow(&mut self) { println!("Hi"); }
}

impl Deref for B {
    type Target = A;
    fn deref(&self) -> &Self::Target { &self.0 }
}

fn main() {
    let mut b = B(A(3));
    b.yow();
}

The error message when you run this (on nightly) is:

<anon>:17:5: 17:6 error: cannot borrow immutable borrowed content as mutable
<anon>:17     b.yow();
              ^
error: aborting due to previous error

This message can be frustrating, especially when the content appears mutable at the point that is highlighted by the span of the error message; after all, I did use let mut b = ...

[nikomatsakis]

Definitely it'd be better to report something like "Values of type B cannot be mutably dereferenced", or "Do not implement DerefMut".

[nikomatsakis]

This should be eminently doable though -- we start out with a Deref impl and come back after the fact to "bias" it towards &mut self. I'll put myself down as mentor. I imagine the hardest part here is probably selecting what message to emit.

[mkpankov]

I'm willing to take this. Where should I look first? This is going to be my first issue.

[nikomatsakis]

@mkpankov if you can ping me on irc (nmatsakis), we can chat live, otherwise i'll try to come up with some links in a day or two

[nagisa]

I hit this today. Took me a while to figure out where the problem was.

[ryanq]

@nikomatsakis I'm interested in working on this issue to get my feet wet working with rustc. Any pointers/links for where I should start looking? I'll be looking at the source to see what I can find on my own.

[nikomatsakis]

@ryanq hmm. That error occurs in the "borrow check" code (librustc_borrowck). I think specifically in the gather_loans pass, iirc. That pass walks a data structure called a cmt (category-mutability-type) that basically breaks down a path. I think from the cmt we could deduce that what you have is an mutable borrow of something that is immutable due to the fact that it is using an overloaded Deref operator, and hence tailor the message. That's all off the top of my head -- maybe you can ping me on IRC and we can setup some time to chat a bit more in depth? (nmatsakis is my nick)

[arielb1]

The error is caused by try_overloaded_autoderef falling back to Deref::deref when DerefMut is not available. We should probably cause a type error instead.

[caipre]

I'm interested in trying this.

I've just started at the Recurse Center and making contributions to Rust, within the type system especially, is one of my goals. So this might be perfect. I may need mentorship, but let's see if I can get anywhere on my own first.

[brson]

@caipre Oh awesome! Sorry not to get back to you for so long. @nikomatsakis or @arielb1 can probably help mentor.

[caipre]

@brson Thanks, and no worries. I've been reacquainting myself with Rust since I've been away from it for a bit; my head hurts. 😄 I've made a few smaller contributions to the project but nothing this close to the compiler. I'll probably reach out sometime next week for some guidance.

[nikomatsakis]

@caipre I would indeed be hapy to mentor. Let me give a tips to get you started. I think @arielb1 was basically right in this comment, though the names may have shifted in the meantime.

The thing about how this works in the compiler is that a lot of times we have to deref a pointer before we know whether it will be a "mut deref" (i.e., via the DerefMut trait) or a "shared deref" (i.e., via the Deref trait). For example, imagine you have vec: Box<Vec<u8>> and we do vec.last(). Box doesn't have a last method so here we would first deref vec (i.e., search for a Deref impl) and find out that it derefs to Vec<u8>. At that point, we find the last method and it is an &self method so we're all good.

OK, now imagine vec.push(22) -- again we have to deref the type of vec to get Vec<u8> before we can find the push method. So we start out with Deref as before. But then we find out that push is &mut self -- so we have kind of go back and try to find a DerefMut implementation at that point.

There are various places where this happens but they all wind up at the method try_overloaded_deref in src/librustc_typeck/check/autoderef.rs. You can see that it takes an argument lvalue_pref: LvaluePreference -- this basically indicates whether we need a DerefMut implementation or whether we are looking for a Deref impl.

Currently, that method is written to "fallback" to a Deref impl if no DerefMut impl is available. I think this is a holdover from an earlier scheme where we sometimes would try to "guess" in advance that we would want DerefMut (but that guess might be wrong). In any case, as far as I can tell, if we ever have a PreferMutLvalue preference and we do not find a DerefMut impl, I think we are guaranteed to have an error down the line.

I'm not quite sure however how best to fix this. Perhaps the best thing is for it to report an error and then continue as it does today, but I feel a bit uncomfortable with that -- this routine's name suggests that it just "looks" for a method but doesn't commit to the choice yet. In other words, while I don't think we do any probing about for a DerefMut impl today, I could imagine us doing so, and hence reporting an error might be a bit eager.

I feel like the thing I would most expect is for the method to return None, but we would then have to make sure and adjust the other callers to handle a None return value correctly. I suspect many of them simply assume that Some will be returned, since we know that we found a Deref impl before and that the fn falls back to a Deref impl.