Inconsistent life time behavior between BorrowMut::borrow_mut() and AsMut::as_mut() #38624

FabianEmmes · 2016-12-26T23:43:58Z

The following code shows inconsistent behavior in lifetime checking:

use std::convert::AsMut;
use std::borrow::BorrowMut;

trait T {}

impl T for u8 {}

fn main() {
    // A (AsMut and Primitive Type):
    {
        let mut r: Box<u8> = Box::new(23u8);
        let _: &mut u8 = r.as_mut();
    }

    // B (AsMut and Trait Objects):
    {
        let mut r: Box<T> = Box::new(23u8);
        let _: &mut T = r.as_mut();
    }

    // C (BorrowMut and Primitive Type):
    {
        let mut r: Box<u8> = Box::new(23u8);
        let _: &mut u8 = r.borrow_mut();
    }

    // D (BorrowMut and Trait Object):
    {
        let mut r: Box<T> = Box::new(23u8);
        let _: &mut T = r.borrow_mut();
    }
}

Using the stable compiler 1.14.0, case B and D yield a lifetime error:

$ rustc --version
rustc 1.14.0 (e8a012324 2016-12-16)

$ rustc test.rs 
error: `r` does not live long enough
  --> test.rs:19:5
   |
18 |         let _: &mut T = r.as_mut();
   |                         - borrow occurs here
19 |     }
   |     ^ `r` dropped here while still borrowed
   |
   = note: values in a scope are dropped in the opposite order they are created

error: `r` does not live long enough
  --> test.rs:31:5
   |
30 |         let _: &mut T = r.borrow_mut();
   |                         - borrow occurs here
31 |     }
   |     ^ `r` dropped here while still borrowed
   |
   = note: values in a scope are dropped in the opposite order they are created

error: aborting due to 2 previous errors

return code: 101

A nightly rustc from last week shows only an error in case D.

$ rustc --version
rustc 1.15.0-nightly (71c06a56a 2016-12-18)

$ rustc test.rs 
error: `r` does not live long enough
  --> test.rs:31:5
   |
30 |         let _: &mut T = r.borrow_mut();
   |                         - borrow occurs here
31 |     }
   |     ^ `r` dropped here while still borrowed
   |
   = note: values in a scope are dropped in the opposite order they are created

error: aborting due to previous error

return code: 101

This is strange, as BorrowMut::borrow_mut() and AsMut::as_mut() have the same type signature. So cases B and D should give the same result.

Furthermore, I am not sure why case A and B (or, for that matter, case C and D) behave differently. The only difference is that they use trait objects instead of primitive types, and as far as I understand, this should have no influence on the lifetime behavior.

The text was updated successfully, but these errors were encountered:

FabianEmmes · 2016-12-26T23:45:17Z

There is some discussion of this issue at https://users.rust-lang.org/t/trait-objects-and-borrowmut/8520

shepmaster · 2017-01-17T02:08:25Z

Likewise, there is a related Stack Overflow question. The code there boiled down to:

use std::borrow::BorrowMut;

trait Foo {}

fn repro(mut foo: Box<Foo>) {
    let borrowed: &mut Foo = foo.borrow_mut();
}

fn main() {}

error: `foo` does not live long enough
 --> src/main.rs:6:30
  |
6 |     let borrowed: &mut Foo = foo.borrow_mut();
  |                              ^^^ does not live long enough
7 | }
  | - borrowed value only lives until here
  |
  = note: borrowed value must be valid for the static lifetime...

This isn't related to the 'static lifetime; using fn repro<'a>(mut foo: Box<Foo + 'a>) also has the failure.

withoutboats · 2017-01-17T03:09:19Z

@rust-lang/compiler Any insight? This looks pretty baffling to me.

nikomatsakis · 2017-01-17T18:14:36Z

The change from stable to nightly feels like potentially a regression. I'm not sure of the cause. I'm also not 100% sure of the cause of the original error, although I agree that trait object lifetime defaults is likely part of it.

(I'll note that &mut (T+'static) seems to work.)

nikomatsakis · 2017-01-17T18:15:26Z

Nominating for prioritization, in case we don't get to it before.

shepmaster · 2017-01-17T22:39:14Z

(I'll note that &mut (T+'static) seems to work.)

In code, this is:

fn repro<'a>(mut foo: Box<Foo + 'a>) {
    let borrowed: &mut (Foo + 'a) = foo.borrow_mut();
}

fn repro2(mut foo: Box<Foo>) {
    let borrowed: &mut (Foo + 'static) = foo.borrow_mut();
}

nikomatsakis · 2017-01-17T23:17:38Z

To be clear, I'm mostly concerned about the change in nightly behavior.

nikomatsakis · 2017-01-17T23:20:01Z

(Yes, so, per some conversation with @eddyb, and in line with the earlier comments on the thread, I think the reason an error is reported in the first place is that &mut T is expanded to &'a mut (T + 'a), where 'a is a fresh inference variable. This must then be assigned from with &mut (T + 'static). The 'static arises because Box<T> is expanded to Box<T + 'static>. Hence 'a = 'static, which then leads to the error because you have a borrow of lifetime 'static.)

eddyb · 2017-01-17T23:24:09Z

I believe there should be a coercion but it's not triggering for some reason.

withoutboats · 2017-01-17T23:26:22Z

Ought it not be 'static <: 'a (or the other way around possibly, I get confused)?

eddyb · 2017-01-17T23:28:56Z

&mut Foo<'a> is always invariant over 'a so without a coercion it can't be anything else.

shepmaster · 2017-01-18T13:56:52Z

@nikomatsakis your analysis was very enlightening, thank you! Could I trouble you to add why Deref behaves differently? This code works:

let borrowed: &mut Foo = &mut *foo;

nikomatsakis · 2017-01-19T08:05:33Z

@withoutboats

Ought it not be 'static <: 'a (or the other way around possibly, I get confused)?

No, because the trait appears underneath the &mut, in which case the lifetimes involved must match exactly (this is called "invariant" in type theory jargon).

@eddyb

I believe there should be a coercion but it's not triggering for some reason.

We do have a coercion for &mut Trait+'a be assigned to &mut Trait+'b, that's true, I imagine that it's not triggering because of the generics involved in the AsRef traits and so forth though. I'm not sure why that would have changed on nightly, but perhaps it did somehow?

@shepmaster

Could I trouble you to add why Deref behaves differently? This code works:

This is because of the coercion that @eddyb mentioned. So, to elaborate, to try and ameliorate this problem, we added a special rule that says if you have a value of type &mut Trait+'a (for some 'a) and you are coercing it to a value of type &mut Trait + 'b, then we will allow that coercion so long as 'a: 'b. Coercion is basically an extra rule that allows assignments to occur in cases where ordinary subtyping would not; this is normally because some runtime change is needed (e.g., we will coerce the thin pointer &[i32; 5] to a fat pointer &[i32]). In this case, it's purely a fiction. Coercion however requires that we know both the target type and the source type in advance, which is why you have to write let borrowed: &mut Foo in your example and not just let borrowed (that would behave somewhat differently). I think this is also why the coercion fails to fire in the more generic AsRef trait case.

