Improve method lookup auto-deref behavior (subissue of trait reform)

[eddyb]

Previously to #12491, there were no real issues with the current method lookup implementation, which collects candidates and picks the first one in the auto-deref chain, without caring too much about mutability.

The problematic case #12491 introduces was (*x).foo() where *x is an overloaded dereference, calling either Deref::deref or DerefMut::deref_mut.

However, the method could take &mut self - which we can't know until we've picked one of deref or deref_mut and continued to the lookup of .foo.

The choice I've made was to always try deref_mut (if not obviously immutable, e.g. x having a type of &T instead of &mut T or T) first, and that will work for RefCell's RefMut, which only had a mutable .get() method previously.

In #12610 it gets worse as the issue can happen at any auto-deref level, and here's a test case that fails to compile:

// Generic unique/owned smaht pointer.
struct Own<T> {
    value: *mut T
}

impl<T> Deref<T> for Own<T> {
    fn deref<'a>(&'a self) -> &'a T {
        unsafe { &*self.value }
    }
}

impl<T> DerefMut<T> for Own<T> {
    fn deref_mut<'a>(&'a mut self) -> &'a mut T {
        unsafe { &mut *self.value }
    }
}

struct Point {
    x: int,
    y: int
}

impl Point {
    fn get(&self) -> (int, int) {
        (self.x, self.y)
    }
}

fn test(x: Own<Point>) {
    // This method call will attempt a deref_mut call, which succeeds
    // (because type checking doesn't have the mem_categorization
    // information borrow checking uses to confirm the sanity of such
    // borrows), but causes a borrow error later.
    // Ideally, auto-deref should recognize x as immutable and call
    // deref instead of deref_mut` Also, method lookup could use
    // the self type of the method to pick deref instead of deref_mut.
    let _ = x.get();
}

cc @nikomatsakis

[pnkfelix]

cc me

[alexcrichton]

Another test case:

use std::cell::RefCell;

fn main() {
    let a = RefCell::new((1i, 2i));
    match *a.borrow_mut() {
        (ref mut a, ref mut b) => { *a += *b; *b += *a; }
    }
}

This fail to compile, but if it's changed to *a.borrow_mut().deref_mut() it compiles just fine.

[nikomatsakis]

I've been working, by the way, on a prototype of a new trait / method resolution algorithm that resolves issue (among many others). I hope to include it in a general RFC for #5527 very shortly.

[nrc]

Same issue with Index/IndexMut, ftr.

[alexcrichton]

Nominating, if this is going to block vec switching to []-syntax I believe this is a 1.0 blocker.

[pnkfelix]

Assigning 1.0 milestone, P-backcompat-lang.

[pcwalton]

I'm going to go ahead and assign this to @nikomatsakis.

[pcwalton]

We need to change precedence so that inherent and trait impls are on equal footing instead of having inherent impls be first.

[P1start]

Is this really fixed? @alexcrichton’s example from above still fails to compile, and is incidentally very similar to the example in #15609, which also does not compile.

[gifnksm]

impl<T> IndexMut<uint, T> for Vec<T> is commented out with refereeing this issue.
Should this issue be reopend or un-comment the impl?

[aturon]

I've confirmed that IndexMut is still not working properly, so we still can't provide full indexing notation on vectors. Note that this is why this issue was nominated and approved as a 1.0 blocker.

@pcwalton, would you prefer me to re-open this issue or create a new one?

[aturon]

Reopening.