-
Notifications
You must be signed in to change notification settings - Fork 13.8k
don't apply temporary lifetime extension rules to non-extended super let
#145838
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
Conversation
This comment has been minimized.
This comment has been minimized.
0542d4f
to
a35548f
Compare
Copied from #145784 (comment), since I think this is a notable caveat of this PR and worth considering before approving it: This comes with a bit of a gotcha in terms of temporary lifetimes: it might be strange that the non_extending({ let x = { &temp() }; f(x) }); // ok than in non_extending({ super let x = { &temp() }; f(x) }); // error Though the case for |
Also copied since it motivates this PR: I think something like this may be necessary for the identity &EXPR === { super let x = &EXPR; x } to hold in both extending and non-extending contexts without changing how block tail expression scopes work. Substituting, e.g. &{ &temp() } drops { super let x = &{ &temp() }; x } would extend it to outlive |
To confirm, with this PR, does this behavior hold (in Rust 2024)?: fn f<T>(_: LogDrop<'_>, x: T) -> T { x }
// These two should be the same.
assert_drop_order(1..=3, |e| {
let _v = f(e.log(2), &{ &raw const *&e.log(1) });
drop(e.log(3));
});
assert_drop_order(1..=3, |e| {
let _v = f(e.log(2), {
super let v = &{ &raw const *&e.log(1) };
v
});
drop(e.log(3));
});
// These two should be the same.
assert_drop_order(1..=3, |e| {
let _v = f(e.log(1), &&raw const *&e.log(2));
drop(e.log(3));
});
assert_drop_order(1..=3, |e| {
let _v = f(e.log(1), {
super let v = &&raw const *&e.log(2);
v
});
drop(e.log(3));
});
// These two should be the same.
assert_drop_order(1..=2, |e| {
let _v = &{ &raw const *&e.log(2) };
drop(e.log(1));
});
assert_drop_order(1..=2, |e| {
let _v = {
super let v = &{ &raw const *&e.log(2) };
v
};
drop(e.log(1));
}); (If any of these are missing, please add them as tests.) |
Does this PR affect any edition 2021 code? |
a35548f
to
f0c43cf
Compare
Those all hold under this PR, yes. I've added them all as tests (with some additional versioning to account for the Edition-dependent drop order in the first one); thanks!
Not that I'm aware of. That detail matters in Edition 2024, since the nested |
This comment has been minimized.
This comment has been minimized.
f0c43cf
to
387cfa5
Compare
// We have extending borrow expressions within the initializer | ||
// expression. |
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.
// We have extending borrow expressions within the initializer | |
// expression. | |
// We have extending borrow expressions within a non-extending | |
// expression within the initializer expression. |
(Revising my earlier text here.)
Team member @traviscross 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. |
cc @m-ou-se @rust-lang/libs-api |
387cfa5
to
23caea2
Compare
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.
The change to making the match itself return a bool is nicer. Tests seem pretty well written.
r=me for this, whether you want to land this independently or alongside #145882. |
Thanks for the review. Let's go ahead and get this in so we can start with the beta nomination. @bors r=jackh726,traviscross rollup |
…jackh726,traviscross don't apply temporary lifetime extension rules to non-extended `super let` Reference PR: rust-lang/reference#1980 This changes the semantics for `super let` (and macros implemented in terms of it, such as `pin!`, `format_args!`, `write!`, and `println!`) as suggested by `@theemathas` in rust-lang#145784 (comment), making `super let` initializers only count as [extending expressions](https://doc.rust-lang.org/nightly/reference/destructors.html#extending-based-on-expressions) when the `super let` itself is within an extending block. Since `super let` initializers aren't temporary drop scopes, their temporaries outside of inner temporary scopes are effectively always extended, even when not in extending positions; this only affects two cases as far as I can tell: - Block tail expressions in Rust 2024. This PR makes `f(pin!({ &temp() }))` drop `temp()` at the end of the block in Rust 2024, whereas previously it would live until after the call to `f` because syntactically the `temp()` was in an extending position as a result of `super let` in `pin!`'s expansion. - `super let` nested within a non-extended `super let` is no longer extended. i.e. a normal `let` is required to treat `super let`s as extending (in which case nested `super let`s will also be extending). Closes rust-lang#145784 This is a breaking change. Both static and dynamic semantics are affected. The most likely breakage is for programs to stop compiling, but it's technically possible for drop order to silently change as well (as in rust-lang#145784). Since this affects stable macros, it probably would need a crater run. Nominating for discussion alongside rust-lang#145784: `@rustbot` label +I-lang-nominated +I-libs-api-nominated Tracking issue for `super let`: rust-lang#139076
Rollup of 9 pull requests Successful merges: - #144871 (Stabilize `btree_entry_insert` feature) - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables) - #145838 (don't apply temporary lifetime extension rules to non-extended `super let`) - #146259 (Suggest removing Box::new instead of unboxing it) - #146410 (Iterator repeat: no infinite loop for `last` and `count`) - #146460 (Add tidy readme) - #146581 (Detect attempt to use var-args in closure) - #146588 (tests/run-make: Update list of statically linked musl targets) - #146647 (Move `#[rustc_coherence_is_core]` to the `crate_level` file) r? `@ghost` `@rustbot` modify labels: rollup
…jackh726,traviscross don't apply temporary lifetime extension rules to non-extended `super let` Reference PR: rust-lang/reference#1980 This changes the semantics for `super let` (and macros implemented in terms of it, such as `pin!`, `format_args!`, `write!`, and `println!`) as suggested by ``@theemathas`` in rust-lang#145784 (comment), making `super let` initializers only count as [extending expressions](https://doc.rust-lang.org/nightly/reference/destructors.html#extending-based-on-expressions) when the `super let` itself is within an extending block. Since `super let` initializers aren't temporary drop scopes, their temporaries outside of inner temporary scopes are effectively always extended, even when not in extending positions; this only affects two cases as far as I can tell: - Block tail expressions in Rust 2024. This PR makes `f(pin!({ &temp() }))` drop `temp()` at the end of the block in Rust 2024, whereas previously it would live until after the call to `f` because syntactically the `temp()` was in an extending position as a result of `super let` in `pin!`'s expansion. - `super let` nested within a non-extended `super let` is no longer extended. i.e. a normal `let` is required to treat `super let`s as extending (in which case nested `super let`s will also be extending). Closes rust-lang#145784 This is a breaking change. Both static and dynamic semantics are affected. The most likely breakage is for programs to stop compiling, but it's technically possible for drop order to silently change as well (as in rust-lang#145784). Since this affects stable macros, it probably would need a crater run. Nominating for discussion alongside rust-lang#145784: ``@rustbot`` label +I-lang-nominated +I-libs-api-nominated Tracking issue for `super let`: rust-lang#139076
…jackh726,traviscross don't apply temporary lifetime extension rules to non-extended `super let` Reference PR: rust-lang/reference#1980 This changes the semantics for `super let` (and macros implemented in terms of it, such as `pin!`, `format_args!`, `write!`, and `println!`) as suggested by ```@theemathas``` in rust-lang#145784 (comment), making `super let` initializers only count as [extending expressions](https://doc.rust-lang.org/nightly/reference/destructors.html#extending-based-on-expressions) when the `super let` itself is within an extending block. Since `super let` initializers aren't temporary drop scopes, their temporaries outside of inner temporary scopes are effectively always extended, even when not in extending positions; this only affects two cases as far as I can tell: - Block tail expressions in Rust 2024. This PR makes `f(pin!({ &temp() }))` drop `temp()` at the end of the block in Rust 2024, whereas previously it would live until after the call to `f` because syntactically the `temp()` was in an extending position as a result of `super let` in `pin!`'s expansion. - `super let` nested within a non-extended `super let` is no longer extended. i.e. a normal `let` is required to treat `super let`s as extending (in which case nested `super let`s will also be extending). Closes rust-lang#145784 This is a breaking change. Both static and dynamic semantics are affected. The most likely breakage is for programs to stop compiling, but it's technically possible for drop order to silently change as well (as in rust-lang#145784). Since this affects stable macros, it probably would need a crater run. Nominating for discussion alongside rust-lang#145784: ```@rustbot``` label +I-lang-nominated +I-libs-api-nominated Tracking issue for `super let`: rust-lang#139076
Rollup of 14 pull requests Successful merges: - #142807 (libtest: expose --fail-fast as an unstable command-line option) - #144871 (Stabilize `btree_entry_insert` feature) - #145071 (Update the minimum external LLVM to 20) - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables) - #145660 (initial implementation of the darwin_objc unstable feature) - #145838 (don't apply temporary lifetime extension rules to non-extended `super let`) - #146259 (Suggest removing Box::new instead of unboxing it) - #146410 (Iterator repeat: no infinite loop for `last` and `count`) - #146460 (Add tidy readme) - #146552 (StateTransform: Do not renumber resume local.) - #146564 (Remove Rvalue::Len again.) - #146581 (Detect attempt to use var-args in closure) - #146588 (tests/run-make: Update list of statically linked musl targets) - #146631 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3)) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145838 - dianne:non-extending-super-let, r=jackh726,traviscross don't apply temporary lifetime extension rules to non-extended `super let` Reference PR: rust-lang/reference#1980 This changes the semantics for `super let` (and macros implemented in terms of it, such as `pin!`, `format_args!`, `write!`, and `println!`) as suggested by ````@theemathas```` in #145784 (comment), making `super let` initializers only count as [extending expressions](https://doc.rust-lang.org/nightly/reference/destructors.html#extending-based-on-expressions) when the `super let` itself is within an extending block. Since `super let` initializers aren't temporary drop scopes, their temporaries outside of inner temporary scopes are effectively always extended, even when not in extending positions; this only affects two cases as far as I can tell: - Block tail expressions in Rust 2024. This PR makes `f(pin!({ &temp() }))` drop `temp()` at the end of the block in Rust 2024, whereas previously it would live until after the call to `f` because syntactically the `temp()` was in an extending position as a result of `super let` in `pin!`'s expansion. - `super let` nested within a non-extended `super let` is no longer extended. i.e. a normal `let` is required to treat `super let`s as extending (in which case nested `super let`s will also be extending). Closes #145784 This is a breaking change. Both static and dynamic semantics are affected. The most likely breakage is for programs to stop compiling, but it's technically possible for drop order to silently change as well (as in #145784). Since this affects stable macros, it probably would need a crater run. Nominating for discussion alongside #145784: ````@rustbot```` label +I-lang-nominated +I-libs-api-nominated Tracking issue for `super let`: #139076
This is a breaking change! Please don't rush it to be insta-stable without some migration period or a mitigation (like future-compat warnings, or some opt-in, or at least keep it nighty-only for a longer period). Formatting strings don't support conditions, so use of a nested I agree with the overall change, since the lifetime extension was already inconsistent, but I'm worried this may suddenly break legitimate code, and Rust isn't supposed to be doing that. |
On lang, we are and were aware this is a breaking change (of a behavior that we released in Rust 1.88 for That's also why we'd prefer to get this change released sooner rather than later. The longer that the other behavior is stable, the more people will fall into writing the thing we're breaking. What went out in Rust 1.88 and Rust 1.89 amounted to an accidental stabilization. When we do those, we try to take it back, if we can. Even if we might later want to support something like this in the future, such as via @dianne's proposal in #146098, our practice has been to first fix the bug when possible. We then do the design work on the feature and later land it intentionally. When deciding whether we can take back an accidental stabilization directly, we consider the degree and character of the breakage, among other factors. Here, we judged that as acceptable given the benefit and given that we're taking this back relatively quickly after releasing this bug.
Note in particular that the pattern we're breaking here, println!("{:?}{:?}", (), if true { &format!("") } else { "" }); did not work until Rust 1.89. That code is an error in Rust 1.88 and all prior versions. Further, due to #145880, println!("{:?}", if true { &format!("") } else { "" }); //~ ERROR is still an error in Rust 1.89. |
Rollup of 14 pull requests Successful merges: - rust-lang/rust#142807 (libtest: expose --fail-fast as an unstable command-line option) - rust-lang/rust#144871 (Stabilize `btree_entry_insert` feature) - rust-lang/rust#145071 (Update the minimum external LLVM to 20) - rust-lang/rust#145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables) - rust-lang/rust#145660 (initial implementation of the darwin_objc unstable feature) - rust-lang/rust#145838 (don't apply temporary lifetime extension rules to non-extended `super let`) - rust-lang/rust#146259 (Suggest removing Box::new instead of unboxing it) - rust-lang/rust#146410 (Iterator repeat: no infinite loop for `last` and `count`) - rust-lang/rust#146460 (Add tidy readme) - rust-lang/rust#146552 (StateTransform: Do not renumber resume local.) - rust-lang/rust#146564 (Remove Rvalue::Len again.) - rust-lang/rust#146581 (Detect attempt to use var-args in closure) - rust-lang/rust#146588 (tests/run-make: Update list of statically linked musl targets) - rust-lang/rust#146631 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3)) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 14 pull requests Successful merges: - rust-lang/rust#142807 (libtest: expose --fail-fast as an unstable command-line option) - rust-lang/rust#144871 (Stabilize `btree_entry_insert` feature) - rust-lang/rust#145071 (Update the minimum external LLVM to 20) - rust-lang/rust#145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables) - rust-lang/rust#145660 (initial implementation of the darwin_objc unstable feature) - rust-lang/rust#145838 (don't apply temporary lifetime extension rules to non-extended `super let`) - rust-lang/rust#146259 (Suggest removing Box::new instead of unboxing it) - rust-lang/rust#146410 (Iterator repeat: no infinite loop for `last` and `count`) - rust-lang/rust#146460 (Add tidy readme) - rust-lang/rust#146552 (StateTransform: Do not renumber resume local.) - rust-lang/rust#146564 (Remove Rvalue::Len again.) - rust-lang/rust#146581 (Detect attempt to use var-args in closure) - rust-lang/rust#146588 (tests/run-make: Update list of statically linked musl targets) - rust-lang/rust#146631 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3)) r? `@ghost` `@rustbot` modify labels: rollup
…r=jackh726 [beta-1.91] Warn on future errors from temporary lifetimes shortening in Rust 1.92 Pursuant to [discussion on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/474880-t-compiler.2Fbackports/topic/.23145838.3A.20beta-nominated/near/540530631), this implements a future-compatibility warning lint `macro_extended_temporary_scopes` for errors in Rust 1.92 caused by #145838: ``` warning: temporary lifetime shortening in Rust 1.92 --> $DIR/macro-extended-temporary-scopes.rs:54:14 | LL | &struct_temp().field | ^^^^^^^^^^^^^ this expression creates a temporary value... ... LL | } else { | - ...which will be dropped at the end of this block in Rust 1.92 | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see <https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#macro-extended-temporary-scopes> = note: consider using a `let` binding to create a longer lived value ``` Implementation-wise, this reuses the existing temporary scoping FCW machinery introduced for the `tail_expr_drop_order` edition lint: this adds `BackwardIncompatibleDropHint` statements to the MIR at the end of the shortened scopes for affected temporaries; these are then checked in borrowck to warn if the temporary is used after the future drop hint. There are trade-offs here: on one hand, I believe this gives some assurance over ad-hoc pattern-recognition that there are no false positives[^1]. On the other hand, this fails to lint on future dangling raw pointers and it complicates the potential addition of explanatory diagnostics or suggestions[^2]. I'm hopeful that the limitation around dangling pointers won't be relevant in real code, though; the only real instance we've seen of breakage so far is future errors in formatting macro invocations, which this should be able to catch. Release logistics notes: - This PR targets the beta branch directly, since the breakage it's a FCW for is landing in the next Rust version. - #146098 undoes the breakage this is a FCW for. If that behavior is merged and stabilizes in Rust 1.92, this PR should be reverted (or shouldn't be merged) in order to avoid spurious warnings. cc `@traviscross` `@rustbot` label +T-lang [^1]: In particular, more syntactic approaches are complicated by having to avoid warning on promoted constants; they'd either be full of holes, they'd need a lot of extra logic, or they'd need to hack more MIR-to-HIR mapping into `PromoteTemps`. [^2]: It's definitely possible to add more context and a suggestion, but the ways I've thought of to do so are either too hacky or too complex to feel appropriate for a last-minute direct-to-beta lint.
…r=jackh726 [beta-1.91] Warn on future errors from temporary lifetimes shortening in Rust 1.92 Pursuant to [discussion on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/474880-t-compiler.2Fbackports/topic/.23145838.3A.20beta-nominated/near/540530631), this implements a future-compatibility warning lint `macro_extended_temporary_scopes` for errors in Rust 1.92 caused by #145838: ``` warning: temporary lifetime shortening in Rust 1.92 --> $DIR/macro-extended-temporary-scopes.rs:54:14 | LL | &struct_temp().field | ^^^^^^^^^^^^^ this expression creates a temporary value... ... LL | } else { | - ...which will be dropped at the end of this block in Rust 1.92 | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see <https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#macro-extended-temporary-scopes> = note: consider using a `let` binding to create a longer lived value ``` Implementation-wise, this reuses the existing temporary scoping FCW machinery introduced for the `tail_expr_drop_order` edition lint: this adds `BackwardIncompatibleDropHint` statements to the MIR at the end of the shortened scopes for affected temporaries; these are then checked in borrowck to warn if the temporary is used after the future drop hint. There are trade-offs here: on one hand, I believe this gives some assurance over ad-hoc pattern-recognition that there are no false positives[^1]. On the other hand, this fails to lint on future dangling raw pointers and it complicates the potential addition of explanatory diagnostics or suggestions[^2]. I'm hopeful that the limitation around dangling pointers won't be relevant in real code, though; the only real instance we've seen of breakage so far is future errors in formatting macro invocations, which this should be able to catch. Release logistics notes: - This PR targets the beta branch directly, since the breakage it's a FCW for is landing in the next Rust version. - #146098 undoes the breakage this is a FCW for. If that behavior is merged and stabilizes in Rust 1.92, this PR should be reverted (or shouldn't be merged) in order to avoid spurious warnings. cc `@traviscross` `@rustbot` label +T-lang [^1]: In particular, more syntactic approaches are complicated by having to avoid warning on promoted constants; they'd either be full of holes, they'd need a lot of extra logic, or they'd need to hack more MIR-to-HIR mapping into `PromoteTemps`. [^2]: It's definitely possible to add more context and a suggestion, but the ways I've thought of to do so are either too hacky or too complex to feel appropriate for a last-minute direct-to-beta lint.
Reference PR: rust-lang/reference#1980
This changes the semantics for
super let
(and macros implemented in terms of it, such aspin!
,format_args!
,write!
, andprintln!
) as suggested by @theemathas in #145784 (comment), makingsuper let
initializers only count as extending expressions when thesuper let
itself is within an extending block. Sincesuper let
initializers aren't temporary drop scopes, their temporaries outside of inner temporary scopes are effectively always extended, even when not in extending positions; this only affects two cases as far as I can tell:f(pin!({ &temp() }))
droptemp()
at the end of the block in Rust 2024, whereas previously it would live until after the call tof
because syntactically thetemp()
was in an extending position as a result ofsuper let
inpin!
's expansion.super let
nested within a non-extendedsuper let
is no longer extended. i.e. a normallet
is required to treatsuper let
s as extending (in which case nestedsuper let
s will also be extending).Closes #145784
This is a breaking change. Both static and dynamic semantics are affected. The most likely breakage is for programs to stop compiling, but it's technically possible for drop order to silently change as well (as in #145784). Since this affects stable macros, it probably would need a crater run.
Nominating for discussion alongside #145784: @rustbot label +I-lang-nominated +I-libs-api-nominated
Tracking issue for
super let
: #139076