Repository navigation
Conversation
tausbn
force-pushed
the
tausbn/yeast-isolate-interpolation-contexts
branch
3 times, most recently
from
November 6, 2026 15:19
027df4b to
81057bd
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Interpolation handling can fail to compile with borrowed builders and leak context mutations across translations, while public query failures can lose readable diagnostics.
6 open issues
Review effort: Max
Wraps each interpolation is a small bit of code that clones the user context before running the block of code, and restores it afterwards. We only do this for blocks that actually mention the `ctx` variable. Other cases (the vast majority) just inline the block directly, so that there's no unnecessary overhead in the most common case. The `ctx` check is not perfect -- it won't notice cases where you (outside the interpolation) rebind it to a different variable, say, but it should be a good enough test for the way we currently write rules.
This allows us to get rid of two uses of `ctx.scoped`, cleaning up the code a bit in the process.
tausbn
force-pushed
the
tausbn/yeast-isolate-interpolation-contexts
branch
from
November 9, 2026 13:05
3d4f761 to
ee41076
Compare
tausbn
marked this pull request as ready for review
November 9, 2026 13:09
paldepind
requested changes
Nov 10, 2026
paldepind
left a comment
paldepind
left a comment
Contributor
There was a problem hiding this comment.
Some nitpick comments, otherwise this LGTM.
|
|
||
| #[derive(Clone, Default)] | ||
| struct InterpolationContext { | ||
| counter: usize, |
Contributor
There was a problem hiding this comment.
I think using a normal unsigned integer would be more natural here.
Suggested change
| counter: usize, | |
| counter: u64, |
| } | ||
| } | ||
|
|
||
| fn block_uses_ctx(block: &syn::Block) -> bool { |
Contributor
There was a problem hiding this comment.
Maybe at a doc comment for this function that also mentions that this is an imperfect check?
Also changes the `InterpolationContext` counter to use `u64` instead of `usize`.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently we have context isolation between rules: a change to
ctxis visible when we translate child nodes (if the translation happens after the change, of course), but these changes cannot leak to different rules.However, the same isolation does not apply to context modifications made inside of the interpolations of an output template. That is, in the following template
both
barandbazwill be evaluated in a reset context. If we swap the two field assignments, then onlybargets a reset context.This behaviour is very surprising, and likely to cause hard-to-spot bugs.
To fix this, we now clone the user context for each interpolation individually (but only if they refer to the
ctxvariable), which means they cannot interfere with each other. This is slightly less efficient, but much safer.This also enables us to get rid of the two uses of
ctx.scopedthat we currently have, and clean up the code a bit.