Implement in-band lifetime bindings #46051

cramertj · 2017-11-17T07:02:40Z

TODO (perhaps in a future PR): Should we ban explicit instantiation of generics with in-band lifetimes, or is it uncontroversial to just append them to the end of the lifetimes list?

Fixes #46042, cc #44524.

r? @nikomatsakis

nikomatsakis

OK, the high-level strategy in this code makes sense to me, though I'd like to see it better documented. I think the most important thing though is to expand the set of tests. I went through and wrote-up all the tests I could think of based on a reading of the spec here in this gist. Do you think you would have time to add such tests and see whether your code behaves in the right way? (If not, I can take a stab at adding the tests at some point, but not tonight!)

nikomatsakis · 2017-11-17T20:19:40Z

src/librustc/hir/lowering.rs

@@ -105,6 +105,13 @@ pub struct LoweringContext<'a> {
    is_in_loop_condition: bool,
    is_in_trait_impl: bool,

+    // Used to create lifetime definitions from in-band lifetime usages.
+    // e.g. `fn foo(x: &'x u8) -> &'x u8` to `fn foo<'x>(x: &'x u8) -> &'x u8`
+    lifetimes_to_define: Vec<(Span, Name)>,


This comment is a bit sparse. Could you say a bit about how it's used?

nikomatsakis · 2017-11-17T20:19:57Z

src/librustc/hir/lowering.rs

+    // Used to create lifetime definitions from in-band lifetime usages.
+    // e.g. `fn foo(x: &'x u8) -> &'x u8` to `fn foo<'x>(x: &'x u8) -> &'x u8`
+    lifetimes_to_define: Vec<(Span, Name)>,
+    is_collecting_in_band_lifetimes: bool,


Similarly here and below.

nikomatsakis · 2017-11-17T20:22:25Z

src/librustc/hir/lowering.rs

+        (lifetime_defs, res)
+    }
+
+    fn with_out_of_band_lifetime_defs<T, F>(


Comment plz.

nikomatsakis · 2017-11-17T20:26:21Z

src/librustc/hir/lowering.rs

+        f: F
+    ) -> (Vec<hir::LifetimeDef>, T) where F: FnOnce(&mut LoweringContext) -> T
+    {
+        self.lifetimes_to_define = Vec::new();


Seems a bit odd to put a new vector but here but a few lines down to use split_off (presumably with intention of re-using the backing buffer). Maybe just assert!(self.lifetimes_to_define.is_empty())?

nikomatsakis · 2017-11-17T21:51:26Z

src/librustc/hir/lowering.rs

+                            name: hir::LifetimeName::Name(name),
+                        },
+                        bounds: Vec::new().into(),
+                        pure_wrt_drop: false,


I guess it's a FIXME to include some sort of synthetic field here, right? I definitely think we should do that.

bors · 2017-11-20T11:02:12Z

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

cramertj · 2017-11-20T20:35:00Z

@nikomatsakis I've addressed the review comments and added a bunch of tests to cover the cases described in your gist.

nikomatsakis

I found a few nits in the tests. Also, I didn't see a test for intermingled lists, did I?

e.g., fn foo<'a>(x: &'a u32, y: &'b u32)?

nikomatsakis · 2017-11-20T20:33:12Z

src/test/ui/in-band-lifetimes/mismatched.rs

