rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.9k stars 1.56k forks source link

RFC: Drop temporaries in tail expressions before local variables #3606

Closed m-ou-se closed 4 months ago

m-ou-se commented 6 months ago

This is one of the results of the temporary lifetimes effort by @nikomatsakis @dingxiangfei2009 and me.

Originally, we were working on a much larger RFC with several changes, but decided to not block small things on big things.

This part is quite small, but requires an edition change.

Rendered

a1phyr commented 6 months ago

An example of something great with this is that it makes tracing's spans more accurate and less surprising !

lcnr commented 6 months ago

Looking at the resulting breakage from this change I would personally love to just land this change by default in all editions instead of only in >=2024, treating it as "clarifying previously underspecified language semantics" (unless we formally stated the existing rules somewhere). This kind of subtle inconsistency between editions is likely to cause some confusion.

I am also very much happy to also only have this behavior in the new edition but would like to at least mention this as an alternative.

nikomatsakis commented 6 months ago

@rfcbot fcp merge

After much exploration, we've decided to break apart the "temporary lifetimes" change into two smaller items. This one corrects the lifetimes in the tail expressions of blocks, a common source of surprising compiler errors. The other item (super let) will be pursued in a separate RFC. We have decided NOT to propose a change to temporary lifetimes in match expressions — which was predicted to affect a lot more existing code — and are exploring alternatives (e.g., starting with a lint to help detect the buggy cases).

Lang team, let's do this!

rfcbot commented 6 months ago

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

nikomatsakis commented 6 months ago

Looking at the resulting breakage from this change I would personally love to just land this change by default in all editions instead of only in >=2024, treating it as "clarifying previously underspecified language semantics" (unless we formally stated the existing rules somewhere). This kind of subtle inconsistency between editions is likely to cause some confusion.

I could be persuaded here, but I am reluctant to make changes in semantics without an edition, because it is difficult to know what code out there may be affected.

tmandry commented 5 months ago

Seems very well scoped and motivated for an edition change. Thanks for the effort that went into design and boiling it down to a simple RFC such as this.

@rfcbot reviewed

rfcbot commented 5 months ago

:bell: This is now entering its final comment period, as per the review above. :bell:

RalfJung commented 5 months ago

I would like to formally register a concerning regarding the effect on unsafe code. The RFC says

There's a very small chance this breaks existing code in a very subtle way. However, we can detect these cases and issue warnings.

However, for unsafe code implicitly relying on the current drop order, it's unclear to me whether that is possible.

(AFAIK rfcbot support for t-lang-advisors has not been implemented so if someone on t-lang could tell the bot about this concern that would be great. :)

m-ou-se commented 5 months ago

@RalfJung Your concern was discussed in the lang triage meeting just now. Niko said that we can warn for this situation (and that that such patterns with pointers are error-prone regardless, so we probably want to warn for such patterns also without this change).

The lang team consensus in the meeting was, given the expectation that this can be linted for, this doesn't have to be a blocking concern on the RFC. (But instead something to take into account before stabilization, when there is more data.)

I will add it to the unresolved questions in the RFC.

hkBst commented 5 months ago

This seems like a great change, but I was interested in seeing if any real-world code is broken by this. Where can I find the crater run with this change applied to all editions?

rfcbot commented 5 months ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

traviscross commented 4 months ago

The lang team has accepted this RFC and we've now merged it.

Thanks to @m-ou-se and @dingxiangfei2009 for pushing this forward, and thanks to all those who reviewed this RFC and provided helpful feedback.

For further updates on this work, follow the tracking issue: