Getter functions and indexing don't seem to follow the same lifetime rules

[ejpbruel2]

This is my first issue for the Rust project. Apologies up front if it is somehow unclear. I'd be happy to provide any additional information you need.

DESCRIPTION

The issue I'm running into is this. Consider the following code:
collection.get_mut(collection.get(0).unwrap().index).unwrap().index = 1;

The inner call to get causes an immutable borrow on collection. We then obtain a index from the return value, and use that for the outer call to get_mut. This causes a mutable borrow on collection.

The above code compiles just fine. My assumption is that because index implements Copy, NLL allow the immutable borrow on collection to be dropped as soon as we have a copy of index, so we can borrow it mutably again after that.

So far, so good. However, now consider the following code:
collection[collection[0].index].index = 1;

EXPECTED BEHAVIOR

This should be equivalent to the earlier code. In fact, I've implemented the Index trait to forward to get(index).unwrap().

ACTUAL BEHAVIOR

This time, however, the compiler complains:
error[E0502]: cannot borrow collection as immutable because it is also borrowed as mutable

For more context, here is a playground link that illustrates the problem:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=21d19fde2c8c58d4859bfdefa3b7720a

[ejpbruel2]

Cc'ing @nikomatsakis as I've been told he'd be particulary interested in issues like this one.

[hellow554]

Hmm, you're right. This should be possible with NLL, e.g. using

let idx = collection[0].index;
collection[idx].index = 1;

does work.

[pnkfelix]

Hmm I wonder if this is due to some weakness in how we try to limit two-phase borrows to just auto-refs on method calls and indexing. (This was implemented as part fo addressing #48431)

In particular, my hypothesis is that whatever method the compiler is using to pattern-match the code to see if we should attempt to apply two-phase borrows, it is probably getting foiled by the .index projection in the code above.

Generalizing two-phase borrow in some manner is generally tracked in #49434, though I imagine we might attempt to land a more targetted fix for this particular issue sooner than we attempt a more complete generalization.

[pnkfelix]

triage: P-medium.

I am assuming that we will consider widening the semantics of 2-phase borrows in the future in a manner that might allow this code to pass. But such a change in the static semantics is not as high priority as other tasks for the NLL team.

[mbrubeck]

Related thread on the internals forum

In that thread, SNCPlay42 notes that this already works on types that implement the IndexMut trait directly (like [T]), but not on types that DerefMut to an indexable type (like Vec<T>). The same is true for method calls, which similarly don't work when an implicit DerefMut is required. So this isn't actually a difference between indexing and function syntax.

[pnkfelix]

(Sorry for the length of this comment; it is transcribed from #74319 which I where I had mistakenly posted it initially.)

After reading this and related issues, and re-reading:

rust/compiler/rustc_middle/src/ty/adjustment.rs

Lines 135 to 151 in 96859db

	/// At least for initial deployment, we want to limit two-phase borrows to
	/// only a few specific cases. Right now, those are mostly "things that desugar"
	/// into method calls:
	/// - using `x.some_method()` syntax, where some_method takes `&mut self`,
	/// - using `Foo::some_method(&mut x, ...)` syntax,
	/// - binary assignment operators (`+=`, `-=`, `*=`, etc.).
	/// Anything else should be rejected until generalized two-phase borrow support
	/// is implemented. Right now, dataflow can't handle the general case where there
	/// is more than one use of a mutable borrow, and we don't want to accept too much
	/// new code via two-phase borrows, so we try to limit where we create two-phase
	/// capable mutable borrows.
	/// See #49434 for tracking.
	#[derive(Copy, Clone, PartialEq, Debug, TyEncodable, TyDecodable, HashStable)]
	pub enum AllowTwoPhase {
	Yes,
	No,
	}

Here is a summary of my understanding of the situation:

• Some people are claiming that v[IDX] = E (where IDX is an expression that calls a &self-method on v) is rejected, while its desugared form is accepted by two-phase borrows
• Other people are responding that v[IDX] = E doesn't desugar to *v.index_mut(IDX) = E (despite what its documentation says); it actually desugars to *IndexMut::index_mut(&mut v, IDX) = E, and the latter is rejected by the borrow-checker today, regardless of two-phase borrows.
• The docs do claim that desugarings that turn into Foo::some_method(&mut x, ...) syntax are one class of desugarings supported by two-phase borrows, but clearly IndexMut is not placed in that class today.
• mbrubeck's note above indicates that the real issue here has something to do with the interaction with DerefMut; IndexMut on its own, without an intervening DerefMut, can be demonstrated to work.

So, what is currently blocking support for v[IDX] = E (where IDX calls &self method on v)...?

• In theory we could extend two-phase borrows to support arbitraryfoo(&mut v, expr_using_v). That is what is tracked by Tracking issue for generalized two-phase borrows #49434
• Or we could treat IndexMut + DerefMut as a special case.

I've already expressed concerns about trying to deliver fully generalized two-phase borrows without a proof of soundness; you can see those comments on #49434.

(If you want to see an example of why we are trying to be conservative here, maybe take a look at #56254, which led us to ratchet back some of two-phase borrows initial expressiveness because we weren't confident that it was compatible with the stacked-borrows model from that time.)

A lot has happened since those discussions; e.g. models have evolved. But we still don't have any proof of soundness of generalized two-phase borrows, as far as I know.

(Having said all that, you all might be able to talk me into treating IndexMut as a special case... at the very least, we could experiment with such a change.)

[Danvil]

@pnkfelix I would try to talk you into at least supporting IndexMut as a special case for now.

Below is a related real-life use case:

#[derive(Debug)]
struct Matrix {
  elements: Vec<f32>,
  rows: usize,
  cols: usize,
}

impl Matrix {
  pub fn set(&mut self, row: usize, col: usize, value: f32) {
    self.elements[self.idx(row, col)] = value;  // ERR
  }
  
  fn idx(&self, row: usize, col: usize) -> usize {
    row * self.cols + col
  }
}

fn main() {
  let mut mat = Matrix { elements: vec![1.0,2.0,3.0,4.0,5.0,6.0], rows: 2, cols: 3 };
  mat.set(1,2, 3.0);
  println!("{:?}", mat);
}

[workingjubilee]

@Danvil It would be more direct to implement IndexMut on the wrapping type you desire, like so, and it compiles (Playground):

use std::ops::{Index, IndexMut};

#[derive(Debug)]
struct Matrix {
    elements: Vec<f32>,
    rows: usize,
    cols: usize,
}

impl Matrix {
    pub fn set(&mut self, row: usize, col: usize, value: f32) {
        self[(row, col)] = value;
    }
}

impl Index<(usize, usize)> for Matrix {
    type Output = f32;
    fn index(&self, (row, col): (usize, usize)) -> &f32 {
        &self.elements[row * self.cols + col]
    }
}

impl IndexMut<(usize, usize)> for Matrix {
    fn index_mut(&mut self, (row, col): (usize, usize)) -> &mut f32 {
        &mut self.elements[row * self.cols + col]
    }
}

fn main() {
    let mut mat = Matrix {
        elements: vec![1.0, 2.0, 3.0, 4.0, 5.0, 6.0],
        rows: 2,
        cols: 3,
    };
    mat.set(1, 2, 3.0);
    println!("{:?}", mat);
}

[Danvil]

@workingjubilee The issue was that I can't define the helper function idx and use it in the &mut case. Your example works because you got rid of that function and duplicated the code. In this example the function is small and simple, but in many cases it would lead to unnecessary and erroneous code duplication.