create fewer region variables in coercions #32306

nikomatsakis · 2016-03-17T09:26:52Z

Fixes #32278.

r? @eddyb

nikomatsakis · 2016-03-17T09:27:27Z

I am still running a final make check, but things were passing at one not so long ago point. :)

nikomatsakis · 2016-03-17T09:27:36Z

Ah, I think I forgot some kind of regression test.

nikomatsakis · 2016-03-17T09:49:32Z

Regression test: rust-lang-deprecated/rustc-benchmarks#3

Do you think it makes sense to add to run-pass? The full test takes 3 or 4 seconds to compile, and reduced ones don't seem that interesting.

nikomatsakis · 2016-03-17T09:54:21Z

Gah, fails to build.

nikomatsakis · 2016-03-17T10:01:30Z

Well, have to stop now, the error feels fixable. Somehow region inference isn't creating the required edges I think. The minimized test case is:

use std::rc::Rc;

#[derive(Clone)]
enum CachedMir<'mir> {
    Ref(&'mir String),
    Owned(Rc<String>),
}

impl<'mir> CachedMir<'mir> {
    fn get_ref<'a>(&'a self) -> &'a String {
        match *self {
            CachedMir::Ref(r) => r,
            CachedMir::Owned(ref rc) => &rc,
        }
    }
}

fn main() { }

which fails with an error:

/home/nmatsakis/tmp/foo.rs:13:42: 13:44 error: `rc` does not live long enough
/home/nmatsakis/tmp/foo.rs:13             CachedMir::Owned(ref rc) => &rc,
                                                                       ^~
/home/nmatsakis/tmp/foo.rs:10:44: 15:6 note: reference must be valid for the lifetime 'a as defined on the block at 10:43...
/home/nmatsakis/tmp/foo.rs:10     fn get_ref<'a>(&'a self) -> &'a String {
/home/nmatsakis/tmp/foo.rs:11         match *self {
/home/nmatsakis/tmp/foo.rs:12             CachedMir::Ref(r) => r,
/home/nmatsakis/tmp/foo.rs:13             CachedMir::Owned(ref rc) => &rc,
/home/nmatsakis/tmp/foo.rs:14         }
/home/nmatsakis/tmp/foo.rs:15     }
/home/nmatsakis/tmp/foo.rs:11:9: 14:10 note: ...but borrowed value is only valid for the match at 11:8
/home/nmatsakis/tmp/foo.rs:11         match *self {
/home/nmatsakis/tmp/foo.rs:12             CachedMir::Ref(r) => r,
/home/nmatsakis/tmp/foo.rs:13             CachedMir::Owned(ref rc) => &rc,
/home/nmatsakis/tmp/foo.rs:14         }
error: aborting due to previous error

nikomatsakis · 2016-03-17T10:03:41Z

(Obviously the &rc isn't needed, but it ought to work.)

eddyb · 2016-03-17T10:25:48Z

src/librustc_typeck/check/coercion.rs

+            //   want to use that target type region (`'b`) because --
+            //   for the program to type-check -- it must be the
+            //   smaller of the two.
+            let r = if self.use_lub {r_a} else {r_b}; // [2] above


I believe you still have to relate r_a and r_b in sub mode, e.g. &'a T coerced to &'b T in sub mode would only require that &'b T is a subtype of &'b T, but not that 'a outlives 'b.

@eddyb

I believe you still have to relate r_a and r_b in sub mode, e.g. &'a T coerced to &'b T in sub mode would only require that &'b T is a subtype of &'b T, but not that 'a outlives 'b.

We discussed on IRC, but this is not the problem. TL;DR regionck adds the edge. I added more text to the comment to address exactly this point in 1f30fe0.

nikomatsakis · 2016-03-17T14:00:30Z

OK, I think the problem is fixed now, and the compilation performance is still good.

eddyb · 2016-03-17T19:15:16Z

src/librustc_typeck/check/coercion.rs

+               ty, autoderefs, autoref);
+        Ok((ty, AdjustDerefRef(AutoDerefRef {
+            autoderefs: autoderefs,
+            autoref: autoref,


This isn't trying to avoid noop reborrow coercions? I don't want the array of strings to have any coercions at all.

@eddyb it's not obvious when we can avoid those -- I guess if r_borrow == r_a and autoderefs == 1, you mean? I can add a test for that, why not.

instead, extract the target region out of the autoderef loop

the goal here is to minimize creating variables

The older code would sometimes swallow errors or fail to produce a suggestion. The newer code does not. However, just printing everything would produce a bunch of new and kind of annoying errors, so continue to swallow `T: 'a` errors so long as there are other things to show.

nikomatsakis · 2016-03-18T21:22:21Z

@eddyb ok so we now should have the suggestions again and it is eliding no-op coercions. make check is still running locally but I wouldn't expect any problems.

nikomatsakis · 2016-03-18T21:23:27Z

src/test/compile-fail/lifetime-inference-give-expl-lifetime-param-3.rs

@@ -22,7 +22,7 @@ fn bar1<'a>(x: &Bar) -> (&'a i32, &'a i32, &'a i32) {
 }

 fn bar2<'a, 'b, 'c>(x: &Bar<'a, 'b, 'c>) -> (&'a i32, &'a i32, &'a i32) {
-    //~^ HELP: consider using an explicit lifetime parameter as shown: fn bar2<'a, 'b, 'c>(x: &'a Bar<'a, 'b, 'c>) -> (&'a i32, &'a i32, &'a i32)
+    //~^ HELP: consider using an explicit lifetime parameter as shown: fn bar2<'a, 'c>(x: &'a Bar<'a, 'a, 'c>) -> (&'a i32, &'a i32, &'a i32)


Sigh. I have no idea why this suggestion changed.

Oh, I think I get why. It's not wrong in any case.

eddyb · 2016-03-18T23:19:53Z

@bors r+

bors · 2016-03-18T23:19:54Z

📌 Commit 43aed96 has been approved by eddyb

bors · 2016-03-19T14:59:07Z

⌛ Testing commit 43aed96 with merge 76d42f5...

bors · 2016-03-19T16:39:25Z

💔 Test failed - auto-mac-32-opt

alexcrichton · 2016-03-19T18:36:31Z

@bors: retry

On Sat, Mar 19, 2016 at 9:39 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-mac-32-opt
http://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/8471

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#32306 (comment)

nikomatsakis · 2016-03-19T21:26:12Z

@alexcrichton I was just going to ask if these timeouts are a thing...never seen one before? I ... highly doubt this patch made things any slower.

alexcrichton · 2016-03-19T21:29:28Z

Yeah lldb timeouts on mac have been happening for quite some time

bors · 2016-03-20T01:39:51Z

⌛ Testing commit 43aed96 with merge 78e8a00...

@eddyb

create fewer region variables in coercions Fixes #32278. r? @eddyb

bors · 2016-03-20T04:15:21Z

rust-highfive assigned eddyb Mar 17, 2016

nikomatsakis force-pushed the issue-32278 branch from b1df47b to f0c7b3c Compare March 17, 2016 09:27

nikomatsakis mentioned this pull request Mar 17, 2016

add regr test for #32278 rust-lang-deprecated/rustc-benchmarks#3

Merged

eddyb reviewed Mar 17, 2016
View reviewed changes

nikomatsakis force-pushed the issue-32278 branch from d3578ef to f7156bf Compare March 18, 2016 20:38

nikomatsakis added 7 commits March 18, 2016 16:38

change coercion to use target region if not LUB

1922041

rename unify to unify_and_identity

00c9420

don't create region vars in LUB coercions

ad6ca08

instead, extract the target region out of the autoderef loop

pick off some easy cases for LUB/GLB in regions

cdaee4a

the goal here is to minimize creating variables

fix coercion behavior for nested references

7f8d245

add comment clarifying what edges are needed

5abacd3

nikomatsakis force-pushed the issue-32278 branch from f7156bf to bca07b5 Compare March 18, 2016 20:39

elide noop coercions

25988b6

nikomatsakis reviewed Mar 18, 2016
View reviewed changes

add assertion and comment about mutability

43aed96

bors added a commit that referenced this pull request Mar 20, 2016

Auto merge of #32306 - nikomatsakis:issue-32278, r=eddyb

78e8a00

create fewer region variables in coercions Fixes #32278. r? @eddyb

bors merged commit 43aed96 into rust-lang:master Mar 20, 2016

nikomatsakis deleted the issue-32278 branch March 30, 2016 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create fewer region variables in coercions #32306

create fewer region variables in coercions #32306

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

eddyb Mar 17, 2016

nikomatsakis Mar 17, 2016

nikomatsakis commented Mar 17, 2016

eddyb Mar 17, 2016

nikomatsakis Mar 18, 2016

nikomatsakis commented Mar 18, 2016

nikomatsakis Mar 18, 2016

nikomatsakis Mar 18, 2016

eddyb commented Mar 18, 2016

bors commented Mar 18, 2016

bors commented Mar 19, 2016

bors commented Mar 19, 2016

alexcrichton commented Mar 19, 2016

nikomatsakis commented Mar 19, 2016

alexcrichton commented Mar 19, 2016

bors commented Mar 20, 2016

bors commented Mar 20, 2016

create fewer region variables in coercions #32306

create fewer region variables in coercions #32306

Conversation

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

nikomatsakis commented Mar 17, 2016

eddyb Mar 17, 2016

Choose a reason for hiding this comment

nikomatsakis Mar 17, 2016

Choose a reason for hiding this comment

nikomatsakis commented Mar 17, 2016

eddyb Mar 17, 2016

Choose a reason for hiding this comment

nikomatsakis Mar 18, 2016

Choose a reason for hiding this comment

nikomatsakis commented Mar 18, 2016

nikomatsakis Mar 18, 2016

Choose a reason for hiding this comment

nikomatsakis Mar 18, 2016

Choose a reason for hiding this comment

eddyb commented Mar 18, 2016

bors commented Mar 18, 2016

bors commented Mar 19, 2016

bors commented Mar 19, 2016

alexcrichton commented Mar 19, 2016

nikomatsakis commented Mar 19, 2016

alexcrichton commented Mar 19, 2016

bors commented Mar 20, 2016

bors commented Mar 20, 2016