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

Document RFC 1623: static lifetime elision. #37928

Merged
merged 4 commits into from
Feb 9, 2017
Merged

Document RFC 1623: static lifetime elision. #37928

merged 4 commits into from
Feb 9, 2017

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Nov 22, 2016

This should be the last item required for stabilizing RFC 1623 (#35897).

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

address will have the `static` lifetime. The compiler is, however, still at
liberty to translate the constant many times, so the address referred to may not
be stable.
address will have the `static` lifetime. (See below on [static lifetime elision](#static-lifetime-elision).) The compiler is, however, still at liberty to
Copy link
Member

Choose a reason for hiding this comment

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

Can you wrap this line to match the indentation of the surrounding lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agh, yes. I've updated it; I meant to fix that before I pushed it!

@steveklabnik
Copy link
Member

So, @rust-lang/docs , this raises a question. How do we make the distinction between stable and unstable stuff here?

@chriskrycho
Copy link
Contributor Author

chriskrycho commented Nov 22, 2016 via email

@GuillaumeGomez
Copy link
Member

It should have an unstable version. I don't have much better idea. :-/

@chriskrycho
Copy link
Contributor Author

I just noted that all the code samples in reference.md get tested. Neat! I'll update the sample accordingly in the morning.

@frewsxcv
Copy link
Member

frewsxcv commented Nov 23, 2016

I don't have any great ideas either, other than explicitly mentioning that it's unstable in the paragraph above and adding in the #[feature(whatever_feature_name)] in the code example. At least we'll get a warning when it stabilizes.

@steveklabnik
Copy link
Member

Putting this on this week's doc team agenda.

@llogiq
Copy link
Contributor

llogiq commented Dec 14, 2016

We could change the feature implementation to my first commit, thus stabilizing the feature and removing the extra code adding the feature gate that makes it unstable in the first place. I presume this should make the doc tests pass.

@steveklabnik
Copy link
Member

So, we talked about this at the docs meeting today.

What it really boils down to is this: it'd be a shame to block this based on figuring out the broader story here. However, once we know that the rest of the feature will be stable, we can just land this, then land the stabilization of the feature. @aturon just put the feature into FCP, so let's see how that goes before we merge.

And in the meantime, let's figure out the broader docs story. I'm going to open an issue on the RFCs repo about this.

@chriskrycho
Copy link
Contributor Author

Related: I'll have time Friday or Saturday to update the not-the-feature-gate bits of the test that are broken here—there's one piece that needs to be tweaked independent of the stabilization/feature flag issue.

@chriskrycho
Copy link
Contributor Author

Kept spacing this; came back to it via other issues. I'll update the non-feature-gate related parts tonight or tomorrow!

@llogiq
Copy link
Contributor

llogiq commented Jan 3, 2017

@chriskrycho do you need any help?

@chriskrycho
Copy link
Contributor Author

@llogiq gah. Slipped my mind. The only thing that needs tweaking is setting ignore on this example. It's re-using items from earlier in the reference, but they're not in scope for that specific example.

@chriskrycho
Copy link
Contributor Author

I just pushed a fixed version of the commit which just includes all the required pieces. We'll see what Travis says…

@chriskrycho
Copy link
Contributor Author

UGH. I pushed the rebase, but not the fix.

This is how my whole first day back at work has been. 🤦‍♂️

@steveklabnik
Copy link
Member

@chriskrycho
Copy link
Contributor Author

chriskrycho commented Jan 23, 2017 via email

@steveklabnik
Copy link
Member

No worries!

@est31 est31 mentioned this pull request Jan 23, 2017
2 tasks
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Actually this is full elision with a 'static default. This means that if the types contains fns, the usual elision rules will apply, only choosing 'static if no elision rule applies.

@chriskrycho
Copy link
Contributor Author

@llogiq can you elaborate?

@steveklabnik I seem to have failed in pacifying Travis and the feature gate. Can you take a gander and indicate what I did wrong?

@llogiq
Copy link
Contributor

llogiq commented Jan 31, 2017

This means if you have a static FOO : &Fn(&Bar) -> &Baz, you actually have a static FOO : &'static Fn<'a>(&'a Bar) -> &'a Baz for some unnamed 'a.

@est31
Copy link
Member

est31 commented Jan 31, 2017

I seem to have failed in pacifying Travis and the feature gate.

I think you can fix it by adding # #![feature(...)] to the doc test. It won't show up in the output then due to the leading #, while fixing the issue. See also the book and the asm documentation, it uses this as well.

@chriskrycho
Copy link
Contributor Author

@est31 ah, it's the extra #. Got it.

@llogiq do you have a suggested change to the write-up here?

without the lifetimes. Returning to our previous example:

```rust
#[feature(static_in_const)]
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be #![feature(static_in_const)]; you forgot the exclamation point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YOU ARE MY HERO

@llogiq
Copy link
Contributor

llogiq commented Jan 31, 2017

I think I could improve it, but I need to find the time first.

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Also after the example, add a paragraph to explain the elision:

In case the static or constant items contain function references or closures, and those work with references, the compiler will try the usual elision rules (link to elision). Failing that, it will default the lifetimes to 'static. This is best explained by an example:

const FUN : fn(&str) -> &str = ..
// per rule #1, this is fn<'a>(&'a str) -> &'a str

const FUNNY : Fn(&Foo, &Bar, &Baz) -> usize = ..
// per rule #2, this is Fn<'a, 'b, 'c>(&'a Foo, &'b Bar, &'c Baz) -> usize

const UNFUNNY : Fn(&Foo, &Bar) -> &Baz = ..
// neither rule applies, so all `&`s are `'static`

@@ -1271,15 +1271,16 @@ guaranteed to refer to the same memory address.

Constant values must not have destructors, and otherwise permit most forms of
data. Constants may refer to the address of other constants, in which case the
address will have the `static` lifetime. The compiler is, however, still at
address will have the `static` lifetime. (See below on [static lifetime
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "will have elided lifetimes where applicable, otherwise – in most cases – defaulting to the 'static lifetime.

@est31
Copy link
Member

est31 commented Feb 5, 2017

Now that beta branched, I would like to ask for beta backport nomination, so that already 1.16 would have it stabilized.

@llogiq
Copy link
Contributor

llogiq commented Feb 5, 2017

Good idea.

@steveklabnik
Copy link
Member

@chriskrycho ping! any interest in keeping this going? this is still blocking stabilization.

@est31 I would be strongly against this; stabilization backports are incredibly rare.

@chriskrycho
Copy link
Contributor Author

@steveklabnik yeah, should be able to allocate an hour to it tomorrow. I'll pull in @llogiq's comments, as well as fix the build issue, then.

@llogiq
Copy link
Contributor

llogiq commented Feb 8, 2017

We may also want to show embedded lifetimes, e.g. Fn(&'a Foo) -> Bar<'a> can be shortened to Fn(&Foo) -> Bar.

@chriskrycho
Copy link
Contributor Author

@llogiq regarding your last comment: did that change with this RFC/implementation? If not, I'd prefer to note it as something to add in a separate commit/PR. I think we actually need a dedicated section for lifetime elision in general; right now it appears to be documented in detail only in the nomicon.

@chriskrycho
Copy link
Contributor Author

Updated with everything other than the embedded lifetimes scenario, and it passed rustup run nightly rustdoc --test ./doc/reference.md so we should finally be good to go. 🤞

@steveklabnik
Copy link
Member

Travis passed!

@bors: r+ rollup

thanks a ton @chriskrycho

@bors
Copy link
Contributor

bors commented Feb 8, 2017

📌 Commit 4096dd6 has been approved by steveklabnik

@chriskrycho
Copy link
Contributor Author

YESSSSSSSSSSS. FINALLLYYYYYYYY.

Sorry for the many delays on this, everyone! Glad it's finally in. So it'll land in, what, 1.17?

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 9, 2017
…eveklabnik

Document RFC 1623: static lifetime elision.

This should be the last item required for stabilizing RFC 1623 (rust-lang#35897).
@est31
Copy link
Member

est31 commented Feb 9, 2017

@chriskrycho

So it'll land in, what, 1.17?

yes.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 9, 2017
…eveklabnik

Document RFC 1623: static lifetime elision.

This should be the last item required for stabilizing RFC 1623 (rust-lang#35897).
bors added a commit that referenced this pull request Feb 9, 2017
Rollup of 9 pull requests

- Successful merges: #37928, #38699, #39589, #39598, #39599, #39641, #39649, #39653, #39671
- Failed merges:
@bors bors merged commit 4096dd6 into rust-lang:master Feb 9, 2017
bors added a commit that referenced this pull request Feb 9, 2017
Stabilize static lifetime in statics

Stabilize the "static_in_const" feature. Blockers before this PR can be merged:

* [x] The [FCP with inclination to stabilize](#35897 (comment)) needs to be over. FCP lasts roughly three weeks, so will be over at Jan 25, aka this thursday.
* [x] Documentation needs to be added (#37928)

Closes #35897.
@brson brson added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 15, 2017
@alexcrichton
Copy link
Member

Removing beta nomination as sentiment seems to be to not backport (also see #35897)

@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 21, 2017
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.