yeast: Add context scoping mechanism#22136
Conversation
There's an awkward divide in yeast between returning a list of nodes or optional node (both of which are iterable), and returning a single node (which is not). In practice, we would like all of these cases to be handled transparently: if a single node is returned, it behaves as if it were a singleton list containing that node. This gives us a uniform interface during translation -- no matter what is returned, it will be an iterable of nodes. To facilitate this, we make the slightly unorthodox choice of implementing IntoIterator for Id, with the behaviour detailed above.
A previous commit added a translate_reset method on BuildCtx, which had the effect of performing a translation in a completely empty context. One issue with this is that this is an all-or-nothing proposition -- If you want to preserve _some_ parts of the context, you have to do something more complicated. Moreover, if you introduce a contextual value that _should_ be preserved, all of the existing uses of translate_reset now silently do the wrong thing. There are two patterns that we want to address. The first one is "modify the context in some way, then do a translation". If the translation is the last step of a Rust block, then we don't actually need translate_reset -- we could just reset the context and then call `translate`. The fact that the outer context is restored afterwards means it's okay to make destructive changes to `ctx.user_ctx` -- none of these changes will persist. The second pattern is the same, but where we want to do more translations using the original context, after having performed a translation with a modified context. In this case, we cannot just overwrite the context, since that would invalidate the subsequent translations. Instead, we introduce a new method `ctx.scoped` which takes a closure as an argument. With this we can now write ``` ctx.scoped(|ctx| ctx.reset(); ctx.translate(...)); ``` and the closure is run with a copy of `ctx` that has a clone of `user_ctx` on the inside, so no changes will persist. (You may wonder: why not just clone `ctx` and use the clone? The answer is that `ctx` owns mutable pointers to the AST etc., and this makes it awkward to just "clone" it. The closure circumvents this issue nicely, since it can borrow these pointers internally.) For now, this rewrite has the same behaviour as the version that used translate_reset -- we clear the entire `user_ctx`. However, we could imagine being more fine-grained in this approach, by implementing, say, SwiftContext::reset_modifiers (which would only affect what modifiers are currently in the context, leaving everything else as-is).
In order to avoid having context changes bubble up through the tree (or from one sibling to another) the current framework makes a copy of the context before calling `translate` recursively, and then restores it afterwards. However, this is a bit silly -- after we're done with all of the translations, there's really no need to restore the context (as it doesn't get accessed again). So instead we change it from "save and then restore" to "clone and then drop". Each rule invocation gets its own copy of the context, and simply drops it when it's done.
1c035f5 to
9b7a9c3
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Yeast translation framework and Swift extractor rules to replace the previous “reset the entire user context” helper with an explicit, more fine-grained scoping mechanism. It introduces BuildCtx::scoped for temporary context mutation (without leaking changes) and moves “reset” semantics into the Swift-specific SwiftContext::reset, aligning the framework with language-specific needs.
Changes:
- Add
BuildCtx::scoped(...)to run a closure with a temporary child context backed by a cloneduser_ctx, discarding mutations on return. - Change Yeast’s context isolation implementation from “clone + restore” to “clone + drop” per rule attempt.
- Update Swift translation rules to use
SwiftContext::reset()plusctx.scoped(...)where the outer context must remain intact after translating a reset subtree.
Show a summary per file
| File | Description |
|---|---|
| unified/extractor/src/languages/swift/swift.rs | Adds SwiftContext::reset and replaces translate_reset usage with explicit reset + optional ctx.scoped(...) to avoid context leakage while preserving needed outer context. |
| shared/yeast/src/lib.rs | Adds Id: IntoIterator (singleton), makes translator handles Copy unconditionally, and switches rule context isolation to “clone and drop” for simpler, ?-safe non-leaking behavior. |
| shared/yeast/src/build.rs | Updates BuildCtx::translate to accept any iterable of ids (including a bare Id) and introduces BuildCtx::scoped for temporary child-context execution. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
jketema
left a comment
There was a problem hiding this comment.
Two questions, and I believe I spotted a typo.
| let val = ctx.scoped(|ctx| { | ||
| ctx.reset(); | ||
| ctx.translate(val) | ||
| })?; |
There was a problem hiding this comment.
This begs the question: why do we need scoped here, but not in any of the other places where we had translate_reset before?
There was a problem hiding this comment.
The difference here is that we want to use ctx (or more specifically the user-controlled user_ctx field on it -- which through some Deref magic is directly accessible on ctx itself) further down in the body of this rule. When we get to ctx.translate(obs), we want that user context to not have been reset. But we've already put var_decl into the result, and this required var to have been translated.
Thus, we have to introduce a temporary context -- or at least it's the easiest way to get the desired behaviour. (I guess technically we could do the var stuff at the end, and then prepend it onto result but I think that's less clear than the current implementation.)
There was a problem hiding this comment.
Let me rephrase, because that's not really helping me. I assume that a context here roughly corresponds to some type of scope in Swift. If so, from the perspective of the Swift AST, what is different here than in the other cases where we had translate_reset.
There was a problem hiding this comment.
The context is a mechanism for passing information from a node down to its descendants. For instance, In some cases we have an outer type that we want to propagate down into some descendant. In this case, we set the appropriate part of the SwiftContext (which is just a struct that we can choose the content of), specifically the following field:
In the previous code, we had two different kinds of translate_reset calls. The simple uses just had things like
(accessor_declaration
body: (block stmt: {ctx.translate_reset(body)?})
)
This had the effect of translating body within an empty context. (That is, it temporarily saves the current context, then creates an empty context, runs the translation, and then restores the saved context.)
But in this case, there's no reason to restore the context -- it gets dropped anyway afterwards, which is why we can just rewrite it to use reset and then the normal translate.
(accessor_declaration
body: (block stmt: {ctx.reset(); ctx.translate(body)?})
)
The difficult case is the following (eliding the unimportant bits):
// previously: val = translate_reset(val)?;
let val = ctx.scoped(|ctx| {
ctx.reset();
ctx.translate(val)
})?;
let var_decl = tree!(
(variable_declaration
modifier: {ctx.outer_modifiers.clone()}
modifier: {chained_modifier(&mut ctx)}
pattern: (name_pattern identifier: (identifier #{name}))
type: {ty}
value: {val})
);
// Publish the property name for the observer rules.
ctx.property_name = Some(tree!((identifier #{name})));
// Observers are subsequent outputs of this flattening
// rule, so they always get `chained_declaration`.
ctx.is_chained = true;
let mut result = vec![var_decl];
for obs in ws.into_iter().chain(ds) {
result.extend(ctx.translate(obs)?);
}
resultThe behaviour is the same as before -- we just express it slightly differently. The important thing is that when we do ctx.translate(val) we need the context to be cleared, otherwise things get translated incorrectly. However, when we do ctx.translate(obs), it shouldn't be cleared (or at least, that's the current behaviour). So, currently we use translate_reset to provide a temporary empty context for the val translation, and for the rewrite we explicitly call ctx.scoped, which allows us to make any changes to ctx that we like without it affecting anything outside the scope.
Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
An addendum to #22120. Removes
translate_resetin favour of a mechanism that is a bit less "all or nothing".The main addition is a new method
ctx.scopedwhich takes a closure as an argument. Thus, where we previously wrotewith
translate_resettaking care of nuking the entireuser_ctx, we now instead writeNote that in particular,
resetis now a method onSwiftContext, not something that the yeast framework provides. This means we can easily make it more fine-grained, e.g. replacing it with a method that just resets the current modifiers.Also, for cases where we don't need
ctxafter doing the translation, we can simply callctx.reset()without the scope -- the changes to the context are prevented from bleeding through by means of the existing context isolation mechanism.Should be reviewed commit-by-commit. The final commit changes the isolation mechanism from "save and then restore" to "clone and then drop", which saves some unnecessary work, and is otherwise just as good.