consider expanding two-phase borrows *just a bit more*

[nikomatsakis]

At first, I thought this was a bug in two-phase borrows, but I realize now that this example does not fall into the subset that @pnkfelix and I identified. That said, I wonder if it should? Try it on play:

#![feature(nll)]

struct Foo<'a> {
    x: &'a u32,
}

impl<'a> Foo<'a> {
    fn method(&mut self, value: &u32) {
        
    }
}

fn main() {
    let mut f = &mut Foo { x: &22 };
    Foo::method(f, f.x)
}

This gives:

error[E0502]: cannot borrow `*f.x` as immutable because it is also borrowed as mutable
  --> src/main.rs:15:20
   |
15 |     Foo::method(f, f.x)
   |                 -  ^^^ immutable borrow occurs here
   |                 |
   |                 mutable borrow occurs here

I wonder if we should expand to auto-refs on any method argument? I think they all have the same character as self.foo(), and those would preserve the self.foo(..) => Foo::foo(self, ..) transformation more faithfully.

cc @rust-lang/wg-compiler-nll

[nikomatsakis]

I encountered this example attempting to bootstrap rustc.

[nikomatsakis]

Also, that error message is ungreat.

[leoyvens]

I wonder if we should expand to auto-refs on any method argument?

I had suggested a rule where that happens only if there are no constraints on the lifetime of the reference. Considering that's probably difficult to implement, we could use a syntactical rule which is a subset of that: We may auto-ref method arguments whenever the lifetime of the argument is elided.

[nikomatsakis]

@leodasvacas

I had suggested a rule where that happens only if there are no constraints on the lifetime of the reference.

Remind me, why does that matter? We don't have an analogous restriction on the self argument today.

[leoyvens]

Nevemind, I got confused and thought this was about general auto-refing.

[nikomatsakis]

cc @bobtwinkles -- what do you think about trying to implement this change?

cc @pnkfelix -- do you object to this expansion?

[sapphire-arches]

I should be able to take a stab at this sometime this week. It might end up being as easy as switching an allow_two_phase_borrow somewhere, but I suspect not because life is never that easy 😄. I'll try to have a PR by Monday at the latest, baring life smacking me upside the head

[sapphire-arches]

OK, so some notes (mostly for myself but perhaps useful to those interested in how I'm progressing).

• This will indeed not be as simple as changing a single allow_two_phase_borrow tag in one place.
• When calling a method without using the . operator, in HIR the call is expressed as an ExprCall rather than an ExprMethodCall. This means that we go through a completely different code path in HIR typeck, and don't directly have control over the borrow used for &self/&mut self assuming that we even are borrowing. It may be that self is consumed directly, is passed as a Box, etc.
• The situation we care about is a CallStep::Builtin (the variant names on that enum are extremely confusing to me to be perfectly honest) ( -> confirm_builtin_call).
• The "obvious" approach is to add some special casing in to confirm_builtin_call where when we are processing a method argument and we need &mut self, figure out where the borrow for &mut self comes from and inject a tag that we then respect after lowering to MIR in dataflow. Approximately none of that infrastructure exists today, since the existing 2-phase tracking only operates on AutoBorrows whereas this would need to tag user borrows.
• Another approach would be to perform this check during HAIR/MIR lowering, since at that the difference between auto borrows and user-borrows disappears and I can inject the flag in to the existing two-phase borrow tracking infrastructure. If that approach pans out I'd be tempted to try and rip the two-phase borrow code out of librustc_typeck entirely if possible to keep things localized and perhaps help tame the beast that is librustc_typeck =P.
• Yet another approach would be to introduce an artificial deref/autoref pair with the two-phase borrow bit set. This feels like a quick and dirty solution that will come back to bite later as one of the "proper" solutions outlined above will need to be implemented as HIR typeck is phased out (assuming that's happening? I'm not sure but that seems to be the case given the full typechecker implemented in rustc_mir =))
• re "I wonder if we should expand to auto-refs on any method argument?".
  pro
  - Faithful desugaring of method call syntax
  - better ergonomics
  con
  - inconsistent magic. If we did this I would be tempted to say we should inject autorefs everywhere, at least for immutable borrows. Of course, I feel like this has been discussed before and discarded and I'm just not able to find it right now.
  - Increased implementation complexity. Though this wouldn't be that bad if we're already doing special casing of methods_calls-as-calls for 2-phase borrows.
  With that in mind, I'm leaning towards not doing it unless we do it everywhere (and either way it would probably require an RFC no?)

[sapphire-arches]

A collection of tricky cases:

struct Foo<'a> {
    x: &'a u32,
}

struct Bar<'a, 'b: 'a> {
    x: &'a mut Foo<'b>
}

impl<'a> Foo<'a> {
    fn method(&mut self, value: &u32) {
        
    }
}

fn trick1() {
    let mut f = &mut Foo { x: &22 };
    let z = Bar {x: f};
    // This is hard because we need to look "through" a projection to figure out where the borrow came from
    Foo::method(z.x, z.x.x)
}

fn trick2() {
    let mut b = &mut Foo { x: &7 };
    let mb = b;
    // similarly, there is more than one assignment we would have to consider
    Foo::method(mb, mb.x);
}

[semenzato]

I don't understand why there is a problem with the original example. Is it because &22 has static lifetime? Otherwise, with a small change the example creates aliasing---isn't that exactly what Rust should protect from?

There is a problem if x is a u32 instead of a &u32. The borrow checker still complains because it applies the checks to the borrows needed to evaluate the arguments, rather than to the borrows needed to make the call after argument evaluation.