+#![allow(warnings)]
+#![feature(in_band_lifetimes)]
+
+fn foo(x: &'a u32, y: u32) -> &'a u32 { y }


Nit: I think this was supposed to be y: &u32, to show that it works the same for anon and named

nikomatsakis · 2017-11-20T20:33:41Z

src/test/ui/in-band-lifetimes/mismatched_trait.stderr

+error[E0621]: explicit lifetime required in the type of `y`
+  --> $DIR/mismatched_trait.rs:16:9
+   |
+15 |     fn baz(&self, x: &'a u32, y: &u32) -> &'a u32 {


Love it. Happy the errors still work. Not that there is any reason they wouldn't.

nikomatsakis · 2017-11-20T20:34:34Z

src/test/ui/in-band-lifetimes/no_in_band_in_struct.stderr

+  --> $DIR/no_in_band_in_struct.rs:15:9
+   |
+15 |     x: &'test u32,
+   |         ^^^^^ undeclared lifetime


I wonder if we should give some better tip here. Seems orthogonal to your PR, but might be worth opening a follow-up pointing to the struct and suggesting that the lifetime parameter be added there.

cramertj · 2017-11-20T21:04:10Z

@nikomatsakis

Also, I didn't see a test for intermingled lists, did I?

That's the test for E0688.

nikomatsakis · 2017-11-20T21:04:35Z

@cramertj ok great!

nikomatsakis

r=me with one last test (sorry, I'm a pain)

nikomatsakis · 2017-11-20T21:09:27Z

src/test/ui/in-band-lifetimes/E0687.rs

+
+fn foo(x: fn(&'a u32)) {}
+
+fn bar(x: &Fn(&'a u32)) {}


OK, one last set of tests I would like to see -- I didn't notice this particular scenario. I'm 99.9% sure it works correctly, but it seems like we might as well be complete!

fn bar<F>(x: &F) where F: Fn(&'a u32) { } //~ ERROR fn bar(x: &impl Fn(&'a u32)) //~ ERROR

But:

fn bar<F>(x: &F) // OK where F: SomeTrait<'a> fn bar(x: &impl SomeTrait<'a>) // OK

If you are too busy, I can try adding them, since I'd love to r+ ASAP. =)

nikomatsakis · 2017-11-21T00:26:03Z

@bors r+

bors · 2017-11-21T00:26:03Z

📌 Commit 8ea891d has been approved by nikomatsakis

bors · 2017-11-21T12:36:28Z

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

I also added some comments explaining what is going on. In short, the changes in question do not, in fact, affect the`TypeckTables` in any semantic way. However, altering the order of lowering can cause it appear to affect the `TypeckTables`: if we lower generics before the body, then the `HirId` for things in the body will be affected. In this case, we are now lowering the generics etc *after* the body, so the hash no longer changes. This seems good.

kennytm · 2017-11-22T17:50:44Z

There's still merge conflict.

nikomatsakis · 2017-11-22T20:26:22Z

@bors r+

bors · 2017-11-22T20:26:23Z

📌 Commit 79bf7db has been approved by nikomatsakis

bors · 2017-11-23T08:20:38Z

⌛ Testing commit 79bf7db with merge 247d98e...

@nikomatsakis

Implement in-band lifetime bindings TODO (perhaps in a future PR): Should we ban explicit instantiation of generics with in-band lifetimes, or is it uncontroversial to just append them to the end of the lifetimes list? Fixes #46042, cc #44524. r? @nikomatsakis

bors · 2017-11-23T10:45:57Z

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

rust-highfive assigned nikomatsakis Nov 17, 2017

cramertj force-pushed the in-band-lifetimes branch 2 times, most recently from c011bd9 to 4a9bac5 Compare November 17, 2017 07:25

petrochenkov self-assigned this Nov 17, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 17, 2017

nikomatsakis requested changes Nov 18, 2017

View reviewed changes

cramertj force-pushed the in-band-lifetimes branch from 4a9bac5 to 22dcad6 Compare November 18, 2017 20:04

cramertj force-pushed the in-band-lifetimes branch 2 times, most recently from bd74337 to e45d3ad Compare November 20, 2017 19:54

nikomatsakis requested changes Nov 20, 2017

View reviewed changes

cramertj force-pushed the in-band-lifetimes branch from e45d3ad to 893840d Compare November 20, 2017 20:58

cramertj mentioned this pull request Nov 20, 2017

Suggest adding undeclared lifetime parameters to struct #46135

Closed

nikomatsakis approved these changes Nov 20, 2017

View reviewed changes

nikomatsakis requested changes Nov 20, 2017

View reviewed changes

cramertj force-pushed the in-band-lifetimes branch from 893840d to 8ea891d Compare November 21, 2017 00:10

nikomatsakis approved these changes Nov 21, 2017

View reviewed changes

cramertj force-pushed the in-band-lifetimes branch from 8ea891d to 9775263 Compare November 21, 2017 04:25

ghost mentioned this pull request Nov 21, 2017

Remove Atomic's _owned methods crossbeam-rs/crossbeam-epoch#35

Merged

nikomatsakis force-pushed the in-band-lifetimes branch 2 times, most recently from a85a453 to 9775263 Compare November 22, 2017 17:09

cramertj and others added 3 commits November 22, 2017 12:44

Implement in-band lifetime bindings

91b7920

add debug log with result of writeback

78ccbba

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 22, 2017

add some tests for the interaction with existential impl trait

79bf7db

nikomatsakis force-pushed the in-band-lifetimes branch from 1e0af43 to 79bf7db Compare November 22, 2017 18:46

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 22, 2017

oli-obk mentioned this pull request Nov 23, 2017

Check //~ERROR comments in ui tests #46116

Merged

bors merged commit 79bf7db into rust-lang:master Nov 23, 2017

bors mentioned this pull request Nov 23, 2017

Generic Associated Types Parsing & Name Resolution #45904

Merged

3 tasks

petrochenkov removed their assignment Nov 26, 2017

cramertj deleted the in-band-lifetimes branch December 16, 2017 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement in-band lifetime bindings #46051

Implement in-band lifetime bindings #46051

cramertj commented Nov 17, 2017 •

edited

Loading

nikomatsakis left a comment

nikomatsakis Nov 17, 2017

nikomatsakis Nov 17, 2017

nikomatsakis Nov 17, 2017

nikomatsakis Nov 17, 2017

nikomatsakis Nov 17, 2017

bors commented Nov 20, 2017

cramertj commented Nov 20, 2017

nikomatsakis left a comment

nikomatsakis Nov 20, 2017

cramertj Nov 20, 2017

nikomatsakis Nov 20, 2017

nikomatsakis Nov 20, 2017

cramertj Nov 20, 2017

cramertj commented Nov 20, 2017

nikomatsakis commented Nov 20, 2017

nikomatsakis left a comment

nikomatsakis Nov 20, 2017

cramertj Nov 21, 2017

nikomatsakis commented Nov 21, 2017

bors commented Nov 21, 2017

bors commented Nov 21, 2017

kennytm commented Nov 22, 2017

nikomatsakis commented Nov 22, 2017

bors commented Nov 22, 2017

bors commented Nov 23, 2017

bors commented Nov 23, 2017

Implement in-band lifetime bindings #46051

Implement in-band lifetime bindings #46051

Conversation

cramertj commented Nov 17, 2017 • edited Loading

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

bors commented Nov 20, 2017

cramertj commented Nov 20, 2017

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

cramertj commented Nov 20, 2017

nikomatsakis commented Nov 20, 2017

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

nikomatsakis commented Nov 21, 2017

bors commented Nov 21, 2017

bors commented Nov 21, 2017

kennytm commented Nov 22, 2017

nikomatsakis commented Nov 22, 2017

bors commented Nov 22, 2017

bors commented Nov 23, 2017

bors commented Nov 23, 2017

cramertj commented Nov 17, 2017 •

edited

Loading