-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Temporary lifetime extension through tuple struct and tuple variant constructors #140593
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
base: master
Are you sure you want to change the base?
Conversation
r? @Nadrieril rustbot has assigned @Nadrieril. Use |
Does this count as 'I-lang-easy-decision'? I think it might be an easy decision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add an explicit test for
let indir = Some;
let x = indir(&temp);
53924ca
to
a9544f2
Compare
We have more unlikely things than this as experiments. I'd say do it, and let's see how it feels. |
had some arguments against that. (Mostly that you won't be able to tell at the call site, I think.)
That argument already applies to this proposal though: you can't tell if it's a tuple struct constructor or a function call. So I feel we're crossing that line already with the proposal at hand.
|
In theory, in practice convention + lints mean that it is not ambiguous: constructors are upper case, functions are not. |
I've given this some thought. I'm in favor. @rfcbot reviewed But I do want to be clear about something... @traviscross wrote
...but I had the impression this was an insta-stable change. At least, I don't see any feature-gate-dependent code in the diff (and I don't know why we would need an FCP otherwise). |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
In principle this could use an RFC. Seems a bit like overkill to me though. |
@rustbot labels +relnotes |
@nikomatsakis That wasn't about this PR. That was about a tangent: having an attribute on function parameters for temporary lifetime extension. (See the comment right before the one you quoted.) The change in this PR is insta-stable, yes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM impl-wise, just one test suggestion.
Given that this is like a stabilization, could you check if the Reference has enough detail about lifetime extension that it would need to be updated for this change? Apart from that, I think r=me with the extra test once the FCP completes. |
Yeah the reference has a section on that at https://doc.rust-lang.org/nightly/reference/destructors.html#temporary-lifetime-extension |
@rustbot labels +S-waiting-on-documentation |
Reference PR: rust-lang/reference#1813 (It's a submodule, so not part of this PR.) |
Looks good, r=me once FCP completes. |
@rustbot labels +S-waiting-on-documentation Thanks for the PR to the Reference. However, we hold off merging the stabilization until the Reference PR is reviewed and ready to merge, so we keep this label until then. |
This makes temporary lifetime extension work for tuple struct and tuple variant constructors, such as
Some()
.Before:
After:
So, with this change, this works:
Until now, we did not extend through tuple struct/variant constructors (like
Some
), because they are function calls syntactically, and we do not want to extend the String lifetime in:However, it turns out to be very easy to distinguish between regular functions and constructors at the point where we do lifetime extension.
In practice, constructors nearly always use UpperCamelCase while regular functions use lower_snake_case, so it should still be easy to for a human programmer at the call site to see whether something qualifies for lifetime extension or not.
This needs a lang fcp.
More examples of what will work after this change: