adding E0623 for return types - both parameters are anonymous #44124

gaurikholkar-zz · 2017-08-28T07:40:31Z

This is a fix for #44018

error[E0621]: explicit lifetime required in the type of `self`
  --> $DIR/ex3-both-anon-regions-return-type-is-anon.rs:17:5
   |
16 |   fn foo<'a>(&self, x: &i32) -> &i32 {
   |                        ----     ----
   |                        |
   |                        this parameter and the return type are
                            declared with different lifetimes...
17 |     x
   |     ^ ...but data from `x` is returned here

error: aborting due to previous error

It also works for the below case where we have self as anonymous

error[E0623]: lifetime mismatch
  --> src/test/ui/lifetime-errors/ex3-both-anon-regions-self-is-anon.rs:17:19
   |
16 |     fn foo<'a>(&self, x: &Foo) -> &Foo {
   |                          ----     ----
   |                          |
   |                          this parameter and the return type are
                            declared with different lifetimes...
17 |         if true { x } else { self }
   |                   ^ ...but data from `x` is returned here

error: aborting due to previous error

r? @nikomatsakis

Currently, I have enabled E0621 where return type and self are anonymous, hence WIP.

arielb1 · 2017-08-28T12:28:57Z

src/test/ui/lifetime-errors/ex3-both-anon-regions-earlybound-regions.rs

@@ -0,0 +1,6 @@
+


missing license and newlines in the code

also, shouldn't this test go with the other PR?

Yeah. Was a mistake

nikomatsakis

First round of suggestions.

nikomatsakis · 2017-08-29T12:13:04Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs


-        let (main_label, label1, label2) = if let (Some(sup_arg), Some(sub_arg)) =
+        let (spanfin1, spanfin2, main_label, label) = if let (Some(sup_arg),
+                                                                       Some(sub_arg)) =


Nit: can we use use the flatter style here too? I really find the if let nesting hard to read. In other words, change the if let (Some(sup_arg), Some(sub_arg)) = (...) { ... } else { false } too:

let sup_arg = or_false!(self.find_arg_with_anonymous_region(sup, sup)); let sub_arg = or_false!(self.find_arg_with_anonymous_region(sub, sub)); ...

nikomatsakis · 2017-08-29T12:14:51Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

-               self.is_return_type_anon(scope_def_id_sub, bregion_sub) {
-                return false;
-            }
+            let (anon_arg_sup, anon_arg_sub) = (sup_arg.arg, sub_arg.arg);


If we do the or_false! transformation above, and we don't care about the other field, we could do:

let AnonymousArgInfo { arg: anon_arg_sup, .. } = or_false!(self.find_arg_with_anonymous_region(sup, sup));

Not sure if this is a net win.

I'm a bit confused about what's happening here

nikomatsakis · 2017-08-29T12:22:42Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

-                 format!(" with one lifetime"),
-                 format!(" into the other"))
+                let (span_1, span_2, span_label) =
+                    if self.is_return_type_anon(scope_def_id_sup, bregion_sup, ty_fndecl_sup)


Hmm, this logic is sort of complex. First off, let's store self.is_return_type_anon(scope_def_id_sup, bregion_sup, ty_fndecl_sup) into a variable:

let sup_is_ret_type = self.is_return_type_anon(scope_def_id_sup, bregion_sup, ty_fndecl_sup);

and same for sub. Then I think I would structure the remaining logic something like this:

let sup_is_ret_type = self.is_return_type_anon(scope_def_id_sup, bregion_sup, ty_fndecl_sup); let sub_is_ret_type = self.is_return_type_anon(scope_def_id_sub, bregion_sub, ty_fndecl_sub); let (span_1, span_2, span_label) = match (sup_is_ret_type, sub_is_ret_type) { (None, None) => (ty_sup.span, ty_sub.span, format!("this type was declared with multiple lifetimes...")), (Some(ret_span), _) => (ty_sub.span, ret_span, format!("this parameter and the return type are declared with different lifetimes...")), (_, Some(ret_span)) => (ty_sup.span, ret_span, format!("this parameter and the return type are declared with different lifetimes...")), };

However, this raises the question -- what if both sub and sup appear in the return type? The existing code seems to ignore this case, but it's probably worth adding something like this:

(Some(ret_span1), Some(ret_span2)) => (ret_span1, ret_span2, format!("the return type is declared with different lifetimes...")),

