Temporary lifetime extension for blocks

[dianne]

This implements a revised version of the temporary lifetime extension semantics I suggested in #145838 (comment), with the goal of making temporary lifetimes and drop order more consistent between extending and non-extending blocks. As a consequence, this undoes the breaking change introduced by #145838 (but in exchange has a much larger surface area).

The change this PR hopes to enforce is a general rule: any expression's temporaries should have the same relative drop order regardless of whether the expression is in an extending context or not: let _ = $expr; and drop($expr); should have the same drop order. To achieve that, this PR applies lifetime extension rules to blocks:

// This `temp()` is now extended past the block tail in all contexts.
{ &temp() }

now extends the lifetime of temp() to outlive the block tail in Rust 2024 regardless of whether the block is an extending expression in a let statement initializer (in which context it was already extended to outlive the block before this PR). The scoping rules for tails of extending blocks remain the same: extending subexpressions' temporary scopes are extended based on the source of the lifetime extension (e.g. to match the scope of a parent let statement's bindings). For blocks not extended by any other source, extending borrows in the tail expression now share a temporary scope with the result of the block. This can in turn extend nested blocks within blocks' tail expressions:

// This `temp()` is extended past the outer block tail.
// It is now dropped after the reference to it at the `;`.
f({{ &temp() }});

// This context-sensitivity is consistent with `let`:
// This `temp()` was already extended.
// It is still dropped after `x` at the end of its scope.
let x = {{ &temp() }};

Since this uses the same rules as let, it only applies to extending sub-expressions.

// This `temp()` is still never extended in any context.
// In Rust 2024, it is dropped at the end of the block tail.
{ identity(&temp()) }

This also applies to if expressions' blocks since lifetime extension applies to if blocks' tail expressions, meaning it affects all editions. This is where breakage from #145838 was observed:

if cond { &temp() } else { &temp() }

now extends temp() to have the same temporary scope as the result of the if expression.

As a further consequence, this makes super let in if expressions' blocks more consistent with block expressions:

if cond() {
    super let x = temp();
    &temp
} else {
    super let x = temp();
    &temp
}

previously only worked in extending contexts (since the super lets would be extended), and now it works everywhere.

Reference PR: rust-lang/reference#2051

@rustbot label +T-lang

[dianne]

@rustbot label -stable-nominated

I'm not intending to stable-nominate this, at least. Someone else can, but I don't expect it's needed or that it would be accepted.

[traviscross]

Does this only affect code in Rust 2024, or would you expect any visible difference in earlier editions?

[dianne]

It should only be visible in Rust 2024. The only extending expressions that introduce temporary drop scopes are Rust 2024 block tail expressions. Edit: this is also visible on earlier editions through if expressions' blocks.

Suppose we have a macro extending!, for which $expr is extending if extending!($expr) is extending. Under this PR, in a non-extending context, { extending!(&temp()) } would give temp() the same temporary scope as the result of the block. Prior to Rust 2021, they're already in the same scope, due to extending! being unable to introduce temporary scopes.

Or to generalize this, the aim of this PR is that in a non-extending context, extending!(&temp()) should give temp() the same temporary scope as the expansion, similar to how let x = extending!(&temp()); gives temp() the same scope as x. This already holds in Rust 2021 and prior.

If new expressions are added to Rust that are both extending and temporary scopes, I'd want this behavior to apply to them as well.

[traviscross]

Since this would effectively reduce the scope of the Rust 2024 tail expression temporary scope change, we'd also want to be sure to reflect that in the behavior of the tail-expr-drop-order lint.

[dianne]

I haven't done extensive testing, but see this test diff for that lint: lint-tail-expr-drop-order-borrowck.rs. I'm applying the lifetime extension rules on all editions, and lifetime extension prevents the temporary scope from being registered as potentially forwards-incompatible (even though the extended scopes are technically the same as the old scopes in old editions). Though I think I've convinced myself at this point that lifetime extension doesn't need to be applied to block tails of non-extending old-edition blocks¹, so potentially the lint change could be implemented in some other way instead.

Footnotes

1. I was worried about mixed-edition code, but I don't think it's an issue anymore. ↩

[bors]

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

[dianne]

I've made some revisions. This should now properly handle if expressions' blocks, meaning it affects all editions (since if blocks are both terminating in all editions and extending when the if expression is extending). Of note, I didn't notice at the time, but I think #145838 affected all editions as well (including the real-world breakage), due to if blocks working like that.

I think the implementation will likely need optimization and cleanup, but it might take a bit of refactoring to get it to a good place, so I'd like to get a vibe check on the design first, if there's room for it in a lang team meeting.

@rustbot label +I-lang-nominated

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[traviscross]

Thanks for the analysis. As the bot is reminding us, if anyone is interested to do it to make later crater runs cleaner, it'd be good if possible to make a PR to crater adding all the crates with flaky tests or spurious failures that we've identified in these runs (by analysis or by observing they failed and then passed in a later run) to the list.

[dianne]

The purse test failure looks like an incorrect implementation of Purse::most_common_type. It should be finding the key in a HashMap mapping to the highest value, but instead it's taking the highest key, which uses the Ord impl of TypeId (and if it was looking at values, it would rely on HashMap iteration order). Seems like an unrelated nondeterministic failure.

[traviscross]

The crater run results seem to be nearly the best we could expect, so, I propose, let's do this.

@rfcbot fcp merge

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

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.

[joshtriplett]

@rfcbot reviewed

Could someone report that non-determinism issue to purse, mentioning that it causes spurious test failures and we're observing them in crater?

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

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

[dianne]

Could someone report that non-determinism issue to purse, mentioning that it causes spurious test failures and we're observing them in crater?

Filed here: rauljordan/purse#2. Is there a standard procedure for determining when to file issues and when to add to crater's ignore list? There's not many others that depend on TypeId ordering, but there's a lot of tests that depend on HashMap iteration order, random number generation, timing, execution order of concurrent threads (often of the tests themselves), and similar sources of nondeterminism.

[tmandry]

I think adding it to the ignore list is fine. Filing an issue upstream is nice, but it's more work, and by itself it leaves us open to more flakes until the crate is fixed.

[traviscross]

@rustbot labels +S-waiting-on-documentation

@dianne, we'll need to put together the Reference changes for this. If you can put up a PR, you and I can iterate on that.

[dianne]

I'll try and get to that soon! If it's not too disruptive, I'm considering reworking the definition of lifetime extension to be from the bottom-up instead of top-down, based on specifying which temporaries would be extended in an extending context; e.g. the operand of a borrow operator has an extendable temporary scope and a normal/unsafe block would count extendable temporaries in its tail expression as extendable for the block as a whole. More generally, "extending subexpressions" could be defined such that the extendable temporaries of an extending subexpression are extendable for its parent as well; e.g. the tail expression of a normal/unsafe block would be an extending subexpression.

The advantages I see are:

• This wouldn't need a separate base case for non-extending blocks and if expressions, which I find a bit messy in the top-down version. Instead, we could have a more general rule (though I'm not happy with the exact framing yet): the extendable temporaries of an extending subexpression are extended to (at least) the temporary scope enclosing its parent. This currently only does anything for blocks and if, but I think the more general framing both more intuitively captures and helps future-proof the behavior I'm trying to get from this PR. Special handling for blocks and if feels arbitrary without justification and would need updating if other extending expressions with their own temporary scopes are added.
• I think it may handle the more special endpoints of lifetime extension (let initializers, static item bodies, and const items/blocks) more cleanly. In particular, in the top-down approach it's tricky to disambiguate lifetime extension for const blocks¹. In the bottom-up approach, I find it a bit easier: a const block extends extendable temporaries in its tail expression to have static lifetimes (and importantly, they don't count as extendable for the const block expression as a whole).

And the disadvantages I see are:

• This would make the compiler implementation and spec differ probably, or the compiler implementation would also need some rewriting for it to match². There's plenty of places where the spec and compiler are less in-sync, but for something as subtle as lifetime extension it's nice to have a direct correspondence.
• It's a substantial change, and bigger than strictly necessary. My hope is that links to the Reference will still work and make sense in context, but it means additional risk there, especially if it needs new terminology.

Any initial opinions on this, @traviscross? I imagine it'll be clearer once I have a draft. There might also be another way to get the advantages I'd want from a rewrite without the churn.

Footnotes

1. The tail expression of a const block is always extending; temporaries it extends are given static lifetimes. This is slightly awkward in the top-down spec: since const blocks are expressions, they can themselves be extending due being in a let statement initializer; there needs to be special clarification that their tail expressions aren't extending with regard to the let statement. See Further specify temporary scoping for statics and consts reference#2041 for my first attempt at dealing with that. ↩

2. I'm not actually sure which implementation would look nicer. The top-down approach needs base cases for non-extending blocks and if expressions, but the bottom-up approach has messier recursive cases. I kept it top-down in the PR as currently written since it seemed least disruptive. ↩

[dianne]

I've opened a reference PR with a first draft at rust-lang/reference#2051 and rebased to make testing it against this PR a bit easier.

[traviscross]

Thanks @dianne.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dianne]

Rebased to account for #147471. I also tweaked a couple comments to hopefully make them clearer.