Implement RFC 1679

[sfackler]

cc #35729

r? @alexcrichton

[alexcrichton]

Nice! Could you also add tests that are exercising these new abilities as well?

[sfackler]

@alexcrichton updated

[alexcrichton]

Ok so I finally got around to actually running this on crater, and the result was three regressions: https://gist.github.com/alexcrichton/0c44e2a95044ae9f4d364496bbd0a388. One of those is spurious but the other two look legitimate.

• The first has some weird lifetime error
• The second looks like some type inference failures

These seem kinda worrisome :(

[sfackler]

The gilrs failure looks like the kind of breakage we'd expect from our back compat guarantees and should be easy to fix. The rss-rs issue is way weirder - not sure what's going on with that.

[alexcrichton]

I'm guessing this is all because the output type is now generic, so there's cases where there's just not enough inference to figure out what it should actually be

[alexcrichton]

Ok, minimizing the rss-rs case a little further:

fn foo<'a>(split: &[&'a [u8]]) -> (&'a [u8], &'a [u8]) {
    (
        unsafe { split.get_unchecked(0) },
        unsafe { split.get_unchecked(1) },
    )
}

On stable that compiles and with this patch it yields

error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
 --> src/lib.rs:3:24
  |
3 |         unsafe { split.get_unchecked(0) },
  |                        ^^^^^^^^^^^^^

error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
 --> src/lib.rs:4:24
  |
4 |         unsafe { split.get_unchecked(1) },
  |                        ^^^^^^^^^^^^^
  |
help: consider using an explicit lifetime parameter as shown: fn foo<'a>(split: &'a [&'a [u8]]) -> (&'a [u8], &'a [u8])
 --> src/lib.rs:1:1
  |
1 | fn foo<'a>(split: &[&'a [u8]]) -> (&'a [u8], &'a [u8]) {
  | ^

error: aborting due to 2 previous errors

That help message seems pretty suspicious for what might be happening here/

[sfackler]

@alexcrichton do you see the same error using normal indexing with/without the patch?

[alexcrichton]

@sfackler interestingly, no! If I change the return value to (split[0], split[1]), it works on stable, nightly, and with this patch. Even if I change it to (&split[0], &split[1]) it works on all three compilers...

[sfackler]

That's bizarre. It is a difference between parameterized impls and parameterized methods, but I wouldn't have expected that to matter in this way.

[bors]

☔ The latest upstream changes (presumably #36491) made this pull request unmergeable. Please resolve the merge conflicts.

[aturon]

cc @nikomatsakis, see #36340 (comment) in particular.

[nikomatsakis]

Hmm. I have some theories but no concrete idea. I think I'll have to dig in a bit.

[nikomatsakis]

I spent about an hour digging into this case today. Got to do a few other things now, but I'll try to come back to it. I haven't yet gotten to the bottom of what's going on, but I am going to leave myself a few notes here:

• the value being returned has type &'1 &'2 [u8], which is then automatically coerced via deref-coercion to &'a [u8] (those are region variables). IOW, the compiler inserts to something like &**split.get_unchecked(0). However, if you add this by hand, everything works. =) So that's a sign of a compiler bug.
• the bad constraint seems to be 'a <= '1, which is added as part of a subtype check when subtyping &'1 [u8] <: &'a [u8]. That is ... valid but I'm not yet quite sure why this subtyping is happening. Anyway, I think this is the problem.
• here '1 represents the lifetime of the return value; there is a constraint that '1 <= '0, where '0 is the lifetime of the reference to self that is passed into get_unchecked. This seems correct too.

[sfackler]

Thanks for looking into it!

@alexcrichton Given that the weird breakage seems to be something that "should" work modulo compiler bugs, I'd lean towards fixing the two crates that regress and landing this. Thoughts?

[alexcrichton]

Hm this is a change to such a core type I'd personally prefer to wait for the compiler bug, if any, to get fixed.

[nikomatsakis]

I'd be more comfortable waiting; I'll try to dig in a bit more asap

On Fri, Oct 21, 2016 at 10:04:18AM -0700, Alex Crichton wrote:

Hm this is a change to such a core type I'd personally prefer to wait for the compiler bug, if any, to get fixed.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#36340 (comment)

[nikomatsakis]

OK, sorry for the delay, last week was crazy, but I dug in some more and I understand the problem. In particular, we have this bit of cleverness called expected_types_for_fn_args() which attempts to propagate constraints from the return type to the arguments. The intention here was things like Some(foo) apply coercions to foo.

In this case, though, we know that the return type like be &'1 [T], and we have the expected return type of &'a [u8]. From this we conclude (falsely) that we should give an expected type of &'a [u8] and we relate that '1: 'a. But ultimately the coercion becomes &** which doesn't require that '1: 'a.

I'm not entirely sure what's the best solution here. I have to kind of bring this coercion bit back in my head, and in particular under what circumstances it is required to commit region edges; it may be that we can apply it in more limited circumstances, or avoid committing all of the region inference edges. I suspect region edges are only needed when they affect new region variables that get created during the coercion -- e.g., the edge here, if it were truly needed, would be recreated, I believe.

cc @eddyb who wrote the code originally

[eddyb]

Yeah, the region stuff was never great. We only truly care about "type skeletons" - as long as the coercion is triggered, having more freedom around regions (i.e. even replacing them all with fresh vars) shouldn't break anything.
Regular subtyping and then later regionck should (re)create all necessary relationships anyway.

[nikomatsakis]

@eddyb

Regular subtyping and then later regionck should (re)create all necessary relationships anyway.

Do you remember why we had to commit the regions? I guess I can remove that step (and perhaps replace them with fresh vars) and see what happens.

[nikomatsakis]

OK, I have a provisional fix for the compiler bug in question. Doing more testing now.

[brson]

Thanks @nikomatsakis!

[sfackler]

Rebased - can we run crater one more time now that niko's fix is in?

[eddyb]

Starting crater run.

[sfackler]

Thanks!

[eddyb]

tidy failed, line longer than 100 chars (should not impact the crater run though).

[sfackler]

Oops, fixed.

[eddyb]

Crater report shows one root regression (postgres).

[sfackler]

Well that's easy! sfackler/rust-postgres@11f1186

[sfackler]

@alexcrichton I think this is good to go now?

[frewsxcv]

As per travis:

error: error pattern 'assertion failed: self < slice.len()' not found!

[sfackler]

Oops, fixed.

[alexcrichton]

@bors: r+

Nice!

[bors]

📌 Commit 5377b5e has been approved by alexcrichton

[bors]

⌛ Testing commit 5377b5e with merge f8614c3...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing f8614c3 to master...