That said, maybe better to wait until we have a test for this case, since I'm not sure off-hand how it can really arise.

nikomatsakis · 2017-08-29T12:22:57Z

src/librustc/infer/error_reporting/anon_anon_conflict.rs

@@ -91,19 +118,50 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                    format!("")
                };

-                let span_label =
-                    format!("these two types are declared with different lifetimes...",);
+                let (var1_span, var2_span, span_label_1, span_label_2) = if


I would restructure this in the same way I as suggested above.

nikomatsakis · 2017-08-29T14:18:47Z

src/librustc/infer/error_reporting/named_anon_conflict.rs

+            if self.is_return_type_anon(scope_def_id, br, fndecl).is_some() ||
+               self.is_self_anon(is_first, scope_def_id) {
+               return false;
+            } }


Nit: } on next line

alexcrichton · 2017-09-07T23:30:10Z

@gaurikholkar ping just to keep this on your radar!

gaurikholkar-zz · 2017-09-08T02:58:40Z

@alexcrichton getting back at it today!

bors · 2017-09-10T18:38:24Z

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

nikomatsakis · 2017-09-13T08:38:39Z

@gaurikholkar so since I had a local checkout of your branch I went ahead and corrected a few nits in the wording of the error messages. However, it looks like now that you've made things better, the example for E0312 isn't working anymore.

I'm a bit torn here --- I feel like most every example we can craft for E0312 is probably just a bug waiting to be fixed (i.e., a case where we can make a better example). But I guess in the interim we can keep shuffling it from bug to bug.

Do you have any instinct for what example would still trigger E0312? Maybe something involving self...?

gaurikholkar-zz · 2017-09-13T08:44:47Z

Are you referring to the you tests w.r.t self, return type and impls? Now that they trigger E0623? Also do we want that for impls?

nikomatsakis · 2017-09-14T14:14:13Z

@gaurikholkar

Are you referring to the you tests w.r.t self, return type and impls? Now that they trigger E0623? Also do we want that for impls?

Sorry, I don't really understand the question. What I am saying is that the example for E0312 found in the extended error message (the one with 'tree and 'human etc) now generates a nice error, instead of E0312. Therefore, to keep the build passing, we have to do one of two things:

Replace that example with something that generates E0312. I'm not sure what that should be. The example will have to get somewhat more convoluted, I imagine. This strikes me as overall a good thing, since we think the newer errors are better than E0312, so I am glad that it is hard to make an example that shows E0312.
- Basically, I think that any example which still shows E0312 can be considered a bug. This implies that we will have to play a game of "whack-a-mole" -- each time we have an example for E0312, we will eventually fix it to display the other error, and then we have to find a new (even more obscure) example for E0312.
Remove the extended error message for E0312 (and perhaps the error itself). At some point, when we tire of playing whack-a-mole, we can just remove the extended error message. Even better would be to remove the code that generates E0312 altogether, if we could guarantee it will not run.

Probably we are not yet at that point where it's worth removing the extended error msg for E0312. I imagine we can find some kind of example that still fails.

arielb1 · 2017-09-19T20:45:29Z

Hi @gaurikholkar. How it's going with this PR? Is there anything blocking you? Feel free to ping me on IRC.

arielb1 · 2017-09-24T09:00:13Z

One small mismatch in a test:

[00:42:51]  error[E0623]: lifetime mismatch
[00:42:51]    --> $DIR/ex3-both-anon-regions-both-are-structs-4.rs:16:11
[00:42:51]     |
[00:42:51]  15 | fn foo(mut x: Ref) {
[00:42:51]     |               ---
[00:42:51]     |               |
[00:42:51] -   |               this type was declared with multiple lifetimes...
[00:42:51] +   |               this type is declared with multiple lifetimes...
[00:42:51]  16 |     x.a = x.b;

arielb1 · 2017-09-24T10:59:01Z

src/librustc/infer/error_reporting/named_anon_conflict.rs

-                   is_first,
-                   scope_def_id);
-            return false;
+        if let Some(anon_ty) = self.find_anon_type(anon, &br) {


style: if let Some((_, fndecl)) = self.find_anon_type(anon, &br)

arielb1 · 2017-09-24T11:03:22Z

LGTM modulo nits

nikomatsakis · 2017-09-26T15:20:44Z

The travis failure is confusing me. I checked out the branch and don't see that file in my copy. Maybe I'm doing something bone-headed...

gaurikholkar-zz · 2017-09-26T15:34:40Z

@nikomatsakis yeah even I couldn't find the file :(

nikomatsakis · 2017-09-26T15:55:01Z

@bors r+

That kind of formatting seems like the job of other code.

arielb1 · 2017-09-26T18:43:11Z

@bors r+

bors · 2017-09-26T18:43:12Z

📌 Commit 73543d5 has been approved by arielb1

arielb1 · 2017-09-26T18:43:29Z

@bors rollup

gaurikholkar-zz · 2017-09-29T13:48:07Z

When does this get merged?

gaurikholkar-zz · 2017-09-29T17:06:25Z

@nikomatsakis

trait Foo {

    fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32;

}

impl Foo for () {

    fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {

        if x > y { x } else { y }

    }

}

I remember discussing that we need not change the message for Impls for the case of E0621, this PR modifies that. Should it be OK?

@nikomatsakis

adding E0623 for return types - both parameters are anonymous This is a fix for rust-lang#44018 ``` error[E0621]: explicit lifetime required in the type of `self` --> $DIR/ex3-both-anon-regions-return-type-is-anon.rs:17:5 | 16 | fn foo<'a>(&self, x: &i32) -> &i32 { | ---- ---- | | | this parameter and the return type are declared with different lifetimes... 17 | x | ^ ...but data from `x` is returned here error: aborting due to previous error ``` It also works for the below case where we have self as anonymous ``` error[E0623]: lifetime mismatch --> src/test/ui/lifetime-errors/ex3-both-anon-regions-self-is-anon.rs:17:19 | 16 | fn foo<'a>(&self, x: &Foo) -> &Foo { | ---- ---- | | | this parameter and the return type are declared with different lifetimes... 17 | if true { x } else { self } | ^ ...but data from `x` is returned here error: aborting due to previous error ``` r? @nikomatsakis Currently, I have enabled E0621 where return type and self are anonymous, hence WIP.

Rollup of 15 pull requests - Successful merges: #44124, #44287, #44320, #44694, #44708, #44794, #44797, #44824, #44836, #44840, #44845, #44854, #44889, #44900, #44903 - Failed merges:

bors · 2017-09-30T13:34:32Z

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

Mark-Simulacrum · 2017-09-30T15:02:42Z

Merged in #44936.

When there's a lifetime mismatch between an argument with an anonymous lifetime being returned in a method with a return type that has a named lifetime, show specialized lifetime error pointing at argument with a hint to give it an explicit lifetime matching the return type. ``` error[E0621]: explicit lifetime required in the type of `other` --> file2.rs:21:21 | 17 | fn bar(&self, other: Foo) -> Foo<'a> { | ----- consider changing the type of `other` to `Foo<'a>` ... 21 | other | ^^^^^ lifetime `'a` required ``` Follow up to rust-lang#44124 and rust-lang#42669.

Handle anon lifetime arg being returned with named lifetime return type When there's a lifetime mismatch between an argument with an anonymous lifetime being returned in a method with a return type that has a named lifetime, show specialized lifetime error pointing at argument with a hint to give it an explicit lifetime matching the return type. ``` error[E0621]: explicit lifetime required in the type of `other` --> file2.rs:21:21 | 17 | fn bar(&self, other: Foo) -> Foo<'a> { | ----- consider changing the type of `other` to `Foo<'a>` ... 21 | other | ^^^^^ lifetime `'a` required ``` Follow up to rust-lang#44124 and rust-lang#42669. Fix rust-lang#44684.

rust-highfive assigned nikomatsakis Aug 28, 2017

gaurikholkar-zz force-pushed the return_self branch from 52b358c to 70ff722 Compare August 28, 2017 07:41

gaurikholkar-zz changed the title ~~adding E0623 for return types~~ adding E0623 for return types - both parameters are anonymous Aug 28, 2017

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2017

arielb1 reviewed Aug 28, 2017

View reviewed changes

nikomatsakis requested changes Aug 29, 2017

View reviewed changes

nikomatsakis reviewed Aug 29, 2017

View reviewed changes

nikomatsakis 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 Aug 30, 2017

nikomatsakis mentioned this pull request Sep 8, 2017

Infer T: 'x outlives requirements on structs rust-lang/rfcs#2093

Merged

gaurikholkar-zz force-pushed the return_self branch 3 times, most recently from 07cf201 to 6d3ac8e Compare September 11, 2017 17:10

gaurikholkar-zz changed the title ~~adding E0623 for return types - both parameters are anonymous~~ adding E0623 for return types - both parameters are anonymous WIP Sep 11, 2017

nikomatsakis mentioned this pull request Sep 11, 2017

tracking issue for lifetime inference error work (E0495) #42516

Closed

19 tasks

gaurikholkar-zz changed the title ~~adding E0623 for return types - both parameters are anonymous WIP~~ adding E0623 for return types - both parameters are anonymous Sep 12, 2017

arielb1 reviewed Sep 24, 2017

View reviewed changes

nikomatsakis approved these changes Sep 26, 2017

View reviewed changes

gaurikholkar and others added 6 commits September 26, 2017 11:55

tidy fix

bbf82be

use present tense consistently and update references

d7bb575

remove random newlines from error messages, fix compile-fail test

9e4649e

That kind of formatting seems like the job of other code.

remove error code description

4bbb58d

minor fixes

5c59bba

fix test reference

73543d5

nikomatsakis force-pushed the return_self branch from 0f836ae to 73543d5 Compare September 26, 2017 15:56

cengiz-io mentioned this pull request Sep 26, 2017

Improve lifetime error with return value #44875

Closed

This was referenced Sep 29, 2017

properly handle anonymous regions appearing in return type- named-anon conflicts #42703

Open

E0623 not working for below examples #43875

Closed

Mark-Simulacrum mentioned this pull request Sep 29, 2017

Rollup of 15 pull requests #44936

Closed

Mark-Simulacrum closed this Sep 30, 2017

estebank mentioned this pull request Nov 4, 2017

Handle anon lifetime arg being returned with named lifetime return type #45751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding E0623 for return types - both parameters are anonymous #44124

adding E0623 for return types - both parameters are anonymous #44124

gaurikholkar-zz commented Aug 28, 2017 •

edited

Loading

arielb1 Aug 28, 2017

arielb1 Aug 28, 2017

gaurikholkar-zz Aug 28, 2017

nikomatsakis left a comment

nikomatsakis Aug 29, 2017

nikomatsakis Aug 29, 2017

gaurikholkar-zz Sep 8, 2017

nikomatsakis Aug 29, 2017

nikomatsakis Aug 29, 2017

nikomatsakis Aug 29, 2017

alexcrichton commented Sep 7, 2017

gaurikholkar-zz commented Sep 8, 2017

bors commented Sep 10, 2017

nikomatsakis commented Sep 13, 2017

gaurikholkar-zz commented Sep 13, 2017

nikomatsakis commented Sep 14, 2017

arielb1 commented Sep 19, 2017

arielb1 commented Sep 24, 2017

arielb1 Sep 24, 2017

arielb1 commented Sep 24, 2017

nikomatsakis commented Sep 26, 2017

gaurikholkar-zz commented Sep 26, 2017

nikomatsakis commented Sep 26, 2017

arielb1 commented Sep 26, 2017

bors commented Sep 26, 2017

arielb1 commented Sep 26, 2017

gaurikholkar-zz commented Sep 29, 2017

gaurikholkar-zz commented Sep 29, 2017

bors commented Sep 30, 2017

Mark-Simulacrum commented Sep 30, 2017

adding E0623 for return types - both parameters are anonymous #44124

adding E0623 for return types - both parameters are anonymous #44124

Conversation

gaurikholkar-zz commented Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 7, 2017

gaurikholkar-zz commented Sep 8, 2017

bors commented Sep 10, 2017

nikomatsakis commented Sep 13, 2017

gaurikholkar-zz commented Sep 13, 2017

nikomatsakis commented Sep 14, 2017

arielb1 commented Sep 19, 2017

arielb1 commented Sep 24, 2017

Choose a reason for hiding this comment

arielb1 commented Sep 24, 2017

nikomatsakis commented Sep 26, 2017

gaurikholkar-zz commented Sep 26, 2017

nikomatsakis commented Sep 26, 2017

arielb1 commented Sep 26, 2017

bors commented Sep 26, 2017

arielb1 commented Sep 26, 2017

gaurikholkar-zz commented Sep 29, 2017

gaurikholkar-zz commented Sep 29, 2017

bors commented Sep 30, 2017

Mark-Simulacrum commented Sep 30, 2017

gaurikholkar-zz commented Aug 28, 2017 •

edited

Loading