-
Notifications
You must be signed in to change notification settings - Fork 558
Specify temporary lifetime extension through expressions #2051
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
base: master
Are you sure you want to change the base?
Conversation
src/destructors.md
Outdated
| r[destructors.scope.lifetime-extension.exprs.extending] | ||
| For a let statement with an initializer, an *extending expression* is an | ||
| expression which is one of the following: | ||
| An *extending expression* is an expression which is one of the following: |
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.
A major change here: an extending expression is now any expression that preserves lifetime extension, defined non-inductively. I found this helps with generalizing the definition beyond let statement initializers, but I also often found myself having to refer to an expression being "extending when its parent is extending"; that's are now just an extending 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.
Yes, this greatly widens the definition of an extending expression, putting more load on the other rules.
src/destructors.md
Outdated
| * The operand of an extending [borrow] expression. | ||
| * The [super operands] of an extending [super macro call] expression. | ||
| * The operand(s) of an extending [array][array expression], [cast][cast | ||
| * The initializer expression of a `let` statement or the body expression of a [static][static item] or [constant item]. |
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.
Something slightly weird here: the last expression of a const block morally should be here, but it'd be a bit messy to have to exclude it from the rule for blocks lower down. Given that this definition of extending expressions doesn't care about where scopes are extended to, it shouldn't be a semantic issue, but it might warrant reformatting and/or an admonition.
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.
Let's see the admonition for this (with one or more examples).
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've handled this by making const block tail expressions no longer be defined as "extending" (along with let statement initializers and static/const item bodies). That way, there's less room for confusion, hopefully. I've also added an example of extending through a const block tail expression to illustrate it (taken from #2041)
src/destructors.md
Outdated
| r[destructors.scope.lifetime-extension.exprs.parent] | ||
| If a temporary scope is extended through the scope of an extending expression, it is extended through that scope's [parent][destructors.scope.nesting]. |
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 struggled a bit with how to express this; in its current form it's a bit of a hack. Ideally, I feel like it wouldn't need to be a separate rule or to refer to the definition of scope nesting, but it's working around something subtle: by ensuring that expressions' temporary scopes are only extended by their scope-ancestors, we can work around const blocks having parent expressions that (to my understanding) shouldn't be considered ancestor scopes of the const block's body; temporaries extended by const blocks are extended to the end of the program1. Maybe there's a simpler way to express this, and regardless it could probably use an admonition.
I do think that some sort of "extended by" or "extended through" or "extending based on" relation is necessary though, regardless of how exactly we choose to define/present it. I feel there's too much ambiguity if we can't precisely associate expressions we're extending the temporary scopes of with the scopes they're being extended to.
Footnotes
-
This PR doesn't make all the changes needed to iron that out, but see Further specify temporary scoping for
statics andconsts #2041. ↩
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.
Let's see the admonition that you have in mind here (with one or more examples). Let's also add rules to define what it means exactly to "extend to", "extend through", etc.
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.
In particular, what it means to "extend through" a parent scope is going to be worth careful explanation and one or more examples in an admonition.
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've fully reworked the formulation of the upward walk from & expressions and added examples.
src/destructors.md
Outdated
| ```rust,edition2024 | ||
| # fn temp() {} | ||
| # fn use_temp(_: &()) {} | ||
| // The final expression of a block is extending. Since the block below | ||
| // is not itself extending, the temporary is extended to the block | ||
| // expression's temporary scope, ending at the semicolon. | ||
| use_temp({ &temp() }); | ||
| // As above, the final expressions of `if`/`else` blocks are | ||
| // extending, which extends the temporaries to the `if` expression's | ||
| // temporary scope. | ||
| use_temp(if true { &temp() } else { &temp() }); | ||
| ``` |
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.
Some additional examples would probably be good. Maybe it would help to have one where temporaries are extended through a block but not to the end of a statement? That could also be used as a compile_fail example.
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.
Maybe it could also use some additional text (or even an admonition?) to make clear the interaction with if block scopes and Rust 2024's tail expression scopes. I'm not sure exactly how much explaining is needed for that, though.
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 all rather subtle. Erring on the side of more examples and explanation will be better here.
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 haven't added additional examples for this case of lifetime extension yet, but I fleshed out the first one quite a bit to walk through what's going on. I removed the second in the process, but I'll think on how best to readd it later (along with negative examples).
src/destructors.md
Outdated
| r[destructors.scope.lifetime-extension.exprs.let] | ||
| A temporary scope extended through a `let` statement scope is [extended] to the scope of the block containing the `let` statement ([destructors.scope.lifetime-extension.let]). | ||
|
|
||
| r[destructors.scope.lifetime-extension.exprs.static] | ||
| A temporary scope extended through a [static][static item] or [constant item] scope or a [const block][const block expression] scope is [extended] to the end of the program ([destructors.scope.lifetime-extension.static]). |
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 wonder if there's a way to cut down on the duplication here. I felt these rules were necessary to be precise about where temporaries' scopes are extended to, but having them in the introduction to the lifetime extension feels necessary too.
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.
Agreed they felt necessary here.
|
Thanks @dianne; will have a look. |
|
@ehuss and I looked carefully through this today. Some thoughts. After staring at this awhile, I see why this makes it easier to express the core idea of rust-lang/rust#146098 -- that the relative drop order within an expression should be consistent regardless of where that expression is. Expressing the rules inductively for defining an extending expression ends up pushing against this. So it does, I think, make a deep sort of sense to drop the inductive approach, as this PR does. At the same time, the inductive approach was carrying a lot of load. The reader could visualize walking down an expression, outside to inside, at each step checking whether it was still an extending expression. Then there was essentially one key rule that leaned on that:
What this PR does, in dropping the inductive framing, is to widen the definition of an extending expression significantly, and thereby put a lot more load on the rules that follow to narrow this back down. That's where it ends up getting a bit subtle. With this approach, both @ehuss and I found building intuition and checking the correctness of the rules to be more difficult. So, in dropping the inductive approach, I think we have some work to do in order to recover the same level of clarity. Adding some I'll leave some other thoughts inline. |
src/destructors.md
Outdated
| A temporary scope extended through a [static][static item] or [constant item] scope or a [const block][const block expression] scope is [extended] to the end of the program ([destructors.scope.lifetime-extension.static]). | ||
|
|
||
| r[destructors.scope.lifetime-extension.exprs.other] | ||
| A temporary scope extended through the scope of a non-extending expression is [extended] to that expression's [temporary scope]. |
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 rule is particular is one where I wonder how we might do better. The idea, currently, is that the rules above define when a temporary scope would be "extended through" the scope of an expression, and then this rule says, well, if it's a non-extending expression, then we only "extend to" it.
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've reworked this to avoid the hack of using scopes to express hierarchy entirely.
src/destructors.md
Outdated
| The operand of an extending [borrow] expression has its [temporary scope] [extended]. | ||
| The [temporary scope] of the operand of a [borrow] expression is *extended through* the scope of the borrow 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.
It's a bit subtle that, after having defined above what is an extending expression, that this rule then doesn't use that definition at all. It makes sense -- all operands of any borrow expression are extending. But it'll be worth adding an admonition under each of these rules that elaborates the rationale and gives one or more examples.
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've reworked this: now it appears before the definition of extending expressions. It's difficult to provide admonitions and examples before defining all the relevant terms, but I've tried to connect the rules together and provided some detailed examples towards the end.
|
I'll try and address individual points soon, but first one higher-level note that I'll try to use as guidance when restructuring:
This PR's approach still has a visual intuition to it, so it should be presented in a way that makes that clear: instead of walking down an extending expression to check whether an |
Yes, exactly. This upward walk is what I'd been expecting but it has to be teased out a bit from the presentation here. Hopefully making that more clear will make the rules here more clear. |
This comment has been minimized.
This comment has been minimized.
024f884 to
1fea72e
Compare
|
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. |
|
I did a pass to clean up definitions, reframe "extending based on expressions" as a property of |
| r[destructors.scope.lifetime-extension.exprs] | ||
| #### Extending based on expressions | ||
|
|
||
| r[destructors.scope.lifetime-extension.exprs.borrows] | ||
| The [temporary scope] of the operand of a [borrow] expression is the *extended scope* of the operand expression, defined below. | ||
|
|
||
| r[destructors.scope.lifetime-extension.exprs.super-macros] | ||
| The [scope][temporary scope] of each [super temporary] of a [super macro call] expression is the extended scope of the super macro call 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.
Two things here:
- I think it's helpful to start
destructors.scope.lifetime-extension.exprswith a straightforward explanation of what all the definitions are for. Otherwise, I found that either too many definitions are needed before any of them can be explained, or the explanations have to refer to terms that haven't been defined yet. For the sake of directness, I moveddestructors.scope.lifetime-extension.exprs.borrowsto the start, but that meant having to refer to terms that are defined later. A possible alternative would be writing a newdestructors.scope.lifetime-extension.exprs.introrule, but when I tried that it felt redundant. - I don't love the term "extended scope", but I wasn't able to think of anything better, either in terms of naming or in terms of reframing to avoid defining it at all. I feel like it could probably also use some additional clarification but I'm still thinking of a good admonition to add for it.
| r[destructors.scope.lifetime-extension.exprs.extending] | ||
| For a let statement with an initializer, an *extending expression* is an | ||
| expression which is one of the following: | ||
| The extended scope of an expression is defined in terms of *extending expressions* and their *extending parents*. An extending expression is an expression which is one of the following: |
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.
Three things here:
- To simplify the definitions of extending expressions and extended scopes, I've made
letinitializers,static/constitems, andconstblock tails no longer extending. This might hurt the intuition of extending expressions a bit though. - I added the definition of "extending parent" when reworking the upwards walk. This keeps it rigorous without hacks (the parent of an expression isn't defined elsewhere, and going through the parent of a scope to save words is confusing). It also also makes it clear that extending expressions are all subexpressions and thus have parent expressions. But spending words on it feels a bit awkward too, either adding bloat or making things convoluted (or both).
- Extending expressions are necessary to define extended scopes, but extended scopes are necessary to justify the definition of extending expressions. As such, I felt a need to connect this to the surrounding sections, but I don't love how it turned out. Maybe there's a better way to handle that.
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.
Following up on making "extending parents" make more sense and making it clear that "extending expression" isn't a classification of expressions but a classification of subexpressions, maybe it would be worth renaming "extending expression" to "extending subexpression"? I think it makes more sense from a spec point of view, but it's more awkward to read and write (and it means having to adapt language elsewhere), so I haven't gone through with it.
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.
It has some appeal, but I similarly see the problems you mention. If one says "extending subexpression", then one kind of wants the parent to be an "extending expression" (in the same way as "header", "subheader", etc.), but that's probably confusing.
1fea72e to
e37d803
Compare
| r[destructors.scope.lifetime-extension.sub-expressions] | ||
| If a [borrow], [dereference][dereference expression], [field][field expression], or [tuple indexing expression] has an extended temporary scope, then so does its operand. If an [indexing expression] has an extended temporary scope, then the indexed expression also has an extended temporary scope. |
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 think this may need a terminology update, a carve-out in destructors.scope.lifetime-extension.exprs, or at least clarification on what "extended temporary scope" means. In particular, if you have an expression like
{ &*&temp() }it's important that this rule takes precedence, so that temp() lives past the block. destructors.scope.lifetime-extension.exprs.borrows currently is worded to be the definitive lifetime of borrow operators' operands, but if only that was considered, temp() would be dropped at the end of the tail expression.
|
We looked through this in the lang-docs office hours call today. The recent changes here are great. Big improvement in clarity. Thanks to @dianne for those. We talked about how this might benefit from an introduction (perhaps in an admonition) to the "extending based on expressions" section that tries to give some intuition -- a reading guide -- to a reader coming into this section. I'm going to give this another careful read through, and @ehuss is planning to run some tests with the PR to make sure the interaction of this with the 2024 edition change is clear, but overall, this seems really close. |
Reference PR for rust-lang/rust#146098. This includes a reworked definition of extending expressions with the aim of expressing the new semantics more uniformly.