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

Fix region errors uncovered by rust-lang/rust#27641 #260

Closed
wants to merge 2 commits into from

Conversation

nikomatsakis
Copy link
Contributor

The fixes for RFC rust-lang/rfcs#1214 uncovered some problems with how nickel was using lifetimes. These two patches correct the usage so that it will build after rust-lang/rust#27641 lands. I also attempted to move away from short names like 'a and 'k and instead give names to meaningful lifetimes within the processing of a request. I'm not sure that I found the best names, though.

From what I can see, the role of the lifetime parameters is as follows:

'k -- represents the internal hyper state, such as the network connection. I called this 'conn.

'a and 'b -- are both varying lifetimes that generally represent "the shortest lifetime of any borrowed data in the Request". Generally this is the lifetime of the "middleware" portion of request processing, so I called it 'mw.

To put this another way, I think that when we are processing any particular request, the stack frames are layered roughly like so, with the lifetimes 'mw and 'conn labeled:

<hyper>                +---------+ 'conn
                                 |
<nickle>               +--+ 'mw  |
                          |      |
<middleware>              |      |
                          v      v

@cburgdorf
Copy link
Member

Oh wow, did we just receive a PR from Mr. Rust himself ;) Thanks for taking the time to prepare us for the upcoming changes. I'm waiting for @Ryman to take a closer look as this stuff is a bit out of my comfort zone.

Btw, I'm super happy about the move to more expressive names for lifetime annotations.

Minor nit, we require commits to follow this commit message convention. But it's fine, we can change the commit message on our end before merging.

@Ryman
Copy link
Member

Ryman commented Aug 14, 2015

I agree with these changes, I actually needed to refactor to two lifetimes in the refactor for #246 (see [1]) but didn't backport it. It took quite a while to get to it at the time but I came to the same conclusion about what the lifetimes represent, albeit without naming them explicitly (even left a TODO for that :D ). The explanations in the commits are really helpful, thanks for that.

@nikomatsakis I guess we need to merge this in order to build on the next nightly? Or are the new rules going to be feature flagged?

@cburgdorf If the answer to the above is the former then we should probably merge, else we can see how the env branch shapes up in the next week or so to see if we get rid of the problem through that?

[1] this commit kind of has the end-result of that Ryman@ebc6aa9#diff-6cfc163228cc31bed4a7214fa0b69300R30

… from mw state

Correct lifetimes in response to errors found by rust-lang/rust#27641;
the lifetime parameters on request ('a, 'b, 'k) seem to play the
following roles:

'a, 'b -- these represent the processing of this individual request.
They can vary.

'k -- this represents the lifetime of the server's internal, mutable
storage. It is fixed.

If you only have two parameters 'x and 'y to supply, then, the correct
pattern is `'x, 'x, 'y`, because then `'x` plays the role of the
intersection of `'a` and `'b`, but `'y` is pinned to the server's
internal storage.
@nikomatsakis
Copy link
Contributor Author

@Ryman

I guess we need to merge this in order to build on the next nightly? Or are the new rules going to be feature flagged?

I did my best to ensure that the changes only receive warnings, but nickel was one of a handful projects that will actually get hard errors (because I couldn't disentangle the effects of the bug fixes from everything else).

@cburgdorf

Minor nit, we require commits to follow this commit message convention. But it's fine, we can change the commit message on our end before merging.

OK, happy to fix it up. Gimme a second.

Attempt to rename 'a and 'k to something more meaningful. Not sure
that I got this part right.
@nikomatsakis
Copy link
Contributor Author

@cburgdorf ok, hopefully that looks better :) if not, feel free to suggest something else.

@nikomatsakis
Copy link
Contributor Author

@cburgdorf

Thanks for taking the time to prepare us for the upcoming changes

ps, my pleasure. :)

Ryman added a commit that referenced this pull request Aug 14, 2015
@Ryman
Copy link
Member

Ryman commented Aug 14, 2015

Edited the commit message on the first commit to add breaking change information and merged to master.

Thanks!

@Ryman Ryman closed this Aug 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants