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.

[nikomatsakis]

Reading through the code, I definitely think the most minimal edit would be to continue behaving as it does but report an error if a DerefMut is requested and not found. However, I still think that's an odd structure, since the fn would report an error in one case (DerefMut requested but not found) but not another (Deref requested but not found). I think the right fix would probably be to return None and then adjust the callers accordingly -- or maybe a more convenient way might be to return Result<Callee, Error> where the error code can distinguish between "no impl found at all" or "requested DerefMut but found Deref, here's the data". The nice thing about using Result is that you could probably consolidate the error reporting code into one helper, since you have all the context you need to do so.

One other bit of context you may be missing: the MethodCallee that gets returned contains the data about what method is being called etc. It is considered "unconfirmed" until it is stored in some central tables via a call like this one (from method/confirm.rs):

self.tables.borrow_mut().method_map.insert(method_call, method);

It's...probably fine to skip doing this insert in the case of "requested deref-mut but found deref", but it may cause ICEs, I'm not sure. So I could imagine that if you did the Result behavior as I am suggesting, then the "report deref error" routine could probably just take the Result<...> and, in the event of success, store the value in the table. In the event of "fallback error", the error could still carry the data for the Deref impl, and we could store that (so that some data is present) but also report an error. This would preserve the existing behavior a bit more closely. (But I think it's not needed, as those tables don't get examined until after typeck is done -- still, if we ever want to "carry on" with compilation after typeck, it might be good to have an entry there -- though in that case we'd probably want an "error placeholder", to be honest.)

[caipre]

FYI, started on this today after taking some time to re-acquaint myself with Rust. (Okay, I was distracted by another project for a bit as well). I think I understand the gist of what's happening and how to proceed. I'm definitely missing any level of comfort with the rust codebase, eg,

• "how do I report an error?"
• "how do I choose / what is the largest current error number?"

Will reach out if I get really stuck.

[shepmaster]

@nikomatsakis do you think this code is a symptom of the same problem, or should I file it separately?

fn main() {
    let mut fns: Vec<Box<FnMut()>> = vec![Box::new(|| println!("called"))];

    (fns[0])(); // error: expected function, found `Box<std::ops::FnMut()>`
    (*&mut fns[0])();
}

It appears to be incorrectly using Deref instead of DerefMut; the explicit &mut forces the use of DerefMut.

[caipre]

Still working on this. I've managed to add a warning that fires when a DerefMut impl is missing (see my branch), but it's yielding some false positives when compiling: https://gist.github.com/caipre/eada8f32b2555a9240a2dd709d97532a

I talked with @nikomatsakis briefly on IRC and he suggested that we check whether we're returning into a *mut, and drop the warning if so. I'm not sure how to make that check...

[nikomatsakis]

@shepmaster

do you think this code is a symptom of the same problem, or should I file it separately?

I think that's a separate bug -- did you wind up filing it?

[shepmaster]

@nikomatsakis I have now: #36786

[abonander]

This doesn't just seem to be a diagnostics issue, but also possibly a coercion logic issue with the call operator: #51157 (comment)

[Mark-Simulacrum]

The error today is:

error[E0596]: cannot borrow data in a dereference of `B` as mutable
  --> src/main.rs:17:5
   |
17 |     b.yow();
   |     ^ cannot borrow as mutable
   |
   = help: trait `DerefMut` is required to modify through a dereference, but it is not implemented for `B`

That seems pretty much perfect to me, so I'm going to close this (a minor improvement might be to note that Deref is implemented, but that seems like we don't need an issue to track it).

[Ghost543]

try this

impl std::ops::DerefMut for B {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}