Track modified variables better in child-scoped eager execution #1032
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.
Depends on #1030
There was a problem which I found after #1020, which made this bug easier to happen, in which a variable would be reconstructed, but it would reside only within the macro scope. If that variable was defined outside of the macro function, and the macro function modifies that variable, then it is problematic.
I wanted to make changes to the way that we evaluate macro functions in eager execution, and I didn't like having all the eager execution logic mixed in with the non-eager logic so I separated it into EagerMacroFunction, which I reworked to extend MacroFunction. I also added some deduping to the deferred lazy reference logic, and the prefix to preserve state logic by making it into a LinkedHashMap rather than just a string, which would allow me to remove the duplicates and override the reconstructed version for a string. (Deferred lazy references made it complicated because you'd want them reconstructed before anything else, but you wouldn't want the same key to have a different reconstruction later in the same PrefixToPreserveString. That's where the LinkedHashMap deduping helped).
I also needed to change how we put deferred values onto the context to be more accurate, and how we reset speculative bindings. The gist of these changes are that a
DeferredValueShadowis used to mark that a value was deferred on that context scope (or a child of it), but it isn't the actual source of the value. Once we find the source of the value, we stop deferring. To visualise, that looks like this, imagine1is the highest level context (global) and5is the lowest level context.A variable
foois set. Then in a child scope, anotherfoois set, but it resides in a child scope. The context starts looking like this:Then if when on context level
5, we end up having a deferred token which does this in deferred execution mode:{% do foo.append('b') %}, we'd havefooas ausedDeferredWordin the DeferredToken so we'd end up with the context looking like this after the DeferredToken is handled on the context:The DeferredValueShadow lets us reset bindings properly as before we were just replacing the lowest-level existing value with the original value
["a"], but now we easily revert the context back to what it was previously if we need to by removing the DeferredValueShadows forfooand replacing3:foowith its original value.The big simplification of what I needed to change in the eager execution macro function logic was to make it more like how we are eagerly evaluating a for loop. It needs to run in a child context using EagerContextWatcher so that we can determine if it is fully resolved and track the modified variables properly. Currently, it runs in its own scope, but it wasn't using EagerContextWatcher so modified variables weren't getting reconstructed properly. This is the current logic:
jinjava/src/main/java/com/hubspot/jinjava/lib/fn/MacroFunction.java
Lines 80 to 103 in 189046b
Here's an example:
Currently, this gets output like:
Oops! We are creating
my_listin a child scope, which means that it's different from the originalmy_list, so the final{{ my_list }}won't havebinside of it.After these changes, it is output like this, which is much better (and correct):
The if statement isn't creating a new scope, so we are still modifying the original
my_listwhen callingmy_list.append('b'), which is what we want so that{{ my_list }}is output as['b']when rendering a second pass.Eager execution is tricky when entering into a child scope (such as is done with macro functions, for tags, and set blocks) so I tried to reuse some of the logic between them. Also, deferred evaluation of if-branches has to similarly reset the speculative bindings. But they are each quite different: