Improve SubSupConflict with a named and an anonymous lifetime parameter #42701 #44167

cengiz-io · 2017-08-29T21:24:03Z

Hello!

This fixes #42701.

UPDATE 01

Tests are producing different results between different env builds.
This inconsistency might take a long time to investigate and fix. So, be patient

UPDATE 02

Changed an FxHashMap with a BTreeMap. Inconsistency seems to be resolved for now.

rust-highfive · 2017-08-29T21:24:14Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

gaurikholkar-zz · 2017-08-30T03:45:46Z

src/test/ui/lifetime-errors/one_named_and_one_anonymous.stderr

@@ -0,0 +1,41 @@
+error[E0495]: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements


#42700
The first snippet of code, that's how the ui test must output

gaurikholkar-zz · 2017-08-30T03:46:27Z

src/test/ui/lifetime-errors/two_anonymous.rs

@@ -0,0 +1,24 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT


This is of type ConcreteFailure, not SubSupConflict

gaurikholkar-zz · 2017-08-30T16:08:07Z

src/librustc/infer/error_reporting/named_anon_conflict.rs

    pub fn try_report_named_anon_conflict(&self, error: &RegionResolutionError<'tcx>) -> bool {
        let (span, sub, sup) = match *error {
            ConcreteFailure(ref origin, sub, sup) => (origin.span(), sub, sup),
+            SubSupConflict(_, ref origin, sub, _, sup) => (origin.span(), sub, sup),
            _ => return false, // inapplicable
        };



You will have to remove the is_self_anon check. Maybe you can try an example without self, that should work.

nikomatsakis · 2017-08-30T16:15:10Z

r? @nikomatsakis

nikomatsakis · 2017-08-30T19:17:43Z

I think this is a good test case:

struct Foo {
    field: i32,
}

fn foo2<'a>(a: &'a Foo, x: &i32) -> &'a i32 {
    if true {
        let p: &i32 = &a.field;
        &*p
    } else {
        &*x
    }
}

fn main() { }

arielb1 · 2017-09-05T18:17:17Z

Hi @cengizio! Nice PR! Can you add @nikomatsakis's test so we can see how the errors look and fix this?

gaurikholkar-zz · 2017-09-07T05:52:15Z

@nikomatsakis can you suggest a test case for anon_anon conflicts too?

nikomatsakis · 2017-09-08T15:11:10Z

@gaurikholkar I guess that's a bit harder. If you just remove the named lifetime, then the elision rules will fail. You can use an &self to still create a conflict, but then we get into the rules that disable anon-anon reporting around self, I believe.

bors · 2017-09-10T18:39:05Z

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

nikomatsakis · 2017-09-14T13:40:44Z

@cengizio

Maybe try this example too?

struct Foo {
    field: i32, 
}

fn foo2<'a, 'b>(a: &'a Foo, x: &'b i32) -> &'a i32 {
    if true {
        let p: &i32 = &a.field;
        &*p
    } else {
        &*x
    }
}

fn main() { }

I imagine this will exercise the "different lifetimes" code a bit.

arielb1 · 2017-09-19T20:46:43Z

You have a tidy error:

[00:03:51] tidy error: /checkout/src/test/ui/lifetime-errors/42701_one_named_and_one_anonymous.rs:12: trailing whitespace

arielb1 · 2017-09-26T17:02:24Z

This PR is basically ready to land, you just have to fix the tidy error. Just a ping to make sure this isn't getting lost.

cengiz-io · 2017-09-26T19:40:41Z

Hello @arielb1

Thanks for notifying. I'm still trying to resolve this example case #44167 (comment)

Whenever that finishes, I'll fix tidy error and push them.

cengiz-io · 2017-09-26T22:11:42Z

Just fixed tidy error.

Also, while rebasing, it turned out to change 3 ui tests. I'm not sure about this because it was an implicit change. 314bb98

@nikomatsakis maybe can shed some light to this implicit change

cengiz-io · 2017-09-27T05:55:34Z

There seems to be some changes which are breaking other tests also. Back to the drawing board...

nikomatsakis · 2017-09-27T21:44:06Z

@cengizio those 3 ui tests appear to be bug fixes =) that is, thanks to your change, the 'new and improved' errors are triggering more often. seems good!

nikomatsakis · 2017-09-27T21:51:36Z

@cengizio also those test errors are not unexpected. I think you just have to update the tests to change them to expect the new error message. However, these tests are a bit surprising to me:

object-lifetime-default-from-box-error.rs
issue-40288-2.rs

I'm not sure what's up with those two cases!

cengiz-io · 2017-10-02T23:00:03Z

@nikomatsakis update-references.sh doesn't help much on compile-fail tests (there's a bash issue but even solving that didn't work out) so I'm updating them manually.

I will also check those two surprising cases and report here.
Thank you!

nikomatsakis · 2017-10-03T10:50:44Z

@cengizio yes, compile-fail tests have to be edited manually.

cengiz-io · 2017-10-08T16:53:23Z

@nikomatsakis @arielb1 @gaurikholkar hello again!

I've updated the test cases.

NOTE: I decided to make test assertions in compile-fail tests as specific as possible. Please let me know if that's not the general case.

gaurikholkar-zz · 2017-10-08T20:26:22Z

@cengizio this test fails
src/test/compile-fail/associated-types/cache/project-fn-ret-invariant.rs

[01:00:24]     Error {
[01:00:24]         line_num: 50,
[01:00:24]         kind: Some(
[01:00:24]             Error
[01:00:24]         ),
[01:00:24]         msg: "50:5: 50:6: lifetime mismatch [E0623]"
[01:00:24]     }
[01:00:24] ]
[01:00:24]

I guess you have given the wrong column no for the error

cengiz-io · 2017-10-08T20:36:56Z

@gaurikholkar thanks for noticing. The annoying part is, all tests are passing on my local machine. (with latest HEAD).

Let's see if this fixes the problem.

cengiz-io · 2017-10-08T22:32:10Z

Argh! There seems to be line differences between my git clone and travis' copy. I'll investigate this further tomorrow.

gaurikholkar-zz · 2017-10-09T04:37:33Z

gaurikholkar-zz@cb0d746
Maybe this should work

gaurikholkar-zz · 2017-10-09T11:29:51Z

Ignore the indentation mess

bors · 2017-11-09T06:34:17Z

☀️ Test successful - status-travis
State: approved= try=True

kennytm · 2017-11-09T06:36:25Z

@rust-lang/infra Perf check wanted (44a477ee0833eef8007499fec6ebd88c5289da8a).

nikomatsakis · 2017-11-10T11:16:35Z

Looks like try build is ready. cc @rust-lang/infra

Mark-Simulacrum · 2017-11-11T18:46:16Z

http://perf.rust-lang.org/compare.html?start=fc77b623d3072005f35e320e652f2b31695d493c&end=44a477ee0833eef8007499fec6ebd88c5289da8a&stat=instructions%3Au

nikomatsakis · 2017-11-11T19:30:20Z

OK so -- the perf results seem fine, but @cengizio and I were discussing that maybe it's still worth replacing the BTreeMap with a Vec and a FxHashSet for duplicate checking. That would achieve the same results and seems better.

nikomatsakis · 2017-11-12T12:59:03Z

@arielb1 and I were talking. We think we'll r+ this PR and then do any perf experiments in follow-up work.

@bors r+

bors · 2017-11-12T12:59:04Z

📌 Commit dad7d94 has been approved by nikomatsakis

bors · 2017-11-12T14:42:10Z

⌛ Testing commit dad7d94 with merge 5b1b7b6c84a3e8e723a0416485a228abf756918c...

bors · 2017-11-12T15:38:09Z

💔 Test failed - status-travis

…rameter #42701

nikomatsakis · 2017-11-12T19:13:59Z

@bors r+

bors · 2017-11-12T19:13:59Z

📌 Commit f53fc57 has been approved by nikomatsakis

bors · 2017-11-12T21:04:46Z

⌛ Testing commit f53fc57 with merge 79cfce3...

Improve SubSupConflict with a named and an anonymous lifetime parameter #42701 Hello! This fixes #42701. ## UPDATE 01 Tests are producing different results between different env builds. This inconsistency might take a long time to investigate and fix. So, be patient ## UPDATE 02 Changed an `FxHashMap` with a `BTreeMap`. Inconsistency seems to be resolved for now.

bors · 2017-11-12T23:31:53Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 79cfce3 to master...

rust-highfive assigned pnkfelix Aug 29, 2017

gaurikholkar-zz suggested changes Aug 30, 2017

View reviewed changes

gaurikholkar-zz reviewed Aug 30, 2017

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned pnkfelix Aug 30, 2017

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 31, 2017

nikomatsakis approved these changes Oct 9, 2017

View reviewed changes

nikomatsakis approved these changes Nov 11, 2017

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2017

nikomatsakis mentioned this pull request Nov 12, 2017

Simplify higher-ranked LUB/GLB #45853

Merged

cengiz-io added 7 commits November 12, 2017 18:56

Improve SubSupConflict case with one named, one anonymous lifetime pa…

da52563

…rameter #42701

Update fn-ret-invariant test assertion

ee839b3

use BTreeMap for region constraints

501c558

update failing E0621 tests

090669d

add reasons of using BTreeMap to a comment

5aac7a5

remove trailing whitespace from comment block

1719971

update project-fn-test-invariant test

f53fc57

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 12, 2017

bors merged commit f53fc57 into rust-lang:master Nov 12, 2017

bors mentioned this pull request Nov 12, 2017

Various refactorings towards universe-based regions #45657

Closed

		@@ -0,0 +1,41 @@
		error[E0495]: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements

		@@ -0,0 +1,24 @@
		// Copyright 2017 The Rust Project Developers. See the COPYRIGHT

Improve SubSupConflict with a named and an anonymous lifetime parameter #42701 #44167

Improve SubSupConflict with a named and an anonymous lifetime parameter #42701 #44167

Conversation

cengiz-io commented Aug 29, 2017 • edited Loading

UPDATE 01

UPDATE 02

rust-highfive commented Aug 29, 2017

gaurikholkar-zz Aug 30, 2017

Choose a reason for hiding this comment

gaurikholkar-zz Aug 30, 2017

Choose a reason for hiding this comment

gaurikholkar-zz Aug 30, 2017

Choose a reason for hiding this comment

nikomatsakis commented Aug 30, 2017

nikomatsakis commented Aug 30, 2017

arielb1 commented Sep 5, 2017 • edited Loading

gaurikholkar-zz commented Sep 7, 2017

nikomatsakis commented Sep 8, 2017

bors commented Sep 10, 2017

nikomatsakis commented Sep 14, 2017

arielb1 commented Sep 19, 2017 • edited Loading

arielb1 commented Sep 26, 2017

cengiz-io commented Sep 26, 2017

cengiz-io commented Sep 26, 2017 • edited Loading

cengiz-io commented Sep 27, 2017

nikomatsakis commented Sep 27, 2017

nikomatsakis commented Sep 27, 2017

cengiz-io commented Oct 2, 2017 • edited Loading

nikomatsakis commented Oct 3, 2017

cengiz-io commented Oct 8, 2017

gaurikholkar-zz commented Oct 8, 2017

cengiz-io commented Oct 8, 2017 • edited Loading

cengiz-io commented Oct 8, 2017

gaurikholkar-zz commented Oct 9, 2017

gaurikholkar-zz commented Oct 9, 2017

bors commented Nov 9, 2017

kennytm commented Nov 9, 2017

nikomatsakis commented Nov 10, 2017

Mark-Simulacrum commented Nov 11, 2017

nikomatsakis commented Nov 11, 2017

nikomatsakis commented Nov 12, 2017

bors commented Nov 12, 2017

bors commented Nov 12, 2017

bors commented Nov 12, 2017

nikomatsakis commented Nov 12, 2017

bors commented Nov 12, 2017

bors commented Nov 12, 2017

bors commented Nov 12, 2017

cengiz-io commented Aug 29, 2017 •

edited

Loading

arielb1 commented Sep 5, 2017 •

edited

Loading

arielb1 commented Sep 19, 2017 •

edited

Loading

cengiz-io commented Sep 26, 2017 •

edited

Loading

cengiz-io commented Oct 2, 2017 •

edited

Loading

cengiz-io commented Oct 8, 2017 •

edited

Loading