Fix EagerDoTag not committing results when partially evaluated#1081
Fix EagerDoTag not committing results when partially evaluated#1081jasmith-hs merged 6 commits intomasterfrom
Conversation
…cks aren't put onto the context
boulter
left a comment
boulter
left a comment
There was a problem hiding this comment.
The description of the solution makes sense, but I'm having trouble grasping what effect each of the code changes you made has and what they actually do. Perhaps you could comment on each change?
| @@ -0,0 +1,2 @@ | |||
| L1: ['a'] | |||
There was a problem hiding this comment.
for short files like these, I generally prefer to inline them in the test so it's easier to see the input and output without flipping between 3 files.
There was a problem hiding this comment.
That's a good point, but separating the input and outputs in separate files lets me have very readable and consistent tests.
These three files:
-commits-variables-from-do-tag-when-partially-resolved.jinja
-commits-variables-from-do-tag-when-partially-resolved.expected.jinja
-commits-variables-from-do-tag-when-partially-resolved.expected.expected.jinja
Self-annotate their relationship to each other and describe what they're testing.
Compare this to how a god-test-file will become complex and inconsistent when able to specify any sort of setup and assertions:
jinjava/src/test/java/com/hubspot/jinjava/interpret/DeferredTest.java
Lines 216 to 368 in 1a909fc
Writing my tests in this manner forces everything necessary for the test to be defined within the Jinjava template string, and also forces the assertions to be visible in the output, which makes it clear to demonstrate the impact.
I'll sometimes add these .expected.expected.jinja or it*SecondPass() tests when the initial output from eager execution isn't immediately obvious to be the correct answer.
In this case, it isn't terribly necessary. But for example with this test input:
{% for __ignored__ in [0] %}
{% set foo = deferred %}
{% endfor %}
{% set foo = deferred %}
{% for __ignored__ in [0] %}
{% if deferred %}
{{ foo }}
{% set foo = 'second' %}
{% endif %}
{{ foo }}
{% endfor %}
{{ foo }}
{% if deferred %}
{% set foo = 'second' %}
{% endif %}
{{ foo }}
The eager execution first-pass output is:
{% set my_list = ['a'] %}{% if deferred %}
{% set __macro_append_stuff_153654787_temp_variable_0__ %}
{% set __macro_foo_97643642_temp_variable_1__ %}
{% do my_list.append('b') %}
{% endset %}{{ __macro_foo_97643642_temp_variable_1__ }}
{% set __macro_foo_97643642_temp_variable_2__ %}
{% do my_list.append('c') %}
{% endset %}{{ __macro_foo_97643642_temp_variable_2__ }}
{% endset %}{{ __macro_append_stuff_153654787_temp_variable_0__ }}
{% endif %}
{% do my_list.append('d') %}
{{ my_list }}
The initial input is doing a roundabout way of creating a list and appending 'a', 'b', 'c', and 'd' to it, so it's useful to verify that the final output looks that way:
['a', 'b', 'c', 'd']
Defining these three strings in files which share a common naming scheme defines their relationship to each other, and ensures that the final output is always correct, even if there's some change in the eager execution code which slightly modifies the output of the first-phase
There was a problem hiding this comment.
I don't see how defining these strings statically in the class is any different than using external files, but you seem to feel strongly about this.
jasmith-hs
left a comment
jasmith-hs
left a comment
There was a problem hiding this comment.
Hopefully this is helpful @boulter
| @@ -0,0 +1,2 @@ | |||
| L1: ['a'] | |||
There was a problem hiding this comment.
That's a good point, but separating the input and outputs in separate files lets me have very readable and consistent tests.
These three files:
-commits-variables-from-do-tag-when-partially-resolved.jinja
-commits-variables-from-do-tag-when-partially-resolved.expected.jinja
-commits-variables-from-do-tag-when-partially-resolved.expected.expected.jinja
Self-annotate their relationship to each other and describe what they're testing.
Compare this to how a god-test-file will become complex and inconsistent when able to specify any sort of setup and assertions:
jinjava/src/test/java/com/hubspot/jinjava/interpret/DeferredTest.java
Lines 216 to 368 in 1a909fc
Writing my tests in this manner forces everything necessary for the test to be defined within the Jinjava template string, and also forces the assertions to be visible in the output, which makes it clear to demonstrate the impact.
I'll sometimes add these .expected.expected.jinja or it*SecondPass() tests when the initial output from eager execution isn't immediately obvious to be the correct answer.
In this case, it isn't terribly necessary. But for example with this test input:
{% for __ignored__ in [0] %}
{% set foo = deferred %}
{% endfor %}
{% set foo = deferred %}
{% for __ignored__ in [0] %}
{% if deferred %}
{{ foo }}
{% set foo = 'second' %}
{% endif %}
{{ foo }}
{% endfor %}
{{ foo }}
{% if deferred %}
{% set foo = 'second' %}
{% endif %}
{{ foo }}
The eager execution first-pass output is:
{% set my_list = ['a'] %}{% if deferred %}
{% set __macro_append_stuff_153654787_temp_variable_0__ %}
{% set __macro_foo_97643642_temp_variable_1__ %}
{% do my_list.append('b') %}
{% endset %}{{ __macro_foo_97643642_temp_variable_1__ }}
{% set __macro_foo_97643642_temp_variable_2__ %}
{% do my_list.append('c') %}
{% endset %}{{ __macro_foo_97643642_temp_variable_2__ }}
{% endset %}{{ __macro_append_stuff_153654787_temp_variable_0__ }}
{% endif %}
{% do my_list.append('d') %}
{{ my_list }}
The initial input is doing a roundabout way of creating a list and appending 'a', 'b', 'c', and 'd' to it, so it's useful to verify that the final output looks that way:
['a', 'b', 'c', 'd']
Defining these three strings in files which share a common naming scheme defines their relationship to each other, and ensures that the final output is always correct, even if there's some change in the eager execution code which slightly modifies the output of the first-phase
| !eagerExecutionResult.getResult().isFullyResolved() || | ||
| interpreter.getContext().isDeferredExecutionMode() | ||
| ) { | ||
| if (interpreter.getContext().isDeferredExecutionMode()) { |
There was a problem hiding this comment.
This change means that we no longer care if the {% do %} block is fully evaluated or not, because we'll always commit the result. Unless in deferred execution mode, for example we wouldn't need to commit in this case:
{% set list1 = ['a'] %}
{% if deferred %}
{% do %}
{% set list2 = ['b'] %}
{% do list1.append(['c']) %}
{% do list2.append(deferred) %}
{% enddo %}
{% endif %}
L1: {{ list1 }}
L2: {{ list2 }}
| entry -> ((OneTimeReconstructible) entry.getValue()).getOriginalValue() | ||
| ) | ||
| ) | ||
| .collect(Collectors.toMap(Entry::getKey, Entry::getValue)) |
There was a problem hiding this comment.
Purpose: Allow storing DeferredValues in the SpeculativeBindings
| ( | ||
| eagerExecutionResult.getResult().isFullyResolved() || | ||
| eagerChildContextConfig.takeNewValue | ||
| ) && | ||
| !(entry.getValue() instanceof DeferredValue) && | ||
| entry.getValue() != null |
There was a problem hiding this comment.
Moved this filtering to line 249
| entry.getKey(), | ||
| ((DeferredValue) contextValue).getOriginalValue() | ||
| ); | ||
| return new AbstractMap.SimpleImmutableEntry<>(entry.getKey(), contextValue); |
There was a problem hiding this comment.
Purpose: Allow storing DeferredValues in the SpeculativeBindings
| if (e.getValue() instanceof DeferredValue) { | ||
| return ((DeferredValue) e.getValue()).getOriginalValue(); | ||
| } |
There was a problem hiding this comment.
Purpose: Allow storing DeferredValues in the SpeculativeBindings
| .get(e.getKey()) | ||
| .equals(getObjectOrHashCode(((DeferredValue) e.getValue()).getOriginalValue())) | ||
| ) { | ||
| return ((DeferredValue) e.getValue()).getOriginalValue(); |
There was a problem hiding this comment.
Purpose: Allow storing DeferredValues in the SpeculativeBindings
| if (value instanceof DeferredValue) { | ||
| value = ((DeferredValue) value).getOriginalValue(); | ||
| } |
There was a problem hiding this comment.
Purpose: Since we were getting the original value before storing these as SpeculativeBindings, we now need to convert them to the original values here instead so that we don't have the values reconstructed like:
{% set foo = {'originalValue': 'foo string'} %}
| .forEach( | ||
| (k, v) -> { | ||
| if (v instanceof DeferredValue) { | ||
| v = ((DeferredValue) v).getOriginalValue(); | ||
| } | ||
| replace(interpreter.getContext(), k, v); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Purpose: this is used to reset the state of the context to what it was before some logic was run, which we aren't committing.
For example if evaluating something within a deferred {% if bool %} block, we'll want to reset the bindings to what they used to be, this includes makes DeferredValues no longer be deferred so that the {% else %} block will evaluate properly. We'll then re-defer any values that got deferred during any if-elif-else blocks.
That logic already happens, I'm just explaining why we'd want to put the DeferredValue#getOriginalValue() onto the context here
| EagerReconstructionUtils.commitSpeculativeBindings( | ||
| interpreter, | ||
| eagerExecutionResult | ||
| ); |
There was a problem hiding this comment.
I'm migrating to this for other tags to have consistency. EagerDoTag is the only tag that actually needs a minor logic change (in that we have to filter out the DeferredValueShadow values).
We'd also want to filter out any DeferredValueShadow values for any of the other tags, but I can't think of any way that we would end up having them there so there aren't any new tests I can write for that. This doesn't change any of the functionality that we know about/have tested. If there does happen to be some situation where that could happen, then we'd filter those, which is desirable
boulter
left a comment
boulter
left a comment
There was a problem hiding this comment.
I think I understand this now. Thanks.
3 participants
In EagerDoTag, because of this logic we weren't committing information about variables which were declared inside the
{% do %}:The do block doesn't create a child scope so variables declared inside of it should propagate outside of the block. This was happening when it was fully resolved, but when it was partially resolved, we weren't committing the results. Specifically, we weren't either deferring the variable or taking any fully resolved values.
To do this, we need to distinguish between which speculative bindings (those child bindings that come from evaluating the
{% do %}block) are fully resolved, and which are deferred values.This distinction is well demonstrated in this test case:
list1is fully resolved when exiting the{% do %}block, butlist2is deferred. We want to commit that information and make the context havelist1=['a']andlist2=DeferredValue.instance(['b']). This way we can resolve{{ list1 }}, but not resolve{{ list2 }}. This is what we need, and before this PR, bothlist1andlist2would remain null so they'd resolve to nothing.So to make this change, instead of converting DeferredValues back to their original value anytime we add one to the
SpeculativeBindings, we'll only convert back to the original value when callingEagerReconstructionUtils#resetSpeculativeBindingsor when creating a set tag to reconstruct the value.