On a related note, one of the changes that I (and others) have been contemplating is to rejigger how the type-checker works to try and have coercions trigger more often (e.g., to allow them to work also in generic cases where the types aren't known up front). But just how effective we can get it is something of an open question.

nikomatsakis · 2017-01-26T21:05:58Z

triage: P-high

@nikomatsakis

Perform lifetime elision (more) syntactically, before type-checking. The *initial* goal of this patch was to remove the (contextual) `&RegionScope` argument passed around `rustc_typeck::astconv` and allow converting arbitrary (syntactic) `hir::Ty` to (semantic) `Ty`. I've tried to closely match the existing behavior while moving the logic to the earlier `resolve_lifetime` pass, and [the crater report](https://gist.github.com/eddyb/4ac5b8516f87c1bfa2de528ed2b7779a) suggests none of the changes broke real code, but I will try to list everything: There are few cases in lifetime elision that could trip users up due to "hidden knowledge": ```rust type StaticStr = &'static str; // hides 'static trait WithLifetime<'a> { type Output; // can hide 'a } // This worked because the type of the first argument contains // 'static, although StaticStr doesn't even have parameters. fn foo(x: StaticStr) -> &str { x } // This worked because the compiler resolved the argument type // to <T as WithLifetime<'a>>::Output which has the hidden 'a. fn bar<'a, T: WithLifetime<'a>>(_: T::Output) -> &str { "baz" } ``` In the two examples above, elision wasn't using lifetimes that were in the source, not even *needed* by paths in the source, but rather *happened* to be part of the semantic representation of the types. To me, this suggests they should have never worked through elision (and they don't with this PR). Next we have an actual rule with a strange result, that is, the return type here elides to `&'x str`: ```rust impl<'a, 'b> Trait for Foo<'a, 'b> { fn method<'x, 'y>(self: &'x Foo<'a, 'b>, _: Bar<'y>) -> &str { &self.name } } ``` All 3 of `'a`, `'b` and `'y` are being ignored, because the `&self` elision rule only cares that the first argument is "`self` by reference". Due implementation considerations (elision running before typeck), I've limited it in this PR to a reference to a primitive/`struct`/`enum`/`union`, but not other types, but I am doing another crater run to assess the impact of limiting it to literally `&self` and `self: &Self` (they're identical in HIR). It's probably ideal to keep an "implicit `Self` for `self`" type around and *only* apply the rule to `&self` itself, but that would result in more bikeshed, and #21400 suggests some people expect otherwise. Another decent option is treating `self: X, ... -> Y` like `X -> Y` (one unique lifetime in `X` used for `Y`). The remaining changes have to do with "object lifetime defaults" (see RFCs [599](https://github.com/rust-lang/rfcs/blob/master/text/0599-default-object-bound.md) and [1156](https://github.com/rust-lang/rfcs/blob/master/text/1156-adjust-default-object-bounds.md)): ```rust trait Trait {} struct Ref2<'a, 'b, T: 'a+'b>(&'a T, &'b T); // These apply specifically within a (fn) body, // which allows type and lifetime inference: fn main() { // Used to be &'a mut (Trait+'a) - where 'a is one // inference variable - &'a mut (Trait+'b) in this PR. let _: &mut Trait; // Used to be an ambiguity error, but in this PR it's // Ref2<'a, 'b, Trait+'c> (3 inference variables). let _: Ref2<Trait>; } ``` What's happening here is that inference variables are created on the fly by typeck whenever a lifetime has no resolution attached to it - while it would be possible to alter the implementation to reuse inference variables based on decisions made early by `resolve_lifetime`, not doing that is more flexible and works better - it can compile all testcases from #38624 by not ending up with `&'static mut (Trait+'static)`. The ambiguity specifically cannot be an early error, because this is only the "default" (typeck can still pick something better based on the definition of `Trait` and whether it has any lifetime bounds), and having an error at all doesn't help anyone, as we can perfectly infer an appropriate lifetime inside the `fn` body. **TODO**: write tests for the user-visible changes. cc @nikomatsakis @arielb1

arielb1 · 2017-02-02T21:33:56Z

Minified:

pub trait BorrowMut<Borrowed> where Borrowed: ?Sized {
    fn borrow_mut(&mut self) -> &mut Borrowed;
}

impl<T> BorrowMut<T> for Box<T> where T: ?Sized {
    fn borrow_mut(&mut self) -> &mut T { panic!() }
}

#[cfg(not(works))]
impl<T> BorrowMut<T> for T where T: ?Sized {
    fn borrow_mut(&mut self) -> &mut T { panic!() }
}
trait T {}

impl T for u8 {}
fn main() {
    // D (BorrowMut and Trait Object):
    let mut r: Box<T> = Box::new(23u8);
    let _: &mut T = BorrowMut::borrow_mut(&mut r);
}

brson · 2017-02-09T17:20:32Z

Anyone want to bisect?

@TimNN :)

TimNN · 2017-02-09T17:21:28Z

@brson: Sure thing :)

arielb1 · 2017-02-09T17:21:41Z

The problem is that this code:

use std::borrow::BorrowMut;

trait T {}

impl T for u8 {}

fn main() {
    let mut r: Box<T> = Box::new(23u8);
    let _: &mut T = r.borrow_mut();
}

compiles on nightly but not on stable

nikomatsakis · 2017-02-09T17:39:44Z

I believe it is due to this PR: #39066

In this case, the change is expected.

nikomatsakis · 2017-02-09T17:43:52Z

I confirmed this by checking nightlies; it has the new behavior in 2017-02-02, the closest nightly I could find after the PR landed, but not in 2017-01-26 (before the PR).

nikomatsakis · 2017-02-09T17:44:45Z

So essentially this issue -- while confusing -- is working as intended. Shall we close?

TimNN · 2017-02-09T17:45:04Z

#39066 does fall into the range of nightlies I bisected this to: nightly-2017-01-26 still produces an error while nightly-2017-02-02 does not.

Live Edit: Well, @nikomatsakis was a bit faster than me ;)

troplin · 2017-02-09T19:36:36Z

My main concern was, that as_mut() and borrow_mut() with identical signature behave differently.
Apparently this is now also solved.
The original code snippet compiles successfully on nightly and beta.

1.14: as_mut() and borrow_mut() both don't compile
1.15: as_mut() compiles, borrow_mut()doesn't
1.16: as_mut() and borrow_mut() both compile

I'm still a bit surprised how that difference could even occur in 1.15. Is one of those traits special-cased in the compiler? Isn't the signature the only relevant thing for the borrow checker?

nikomatsakis · 2017-02-16T15:15:03Z

@troplin neither is special to the compiler that I know of; I don't know why as_mut behaved differently.

nikomatsakis · 2017-02-16T15:17:31Z

@troplin ah well also I think @eddyb landed a PR that adjusts how the defaulting works around &mut Trait declarations. Specifically in a fn body, that would default before to &'a mut (Trait + 'a) for some fresh variable 'a. This caused a lot of problems. Now we default to &'a mut (Trait + 'b) -- i.e., two fresh variables. This allows more code to work, and so is presumably related here as well.

eddyb · 2017-02-16T15:23:35Z

Yeah, I did mention (on IRC?) that change being a fix to the strictness identified in #38624 (comment).

arielb1 · 2017-02-16T21:08:16Z

Since both changes here seem to be intentional, closing.

nikomatsakis added A-typesystem Area: The type system I-wrong regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jan 17, 2017

eddyb mentioned this issue Jan 25, 2017

Perform lifetime elision (more) syntactically, before type-checking. #39305

Merged

brson added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 26, 2017

nikomatsakis self-assigned this Jan 26, 2017

rust-highfive added P-high High priority and removed I-nominated labels Jan 26, 2017

pnkfelix self-assigned this Jan 26, 2017

brson added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Feb 9, 2017

arielb1 closed this as completed Feb 16, 2017

arielb1 mentioned this issue Feb 2, 2019

Associated types bug related to trait objects #30833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inconsistent life time behavior between BorrowMut::borrow_mut() and AsMut::as_mut() #38624

Inconsistent life time behavior between BorrowMut::borrow_mut() and AsMut::as_mut() #38624

FabianEmmes commented Dec 26, 2016

FabianEmmes commented Dec 26, 2016

shepmaster commented Jan 17, 2017 •

edited

Loading

withoutboats commented Jan 17, 2017 •

edited

Loading

nikomatsakis commented Jan 17, 2017

nikomatsakis commented Jan 17, 2017

shepmaster commented Jan 17, 2017

nikomatsakis commented Jan 17, 2017

nikomatsakis commented Jan 17, 2017

eddyb commented Jan 17, 2017

withoutboats commented Jan 17, 2017

eddyb commented Jan 17, 2017

shepmaster commented Jan 18, 2017

nikomatsakis commented Jan 19, 2017

nikomatsakis commented Jan 26, 2017

arielb1 commented Feb 2, 2017

brson commented Feb 9, 2017

TimNN commented Feb 9, 2017

arielb1 commented Feb 9, 2017

nikomatsakis commented Feb 9, 2017

nikomatsakis commented Feb 9, 2017

nikomatsakis commented Feb 9, 2017

TimNN commented Feb 9, 2017

troplin commented Feb 9, 2017

nikomatsakis commented Feb 16, 2017

nikomatsakis commented Feb 16, 2017

eddyb commented Feb 16, 2017 •

edited

Loading

arielb1 commented Feb 16, 2017

Inconsistent life time behavior between BorrowMut::borrow_mut() and AsMut::as_mut() #38624

Inconsistent life time behavior between BorrowMut::borrow_mut() and AsMut::as_mut() #38624

Comments

FabianEmmes commented Dec 26, 2016

FabianEmmes commented Dec 26, 2016

shepmaster commented Jan 17, 2017 • edited Loading

withoutboats commented Jan 17, 2017 • edited Loading

nikomatsakis commented Jan 17, 2017

nikomatsakis commented Jan 17, 2017

shepmaster commented Jan 17, 2017

nikomatsakis commented Jan 17, 2017

nikomatsakis commented Jan 17, 2017

eddyb commented Jan 17, 2017

withoutboats commented Jan 17, 2017

eddyb commented Jan 17, 2017

shepmaster commented Jan 18, 2017

nikomatsakis commented Jan 19, 2017

nikomatsakis commented Jan 26, 2017

arielb1 commented Feb 2, 2017

brson commented Feb 9, 2017

TimNN commented Feb 9, 2017

arielb1 commented Feb 9, 2017

nikomatsakis commented Feb 9, 2017

nikomatsakis commented Feb 9, 2017

nikomatsakis commented Feb 9, 2017

TimNN commented Feb 9, 2017

troplin commented Feb 9, 2017

nikomatsakis commented Feb 16, 2017

nikomatsakis commented Feb 16, 2017

eddyb commented Feb 16, 2017 • edited Loading

arielb1 commented Feb 16, 2017

shepmaster commented Jan 17, 2017 •

edited

Loading

withoutboats commented Jan 17, 2017 •

edited

Loading

eddyb commented Feb 16, 2017 •

edited

Loading