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.

I don't think this is ready to merge yet. It should have a Reference PR, it will need a lang FCP, and it may need other things as well.

@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

[dianne]

Is there a way to tell where instruction count regressions are from? Looking at the wall time breakdown from the detailed report pages though, it looks like this doesn't make region_scope_tree significantly slower. e.g. the regression in cargo-0.87.1 (full) seems to be from spending more time in codegen (including a couple more executions of is_codegened_item) and the extra query executions in cranelift-codegen-0.119.0 (incr-patched: println) also have nothing directly to do with region_scope_tree as far as I can tell. If this PR is affecting their performance, it could be through assigning different scopes to temporaries in already-compiling code; it seems it can shuffle StorageDeads around at least¹. If that's the cause of any of those regressions, it would be a consequence of this change's design (and its interactions with certain implementation details elsewhere in the compiler¹) rather than in inefficiencies in my unoptimized scope resolution implementation.

Footnotes

1. This came up in a single mir-opt test (diff here). One temporary involved in a slice indexing operation was extended slightly so that its StorageDead swapped with another temporary's. Notably, the slice indexing expression being borrowed from there is a place expression; as I understand at least, its temporaries are implementation details and shouldn't need to live for the temporary scope of the expression (and indeed, the borrowed element outlives the temporary scope of the indexing expression without issue). But it seemingly does use the expression's temporary scope, as it was extended. ↩ ↩²

[Kobzol]

Cachegrind can be used to show function-level profiling results using https://github.com/rust-lang/rustc-perf:

cargo run --release --bin collector \
    profile_local cachegrind \
    +caccb4d0368bd918ef6668af8e13834d07040417 \
    --rustc2 +2440211fe03bc45c89b6dc1a3df18382ce91e32b \
    --exact-match cargo-0.87.1 \
    --profiles Opt \
    --scenarios Full

[dianne]

Thanks! Looking at the diff for the cargo (full) test, it indeed looks like the instruction count increase for that test is in llvm; the diff was dominated by changes in llvm instruction counts (mostly increases). I couldn't find any functions in rustc_hir_analysis::check::region in there (or things I'd expect extra calls of). The biggest instruction count change in the frontend I can see in the report is +165,064,649 attributed to ThirBuildCx::make_mirror_unadjusted::{closure}.. paired with -168,609,045 across ThirBuildCx::mirror_expr and ThirBuildCx::mirror_expr_inner; in sum, a small instruction count change for THIR building (less significant than anything else that made it into the report). It also looks like some more instructions were spent allocating space for vectors and hash maps in some places and fewer were spent on that in others. The only significant change seems to be in codegen.

If the llvm perf regression in that test and the incremental perf regression in cranelift-codegen are due to implementation-detail temporaries inheriting expressions' temporary scopes and having their StorageDeads later than necessary (which could definitely affect analyses), I wonder if there's room for some small perf gains by re-scoping those temporaries to have shorter lifetimes. I don't think that would belong in this PR, though.

[Kobzol]

This seems like a relatively involved change that indeed could have effect on (LLVM) codegen. I think that in light of that the small regressions are justifiable, especially if you also have some ideas about how it could be improved in the future.

[dianne]

I've opened a PR to fix the imprecise array/slice index temporary scopes at #147083. There may be other instances of imprecise temporary scoping in MIR building that this PR would interact with, but that's at least a start.

[BoxyUwU]

@dianne I can probably review this PR At Some Point:tm: but it'll take a bit before I have the time to properly dig into the surrounding context of lifetime extension stuff, and I imagine it'll also take a while to get up to speed on that :thinking: If there's someone already up to speed on this that could review this PR that might be better but otherwise this is fine :+1:

[dianne]

Unfortunately, I'm not sure who all on review rotation would have lifetime extension fresh in mind. Maybe @jackh726 since you reviewed my last temporary scoping PR recently?
Looking at the compiler team as a whole, cc @m-ou-se @nikomatsakis

Otherwise, thanks in advance! ^^

[craterbot]

🚧 Experiment pr-146098 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-146098 is completed!
📊 217 regressed and 225 fixed (705065 total)
📊 73009 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-146098/retry-regressed-list.txt ↩

[traviscross]

@craterbot run p=1 mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-146098/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-146098-1 created and queued.
🤖 Automatically detected try build 2440211
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-146098-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[dianne]

The WLambda regression (https://crater-reports.s3.amazonaws.com/pr-146098/try%23028592fec99e54cc92def5a2a849c673b066dd93/gh/WeirdConstructor.WBlockDSP/log.txt) looks real. It uses a raw pointer cast, &*l.borrow() as *const VVal as i64 which isn't intended to extend l.borrow() but does under this PR. Here's the context: https://github.com/WeirdConstructor/WLambda/blob/87efe7b177c2709b0568fac451cecb04245c51b5/src/vval.rs#L4533-L4539. If this was in the initializer of a let statement rather than returned from a function, it would be extended without the PR, so I personally think it's worth being consistent in behavior. There is an easy fix, at least: (*l).as_ptr() should be preferred in these cases.

If necessary, I think it'd be possible to lint on raw borrows (and raw pointer casts of borrow expressions) that would be extended by this PR and (when possible) suggest non-extending alternatives (such as .as_ptr()).

[traviscross]

It's probably worth making the PR to WLambda in any case. Whatever we do, their code would certainly be made clearer and better by not relying on this. Once they merge it, it'd be then maybe worth notifying any dependents that need to update.

Let us know what you think after looking through the test failures (I know, those are hard to go through).

If we can put out an FCW here, that's always going to be an easier decision for us to make than taking the breakage directly.

[craterbot]

🎉 Experiment pr-146098-1 is completed!
📊 90 regressed and 70 fixed (73180 total)
📊 453 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-146098-1/retry-regressed-list.txt ↩

[dianne]

WLambda still seems to be the only build failure. This time it appeared in the root results as well.

For the tests, 87's still quite a few; a rerun might be worthwhile? In the mean time, I'll start looking over them in batches; it'll be good to identify flaky tests regardless (but it'd take a few days to get through all 87).

The first 16 test failures (all spurious regressions)

• MechanicalOrange5.definitely-only-once.d7144ac4e1d91039356e9e41a500eff3978ad47f (log): unrelated UB; no temporary lifetime extension is present.
• Pjt727.arc-6502.c51f03a4238d8334cb53cd1e604135e7d4e11ec3 (log): timing-sensitive (polling after thread::sleep)
• ShivanshuKantPrasad.Automata.95af40f4c4315535dfc8c7d7c5fef846ff491d1d (log): relies on HashMap iteration order
• ZhiyaoMa98.hadusos-protocol.f08335a50dfb5b37cd4fdce8a7263e2d2af21fff (log): timing-sensitive (requires completion within a timeout)
• ggutoski.k256_test.cf3feb239b7c4c2a6e40487c320de94fe679d38f (log): the repository description, accurately, is "A test reproducing a nondeterministic assertion failure"
• hsqStephenZhang.git-rs.f0040f78c24ff93e60cefe6b989614af8e7761d4 (log): the errors are from failing to read from paths that exist relative to the repository root. I found two tests that change the current directory, so my guess would be nondeterministic failure from running the tests concurrently
• lambdaclass.spawned.d68258dba5ed6f90c5508af64b2fa694be04c578 (log): timing-sensitive (comparison after sleep)
• lavrxxx.leetcode.0a357004fee62c49837e3228670e85d50c3b80ba (log): relies on HashMap iteration order
• m16d.reaction.da9287c16c65e3bd7afdb3c2cae1abe38e6ec450 (log): timing-sensitive (comparison after sleep)
• mikeleppane.Crust-of-Rust.b4e5a00ce068794a2fef0b86a454ab48e44f4764 (log): nondeterministic (race condition)
• nnethercott.simsimd_vs_manual.2a8e3e64d999a2e681ed9849660a917f7cc84c19 (log): nondeterministic (floating point arithmetic with random numbers; insufficient margin of error)
• octofhir.fhirpath-rs.73b2a77360ebbb658f9c0f9c74da6cd5f1970321 (log): timing-sensitive (timeouts)
• ozankasikci.command-vault.75d786a5792f994358edd7f50ca01b3c99f25160 (log): tests conflict when run concurrently (tests set and unset current directory)
• radhesh1.radb.c29bccaf73c83115c27edd936234752dbbf0b510 (log): nondeterministic (multithreading)
• rotaki.kll.d9b3f04fc1ce26cf92a3a5506b4a84fbf21418fc (log): nondeterministic (floating point arithmetic with random numbers; insufficient margin of error)
• trace-andreason.tp-rust.db10398d2d8de6f92c388e7b6131cf095414a3c4 (log): I'd have to look deeper to see exactly what's going wrong here, but it looks unrelated; the tests are nondeterministic both for timing reasons and concurrently-run-tests-changing-the-current-directory reasons (and maybe other reasons too)

[traviscross]

@craterbot run p=5 mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-146098-1/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-146098-2 created and queued.
🤖 Automatically detected try build 2440211
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[dianne]

PR opened to WLambda: WeirdConstructor/WLambda#17

[craterbot]

🚧 Experiment pr-146098-2 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more