-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Assert that non-extended temporaries and super let
bindings have scopes
#147471
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
r? @nnethercote rustbot has assigned @nnethercote. Use |
// Similarly, `const X: ... = &f();` would have the result of `f()` | ||
// live for `'static`, implying (if Drop restrictions on constants | ||
// ever get lifted) that the value *could* have a destructor, but | ||
// it'd get leaked instead of the destructor running during the | ||
// evaluation of `X` (if at all allowed by CTFE). |
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.
I can't tell if the remarks on on Drop
restrictions are out of date or just worded a bit confusingly. I'm also not sure whether this PR is the right place to update that.
af03a85
to
444eb4c
Compare
I'm not sure what this means. But I just wanted to note that constant-promotion in consts/statics can have a similar effect to lifetime extension, and has different behavior from constant-promotion outside of consts/statics. See #124328 |
I've clarified the PR description; thanks. Does it read better now? This is only about the temporary scopes assigned to expressions before |
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.
This is stuff I know very little about, but the change is small enough and well explained so I'm happy with it.
@bors r+ rollup |
…nethercote Assert that non-extended temporaries and `super let` bindings have scopes This PR clarifies a point of confusion in the compiler: all bodies have an outer temporary drop scope, including `static` and `const` item bodies[^1]. Whenever a temporary should be dropped in its enclosing temporary scope, it should have a temporary scope to be dropped in so that its drop can be scheduled[^2]. As such, I've updated some relevant comments and made `ScopeTree::default_temporary_scope` and `RvalueScopes::temporary_scope` panic when an enclosing temporary scope isn't found instead of allowing potential bugs where potentially-drop-sensitive temporaries are effectively given static lifetimes. Since non-extended `super let` bindings are dropped in their block's enclosing temporary scope, this applies to them as well: the enclosing temporary scope should exist. [^1]: See https://github.com/rust-lang/rust/blob/master/compiler/rustc_hir_analysis/src/check/region.rs#L773-L778 for non-`fn`/closure bodies. The `this.cx.var_parent = None;` enables [lifetime extension to `'static` lifetimes](https://doc.rust-lang.org/stable/reference/destructors.html#r-destructors.scope.lifetime-extension.static) and the `ScopeData::Destruction` scope ensures non-extended temporaries are dropped in the body expression's scope. [^2]: For certain borrowed temporaries, drops that don't require running destructors may later be removed by constant promotion. That is unrelated to this PR.
Rollup of 7 pull requests Successful merges: - #144266 (Supress swapping lhs and rhs in equality suggestion in extern macro ) - #147471 (Assert that non-extended temporaries and `super let` bindings have scopes) - #147533 (Renumber return local after state transform) - #147566 (rewrite outlives placeholder constraints to outlives static when handling opaque types) - #147613 (Make logging filters work again by moving EnvFilter into its own layer) - #147615 (reduce calls to attr.span() in old doc attr parsing) - #147636 (miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #147471 - dianne:assert-temporary-scope, r=nnethercote Assert that non-extended temporaries and `super let` bindings have scopes This PR clarifies a point of confusion in the compiler: all bodies have an outer temporary drop scope, including `static` and `const` item bodies[^1]. Whenever a temporary should be dropped in its enclosing temporary scope, it should have a temporary scope to be dropped in so that its drop can be scheduled[^2]. As such, I've updated some relevant comments and made `ScopeTree::default_temporary_scope` and `RvalueScopes::temporary_scope` panic when an enclosing temporary scope isn't found instead of allowing potential bugs where potentially-drop-sensitive temporaries are effectively given static lifetimes. Since non-extended `super let` bindings are dropped in their block's enclosing temporary scope, this applies to them as well: the enclosing temporary scope should exist. [^1]: See https://github.com/rust-lang/rust/blob/master/compiler/rustc_hir_analysis/src/check/region.rs#L773-L778 for non-`fn`/closure bodies. The `this.cx.var_parent = None;` enables [lifetime extension to `'static` lifetimes](https://doc.rust-lang.org/stable/reference/destructors.html#r-destructors.scope.lifetime-extension.static) and the `ScopeData::Destruction` scope ensures non-extended temporaries are dropped in the body expression's scope. [^2]: For certain borrowed temporaries, drops that don't require running destructors may later be removed by constant promotion. That is unrelated to this PR.
Rollup of 7 pull requests Successful merges: - rust-lang/rust#144266 (Supress swapping lhs and rhs in equality suggestion in extern macro ) - rust-lang/rust#147471 (Assert that non-extended temporaries and `super let` bindings have scopes) - rust-lang/rust#147533 (Renumber return local after state transform) - rust-lang/rust#147566 (rewrite outlives placeholder constraints to outlives static when handling opaque types) - rust-lang/rust#147613 (Make logging filters work again by moving EnvFilter into its own layer) - rust-lang/rust#147615 (reduce calls to attr.span() in old doc attr parsing) - rust-lang/rust#147636 (miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup
This PR clarifies a point of confusion in the compiler: all bodies have an outer temporary drop scope, including
static
andconst
item bodies1. Whenever a temporary should be dropped in its enclosing temporary scope, it should have a temporary scope to be dropped in so that its drop can be scheduled2. As such, I've updated some relevant comments and madeScopeTree::default_temporary_scope
andRvalueScopes::temporary_scope
panic when an enclosing temporary scope isn't found instead of allowing potential bugs where potentially-drop-sensitive temporaries are effectively given static lifetimes.Since non-extended
super let
bindings are dropped in their block's enclosing temporary scope, this applies to them as well: the enclosing temporary scope should exist.Footnotes
See https://github.com/rust-lang/rust/blob/master/compiler/rustc_hir_analysis/src/check/region.rs#L773-L778 for non-
fn
/closure bodies. Thethis.cx.var_parent = None;
enables lifetime extension to'static
lifetimes and theScopeData::Destruction
scope ensures non-extended temporaries are dropped in the body expression's scope. ↩For certain borrowed temporaries, drops that don't require running destructors may later be removed by constant promotion. That is unrelated to this PR. ↩