Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement in-band lifetime bindings #46051

Merged
merged 4 commits into from
Nov 23, 2017
Merged

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Nov 17, 2017

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

@cramertj cramertj force-pushed the in-band-lifetimes branch 2 times, most recently from c011bd9 to 4a9bac5 Compare November 17, 2017 07:25
@petrochenkov petrochenkov self-assigned this Nov 17, 2017
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 17, 2017
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

@@ -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)>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here and below.

(lifetime_defs, res)
}

fn with_out_of_band_lifetime_defs<T, F>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment plz.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bors
Copy link
Contributor

bors commented Nov 20, 2017

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

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

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

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

#![allow(warnings)]
#![feature(in_band_lifetimes)]

fn foo(x: &'a u32, y: u32) -> &'a u32 { y }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cramertj
Copy link
Member Author

@nikomatsakis

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

That's the test for E0688.

@nikomatsakis
Copy link
Contributor

@cramertj ok great!

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

fn bar(x: &Fn(&'a u32)) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. =)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 21, 2017

📌 Commit 8ea891d has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 21, 2017

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

@nikomatsakis 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
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 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
@kennytm
Copy link
Member

kennytm commented Nov 22, 2017

There's still merge conflict.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 22, 2017

📌 Commit 79bf7db has been approved by nikomatsakis

@kennytm 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
@bors
Copy link
Contributor

bors commented Nov 23, 2017

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

bors added a commit that referenced this pull request Nov 23, 2017
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
Copy link
Contributor

bors commented Nov 23, 2017

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

@bors bors merged commit 79bf7db into rust-lang:master Nov 23, 2017
@petrochenkov petrochenkov removed their assignment Nov 26, 2017
@cramertj cramertj deleted the in-band-lifetimes branch December 16, 2017 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants