Suggestion for 'static impl Trait return #51444

estebank · 2018-06-08T22:27:20Z

When encountering a named or anonymous sup requirement (for example,
&'a self) and a 'static impl Trait return type, suggest adding the
'_ lifetime constraing to the return type.

Fix #43719, #51282.

error: cannot infer an appropriate lifetime
  --> $DIR/static-return-lifetime-infered.rs:17:16
   |
LL |     fn iter_values_anon(&self) -> impl Iterator<Item=u32> {
   |                                   ----------------------- this return type evaluates to the `'static` lifetime...
LL |         self.x.iter().map(|a| a.0)
   |         ------ ^^^^
   |         |
   |         ...but this borrow...
   |
note: ...can't outlive the anonymous lifetime #1 defined on the method body at 16:5
  --> $DIR/static-return-lifetime-infered.rs:16:5
   |
LL | /     fn iter_values_anon(&self) -> impl Iterator<Item=u32> {
LL | |         self.x.iter().map(|a| a.0)
LL | |     }
   | |_____^
help: you can add a constraint to the return type to make it last less than `'static` and match the anonymous lifetime #1 defined on the method body at 16:5
   |
LL |     fn iter_values_anon(&self) -> impl Iterator<Item=u32> + '_ {
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

When encountering a named or anonymous sup requirement (for example, `&'a self`) and a `'static` impl Trait return type, suggest adding the `'_` lifetime constraing to the return type.

rust-highfive · 2018-06-08T22:27:23Z

r? @cramertj

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

cramertj · 2018-06-08T22:47:05Z

src/test/ui/impl-trait/static-return-lifetime-infered.stderr

+LL |     fn iter_values_anon(&self) -> impl Iterator<Item=u32> {
+   |                                   ----------------------- this return type evaluates to the `'static` lifetime...
+LL |         self.x.iter().map(|a| a.0)
+   |         ------ ^^^^ can't infer an appropriate lifetime


Nit: this cant infer an appropriate lifetime hint seems out-of-place to me. it interjects in the middle of the sentence "this return type evaluates to the 'static lifetime but this borrow...". Perhaps move it or combine it somehow with the "can't outlive the anonymous lifetime" error?

Agree. I tried a couple of different outputs, including:

error: can't infer an appropriate lifetime --> file.rs:11:16 | 11 | self.x.iter().map(|a| a.0) | ^^^^ can't infer an appropriate lifetime for this | note: this return type evaluates to the `'static` lifetime... --> file.rs:10:35 | 10 | fn iter_values_anon(&self) -> impl Iterator<Item=u32> { | ^^^^^^^^^^^^^^^^^^^^^^^ note: ...but this borrow... --> file.rs:11:9 | 11 | self.x.iter().map(|a| a.0) | ^^^^^^ note: ...can't outlive the anonymous lifetime #1 defined on the method body at 10:5 --> file.rs:10:5 | 10 | / fn iter_values_anon(&self) -> impl Iterator<Item=u32> { 11 | | self.x.iter().map(|a| a.0) 12 | | } | |_____^ help: you can add a constraint to make the `impl Trait` non`'static` and match the anonymous lifetime #1 defined on the method body at 10:5 | 10 | fn iter_values_anon(&self) -> impl Iterator<Item=u32> + '_ { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I feel that the current output without any primary label would look reasonably good.

I actually like what you have above, but I think you could remove the "can't infer an appropriate lifetime for this" at the top, since that's covered in the "but this borrow... can't outlive the anonymous lifetime".

Can you take a look at the current output? I removed the primary label and it does seem slightly easier to read.

cramertj · 2018-06-08T22:47:12Z

src/librustc/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                                     less than `'static` and match {}",
+                                    lifetime,
+                                ),
+                                format!("{} + '_", snippet),


In the case where the lifetime is named, think it would be more useful to recommend adding a named lifetime here (rather than '_).

Agree somewhat, the only reason I didn't implement it that way is that from my cursory tests '_ always works, but I could contrive a case where 'a wouldn't. That being said, I can change it if you feel strongly enough (or we could also leave that as a follow up task).

That makes sense. Thanks for the investigation!

Changed to suggest 'a.

estebank · 2018-06-11T02:24:28Z

cc @nikomatsakis you might be interested in providing feedback here too.

cramertj · 2018-06-11T16:11:03Z

r=me once @nikomatsakis has had a chance to look, too. I still feel strange about recommending + '_ in cases like fn foo(x: &'a u8, y: &'a u8) -> impl Trait. I considered suggesting that we check the lifetime of the return type against the input lifetimes to the function, but that gets complicated once you have multiple lifetimes with no outlives relations, where the result could be any of the input lifetimes or an entirely separate lifetime that two or three of them outlive.

estebank · 2018-06-11T16:36:25Z

@cramertj I can add the code to do that in the trivial case (self has only one lifetime), and give up with the current diagnostic otherwise. That would make it easy to implement with a fallback not worse than what we already have.

cramertj · 2018-06-11T17:38:21Z

@estebank That sounds worth it to me! By "self has only one lifetime", you mean the case where all lifetimes in input types are named and identical?

estebank · 2018-06-11T17:45:21Z

@cramertj right

pietroalbini · 2018-06-18T09:24:47Z

r? @nikomatsakis

estebank · 2018-06-20T20:03:14Z

Ping.

nikomatsakis · 2018-06-20T20:36:19Z

src/librustc/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                ) => {
+                    let anon_reg_sup = self.is_suitable_region(sup_r)?;
+                    if sub_r == &RegionKind::ReStatic &&
+                        self._is_return_type_impl_trait(anon_reg_sup.def_id)


Nit: We don't normally use method names with a leading _.

nikomatsakis · 2018-06-20T20:40:04Z

src/librustc/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                    sup_r,
+                ) => {
+                    let anon_reg_sup = self.is_suitable_region(sup_r)?;
+                    if sub_r == &RegionKind::ReStatic &&


Are we just concluding that any time that we have a bound of 'static and a fn with an impl Trait return, the two are connected?

e.g., what happens with this example?

use std::fmt::Debug; fn foo(x: &u32) -> impl Debug { let _: &'static u32 = x; () }

Are we just concluding that any time that we have a bound of 'static and a fn with an impl Trait return, the two are connected?

Close, unless I misunderstand, the code checks that the return type is impl Trait and that it is bound to 'static.

what happens with

The two provided examples keep the current output:

error[E0621]: explicit lifetime required in the type of `x` --> file.rs:3:25 | 2 | fn foo(x: &u32) -> impl Debug { | - consider changing the type of `x` to `&'static u32` 3 | let _: &'static u32 = x; | ^ lifetime `'static` required error[E0621]: explicit lifetime required in the type of `x` --> file.rs:7:29 | 7 | fn bar(x: &u32, y: &u32) -> impl Debug { | - ^^^^^^^^^^ lifetime `'static` required | | | consider changing the type of `x` to `&'static u32`

For the second case, we could expand this code handle it and suggest the following, but it'll be very involved:

fn bar<'a>(x: &'a u32, y: &u32) -> impl Debug + 'a { x }

nikomatsakis · 2018-06-20T20:43:20Z

I agree with @cramertj that always suggesting '_ feels wrong. I believe that in we can check out sub to see whether it is (e.g.) a named region or not -- '_ is basically only right in the case where sub is either the only region in the arguments or appears in self.

Another related example:

use std::fmt::Debug;
fn foo(x: &u32, y: &u32) -> impl Debug {
  x
}

if we suggest '_ here, that will result in an error, which seems suboptimal.

estebank · 2018-06-20T22:40:31Z

Output for the two samples doesn't change:

error[E0621]: explicit lifetime required in the type of `x`
 --> file.rs:3:25
  |
2 | fn foo(x: &u32) -> impl Debug {
  |        - consider changing the type of `x` to `&'static u32`
3 |   let _: &'static u32 = x;
  |                         ^ lifetime `'static` required

error[E0621]: explicit lifetime required in the type of `x`
 --> file.rs:7:29
  |
7 | fn bar(x: &u32, y: &u32) -> impl Debug {
  |        -                    ^^^^^^^^^^ lifetime `'static` required
  |        |
  |        consider changing the type of `x` to `&'static u32`

Now using the lifetime name if available, as reflected in the test:

error: cannot infer an appropriate lifetime
  --> $DIR/static-return-lifetime-infered.rs:21:16
   |
LL |     fn iter_values<'a>(&'a self) -> impl Iterator<Item=u32> {
   |                                     ----------------------- this return type evaluates to the `'static` lifetime...
LL |         self.x.iter().map(|a| a.0)
   |         ------ ^^^^
   |         |
   |         ...but this borrow...
   |
note: ...can't outlive the lifetime 'a as defined on the method body at 20:5
  --> $DIR/static-return-lifetime-infered.rs:20:5
   |
LL |     fn iter_values<'a>(&'a self) -> impl Iterator<Item=u32> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: you can add a constraint to the return type to make it last less than `'static` and match the lifetime 'a as defined on the method body at 20:5
   |
LL |     fn iter_values<'a>(&'a self) -> impl Iterator<Item=u32> + 'a {
   |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

pietroalbini · 2018-06-25T15:22:25Z

Ping from triage @nikomatsakis! This PR needs your review.

nikomatsakis · 2018-06-25T21:46:30Z

Ok, try this example @estebank

use std::fmt::Debug;

struct Foo { }

impl Foo {
    fn foo(&self) -> impl Debug {
        let _: &'static Foo = self;
        ()
    }
}

fn main() { }

I think the reason that this code didn't fire is because a "better" pretty-printed region error was triggering.

nikomatsakis · 2018-06-25T21:53:07Z

src/librustc/infer/error_reporting/nice_region_error/static_impl_trait.rs

+                ) => {
+                    let anon_reg_sup = self.is_suitable_region(sup_r)?;
+                    if sub_r == &RegionKind::ReStatic &&
+                        self.is_return_type_impl_trait(anon_reg_sup.def_id)


I think that it would be better if we could somehow detect that the 'static was actually coming from the impl trait (as opposed to some other source). One way to do that might be to make use of the sub_origin variable -- but we'd have to update the origin that appears here:

rust/src/librustc/infer/anon_types/mod.rs

Line 367 in b7c6e8f

let bound_region = |r| self.sub_regions(infer::CallReturn(span), least_region, r);

and maybe here too, though that could come later and is a bit more annoying (since that is an obligation cause). The current values are totally bogus anyway: they claim that impl Trait requirements arise "because returns types must be sized", which is just..not even kind of true.

Anyway, if we added a new SubregionOrigin for "impl trait obligation" and then checked here for that case, I think this would continue to work in the cases that we want, but rule out the cases we don't.

That said, I am not that keen on adding more uses of SubregionOrigin, which is "deprecated" (essentially) in the NLL checker. Still, I'm not sure what better thing to use for now.

estebank · 2018-06-25T22:32:45Z

The output for

use std::fmt::Debug;

struct Foo { }

impl Foo {
    fn foo(&self) -> impl Debug {
        let _: &'static Foo = self;
        ()
    }
}

fn main() { }

is

error[E0312]: lifetime of reference outlives lifetime of borrowed content...
 --> file.rs:7:31
  |
7 |         let _: &'static Foo = self;
  |                               ^^^^
  |
  = note: ...the reference is valid for the static lifetime...
note: ...but the borrowed content is only valid for the anonymous lifetime #1 defined on the method body at 6:5
 --> file.rs:6:5
  |
6 | /     fn foo(&self) -> impl Debug {
7 | |         let _: &'static Foo = self;
8 | |         ()
9 | |     }
  | |_____^

nikomatsakis · 2018-06-26T21:57:57Z

@estebank huh, ok. Why doesn't your new error message trigger in that case?

estebank · 2018-06-26T23:27:04Z

Why doesn't your new error message trigger in that case?

Still checking, but it seems that case is not a SubSupConflict.

It doesn't trigger because it only triggers on SubSupConflict, but the last sample is a ConcreteFailure(Reborrow(file.rs:7:31: 7:35), ReStatic, ReFree(DefId(0/0:6 ~ file[317d]::{{impl}}[0]::foo[0]), BrAnon(0))). The other samples don't even call try_report_static_impl_trait.

I feel pretty comfortable merging this because it is purposely restricted to a specific case which is also a common pitfall for people trying impl Trait.

nikomatsakis · 2018-06-27T22:16:05Z

OK, I still feel like the test is too broad, but I guess we can wait for someone to report problems to us.

nikomatsakis · 2018-06-27T22:16:19Z

@bors r+

bors · 2018-06-27T22:16:20Z

📌 Commit 612657d has been approved by nikomatsakis

Suggestion for 'static impl Trait return When encountering a named or anonymous sup requirement (for example, `&'a self`) and a `'static` impl Trait return type, suggest adding the `'_` lifetime constraing to the return type. Fix #43719, #51282. ``` error: cannot infer an appropriate lifetime --> $DIR/static-return-lifetime-infered.rs:17:16 | LL | fn iter_values_anon(&self) -> impl Iterator<Item=u32> { | ----------------------- this return type evaluates to the `'static` lifetime... LL | self.x.iter().map(|a| a.0) | ------ ^^^^ | | | ...but this borrow... | note: ...can't outlive the anonymous lifetime #1 defined on the method body at 16:5 --> $DIR/static-return-lifetime-infered.rs:16:5 | LL | / fn iter_values_anon(&self) -> impl Iterator<Item=u32> { LL | | self.x.iter().map(|a| a.0) LL | | } | |_____^ help: you can add a constraint to the return type to make it last less than `'static` and match the anonymous lifetime #1 defined on the method body at 16:5 | LL | fn iter_values_anon(&self) -> impl Iterator<Item=u32> + '_ { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ```

bors · 2018-06-28T03:57:35Z

⌛ Testing commit 612657d with merge ba196bd...

bors · 2018-06-28T06:06:26Z

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

Suggestion for 'static impl Trait return

82d3a49

When encountering a named or anonymous sup requirement (for example, `&'a self`) and a `'static` impl Trait return type, suggest adding the `'_` lifetime constraing to the return type.

rust-highfive assigned cramertj Jun 8, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2018

cramertj reviewed Jun 8, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Remove primary label for more readable output

6be16ba

This comment has been minimized.

Sign in to view

Add nll test output

aaf78a5

rust-highfive assigned nikomatsakis and unassigned cramertj Jun 18, 2018

nikomatsakis reviewed Jun 20, 2018

View reviewed changes

Use lifetime name if available

612657d

nikomatsakis requested changes Jun 25, 2018

View reviewed changes

nikomatsakis approved these changes Jun 27, 2018

View reviewed changes

bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2018

bors merged commit 612657d into rust-lang:master Jun 28, 2018

bors mentioned this pull request Jun 28, 2018

HirId-ification, continued #51321

Merged

estebank mentioned this pull request Aug 28, 2018

NLL regresses diagnostic for impl-trait/static-return-lifetime-infered.rs #53771

Closed

estebank deleted the impl-static branch November 9, 2023 05:24

Suggestion for 'static impl Trait return #51444

Suggestion for 'static impl Trait return #51444

Conversation

estebank commented Jun 8, 2018 • edited Loading

rust-highfive commented Jun 8, 2018

Choose a reason for hiding this comment

estebank Jun 8, 2018 • edited Loading

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

cramertj Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

estebank commented Jun 11, 2018

cramertj commented Jun 11, 2018

estebank commented Jun 11, 2018

cramertj commented Jun 11, 2018

estebank commented Jun 11, 2018

pietroalbini commented Jun 18, 2018

estebank commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jun 20, 2018

estebank commented Jun 20, 2018

pietroalbini commented Jun 25, 2018

nikomatsakis commented Jun 25, 2018

Choose a reason for hiding this comment

estebank commented Jun 25, 2018

nikomatsakis commented Jun 26, 2018

estebank commented Jun 26, 2018 • edited Loading

nikomatsakis commented Jun 27, 2018

nikomatsakis commented Jun 27, 2018

bors commented Jun 27, 2018

bors commented Jun 28, 2018

bors commented Jun 28, 2018

estebank commented Jun 8, 2018 •

edited

Loading

estebank Jun 8, 2018 •

edited

Loading

cramertj Jun 8, 2018 •

edited

Loading

estebank commented Jun 26, 2018 •

edited

Loading