Refactor createReconstructedContext into checkForContextChanges

jasmith-hs · jasmith-hs · commit e79b14ff7a2f · 2022-12-21T11:23:18.000-05:00
diff --git a/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java b/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java
@@ -44,7 +44,6 @@ private String eagerResolveExpression(
         .withPartialMacroEvaluation(
           interpreter.getConfig().isNestedInterpretationEnabled()
         )
-        .withCheckForContextChanges(interpreter.getContext().isDeferredExecutionMode())
         .build()
     );
 
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerCallTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerCallTag.java
@@ -62,7 +62,6 @@ public String eagerInterpret(
           .withPartialMacroEvaluation(
             interpreter.getConfig().isNestedInterpretationEnabled()
           )
-          .withCheckForContextChanges(interpreter.getContext().isDeferredExecutionMode())
           .build()
       );
       StringBuilder prefixToPreserveState = new StringBuilder();
@@ -140,7 +139,6 @@ public String eagerInterpret(
               interpreter,
               EagerChildContextConfig
                 .newBuilder()
-                .withCheckForContextChanges(true)
                 .withForceDeferredExecutionMode(true)
                 .build()
             )
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerCycleTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerCycleTag.java
@@ -48,11 +48,7 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
       eagerInterpreter ->
         EagerExpressionResolver.resolveExpression(expression, interpreter),
       interpreter,
-      EagerChildContextConfig
-        .newBuilder()
-        .withTakeNewValue(true)
-        .withCheckForContextChanges(interpreter.getContext().isDeferredExecutionMode())
-        .build()
+      EagerChildContextConfig.newBuilder().withTakeNewValue(true).build()
     );
 
     StringBuilder prefixToPreserveState = new StringBuilder();
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java
@@ -45,11 +45,7 @@ public String innerInterpret(TagNode tagNode, JinjavaInterpreter interpreter) {
         return expressionResult;
       },
       interpreter,
-      EagerChildContextConfig
-        .newBuilder()
-        .withCheckForContextChanges(true)
-        .withCreateReconstructedContext(true)
-        .build()
+      EagerChildContextConfig.newBuilder().withCheckForContextChanges(true).build()
     );
     if (
       result.getResult().getResolutionState() == ResolutionState.NONE ||
@@ -164,11 +160,7 @@ private EagerExecutionResult runLoopOnce(
         );
       },
       interpreter,
-      EagerChildContextConfig
-        .newBuilder()
-        .withForceDeferredExecutionMode(true)
-        .withCheckForContextChanges(true)
-        .build()
+      EagerChildContextConfig.newBuilder().withForceDeferredExecutionMode(true).build()
     );
   }
 
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerIfTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerIfTag.java
@@ -64,7 +64,6 @@ public String eagerInterpret(
           EagerChildContextConfig
             .newBuilder()
             .withForceDeferredExecutionMode(true)
-            .withCheckForContextChanges(true)
             .build()
         )
         .asTemplateString()
@@ -118,7 +117,6 @@ public String eagerRenderBranches(
           EagerChildContextConfig
             .newBuilder()
             .withForceDeferredExecutionMode(true)
-            .withCheckForContextChanges(true)
             .build()
         );
         sb.append(result.getResult());
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerInlineSetTagStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerInlineSetTagStrategy.java
@@ -35,11 +35,7 @@ public EagerExecutionResult getEagerExecutionResult(
       eagerInterpreter ->
         EagerExpressionResolver.resolveExpression('[' + expression + ']', interpreter),
       interpreter,
-      EagerChildContextConfig
-        .newBuilder()
-        .withTakeNewValue(true)
-        .withCheckForContextChanges(interpreter.getContext().isDeferredExecutionMode())
-        .build()
+      EagerChildContextConfig.newBuilder().withTakeNewValue(true).build()
     );
   }
 
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java
@@ -56,11 +56,7 @@ public static String interpretExpression(
     EagerExecutionResult eagerExecutionResult = EagerReconstructionUtils.executeInChildContext(
       eagerInterpreter -> EagerExpressionResolver.resolveExpression(expr, interpreter),
       interpreter,
-      EagerChildContextConfig
-        .newBuilder()
-        .withTakeNewValue(true)
-        .withCheckForContextChanges(interpreter.getContext().isDeferredExecutionMode())
-        .build()
+      EagerChildContextConfig.newBuilder().withTakeNewValue(true).build()
     );
     StringBuilder prefixToPreserveState = new StringBuilder();
     if (interpreter.getContext().isDeferredExecutionMode()) {
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerStateChangingTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerStateChangingTag.java
@@ -44,7 +44,6 @@ public String eagerInterpret(
             interpreter,
             EagerChildContextConfig
               .newBuilder()
-              .withCheckForContextChanges(true)
               .withForceDeferredExecutionMode(true)
               .build()
           )
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerTagDecorator.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerTagDecorator.java
@@ -114,7 +114,6 @@ public String eagerInterpret(
           EagerChildContextConfig
             .newBuilder()
             .withForceDeferredExecutionMode(true)
-            .withCheckForContextChanges(true)
             .build()
         )
         .asTemplateString()
diff --git a/src/main/java/com/hubspot/jinjava/util/DeferredValueUtils.java b/src/main/java/com/hubspot/jinjava/util/DeferredValueUtils.java
@@ -164,7 +164,6 @@ public static Set<String> findAndMarkDeferredProperties(
               .getScope()
               .entrySet()
               .stream()
-              //              .filter(entry -> !entry.getKey().equals(word))
               .filter(entry -> entry.getValue() == wordValue)
               .forEach(
                 entry -> {
@@ -174,9 +173,6 @@ public static Set<String> findAndMarkDeferredProperties(
                     .setReferenceKey(entry.getKey());
                 }
               );
-            //              .forEach(
-            //                entry -> entry.setValue(DeferredLazyReference.instance(context, word))
-            //              );
             temp = temp.getParent();
           }
           if (matchingEntries.size() > 1) { // at least one duplicate
diff --git a/src/main/java/com/hubspot/jinjava/util/EagerReconstructionUtils.java b/src/main/java/com/hubspot/jinjava/util/EagerReconstructionUtils.java
@@ -82,7 +82,6 @@ public static EagerExecutionResult executeInChildContext(
       EagerChildContextConfig
         .newBuilder()
         .withTakeNewValue(takeNewValue)
-        .withCheckForContextChanges(checkForContextChanges)
         .withForceDeferredExecutionMode(checkForContextChanges)
         .withPartialMacroEvaluation(partialMacroEvaluation)
         .build()
@@ -98,7 +97,7 @@ public static EagerExecutionResult executeInChildContext(
     Set<String> metaContextVariables = interpreter.getContext().getMetaContextVariables();
     final Map<String, Object> initiallyResolvedHashes;
     final Map<String, String> initiallyResolvedAsStrings;
-    if (eagerChildContextConfig.createReconstructedContext) {
+    if (eagerChildContextConfig.checkForContextChanges) {
       initiallyResolvedHashes =
         interpreter
           .getContext()
@@ -157,7 +156,7 @@ public static EagerExecutionResult executeInChildContext(
           ? new HashMap<>()
           : interpreter.getContext().getSessionBindings();
     }
-    if (eagerChildContextConfig.createReconstructedContext) {
+    if (eagerChildContextConfig.checkForContextChanges) {
       speculativeBindings =
         getAllSpeculativeBindings(
           interpreter,
@@ -401,7 +400,7 @@ private static Object getObjectOrHashCode(
     if (o instanceof LazyExpression) {
       o = ((LazyExpression) o).get();
     }
-    if (eagerChildContextConfig.createReconstructedContext) {
+    if (eagerChildContextConfig.checkForContextChanges) {
       if (o instanceof PyList && !((PyList) o).toList().contains(o)) {
         return o.hashCode();
       }
@@ -483,7 +482,6 @@ private static String reconstructMacroFunctionsBeforeDeferring(
             EagerChildContextConfig
               .newBuilder()
               .withForceDeferredExecutionMode(true)
-              .withCheckForContextChanges(true)
               .build()
           )
       )
@@ -963,24 +961,21 @@ public static class EagerChildContextConfig {
 
     private final boolean discardSessionBindings;
     private final boolean partialMacroEvaluation;
-    private final boolean checkForContextChanges;
 
-    private final boolean createReconstructedContext;
+    private final boolean checkForContextChanges;
     private final boolean forceDeferredExecutionMode;
 
     private EagerChildContextConfig(
       boolean takeNewValue,
       boolean discardSessionBindings,
       boolean partialMacroEvaluation,
       boolean checkForContextChanges,
-      boolean createReconstructedContext,
       boolean forceDeferredExecutionMode
     ) {
       this.takeNewValue = takeNewValue;
       this.discardSessionBindings = discardSessionBindings;
       this.partialMacroEvaluation = partialMacroEvaluation;
       this.checkForContextChanges = checkForContextChanges;
-      this.createReconstructedContext = createReconstructedContext;
       this.forceDeferredExecutionMode = forceDeferredExecutionMode;
     }
 
@@ -994,8 +989,6 @@ public static class Builder {
       private boolean discardSessionBindings;
       private boolean partialMacroEvaluation;
       private boolean checkForContextChanges;
-
-      private boolean createReconstructedContext;
       private boolean forceDeferredExecutionMode;
 
       private Builder() {}
@@ -1020,11 +1013,6 @@ public Builder withCheckForContextChanges(boolean checkForContextChanges) {
         return this;
       }
 
-      public Builder withCreateReconstructedContext(boolean createReconstructedContext) {
-        this.createReconstructedContext = createReconstructedContext;
-        return this;
-      }
-
       public Builder withForceDeferredExecutionMode(boolean forceDeferredExecutionMode) {
         this.forceDeferredExecutionMode = forceDeferredExecutionMode;
         return this;
@@ -1036,7 +1024,6 @@ public EagerChildContextConfig build() {
           discardSessionBindings,
           partialMacroEvaluation,
           checkForContextChanges,
-          createReconstructedContext,
           forceDeferredExecutionMode
         );
       }
diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerForTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerForTagTest.java
@@ -1,22 +1,19 @@
 package com.hubspot.jinjava.lib.tag.eager;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.in;
 
 import com.google.common.collect.ImmutableList;
 import com.hubspot.jinjava.ExpectedNodeInterpreter;
 import com.hubspot.jinjava.JinjavaConfig;
 import com.hubspot.jinjava.LegacyOverrides;
 import com.hubspot.jinjava.interpret.DeferredValue;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
-import com.hubspot.jinjava.interpret.TemplateError.ErrorReason;
 import com.hubspot.jinjava.lib.tag.ForTagTest;
 import com.hubspot.jinjava.mode.EagerExecutionMode;
 import com.hubspot.jinjava.tree.parse.TagToken;
 import java.util.Optional;
 import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public class EagerForTagTest extends ForTagTest {
diff --git a/src/test/java/com/hubspot/jinjava/util/EagerReconstructionUtilsTest.java b/src/test/java/com/hubspot/jinjava/util/EagerReconstructionUtilsTest.java
@@ -80,7 +80,6 @@ public void itExecutesInChildContextAndTakesNewValue() {
         .withTakeNewValue(true)
         .withForceDeferredExecutionMode(true)
         .withCheckForContextChanges(true)
-        .withCreateReconstructedContext(true)
         .build()
     );
 
@@ -110,7 +109,6 @@ public void itExecutesInChildContextAndDefersNewValue() {
         .newBuilder()
         .withForceDeferredExecutionMode(true)
         .withCheckForContextChanges(true)
-        .withCreateReconstructedContext(true)
         .build()
     );
     assertThat(result.getResult().toString()).isEqualTo("function return");
@@ -381,7 +379,7 @@ public void itDiscardsSessionBindings() {
       EagerChildContextConfig
         .newBuilder()
         .withDiscardSessionBindings(false)
-        .withCreateReconstructedContext(true)
+        .withCheckForContextChanges(true)
         .build()
     );
     EagerExecutionResult withoutSessionBindings = EagerReconstructionUtils.executeInChildContext(
@@ -393,7 +391,7 @@ public void itDiscardsSessionBindings() {
       EagerChildContextConfig
         .newBuilder()
         .withDiscardSessionBindings(true)
-        .withCreateReconstructedContext(true)
+        .withCheckForContextChanges(true)
         .build()
     );
     assertThat(withSessionBindings.getSpeculativeBindings())
diff --git a/src/test/resources/eager/handles-higher-scope-reference-modification.expected.expected.jinja b/src/test/resources/eager/handles-higher-scope-reference-modification.expected.expected.jinja
@@ -1,8 +1,8 @@
 C: ['a', 'b', 'c'].
-B: ['a', 'b', 'c', 'b'].
-A: ['a', 'b', 'c', 'b', 'a'].
+B: ['a', 'b', 'c', 'B'].
+A: ['a', 'b', 'c', 'B', 'A'].
 ---
 
 C: ['a', 'b', 'c'].
-B: ['a', 'b', 'c', 'b'].
-A: ['a', 'b', 'c', 'b', 'a'].
+B: ['a', 'b', 'c', 'B'].
+A: ['a', 'b', 'c', 'B', 'A'].
diff --git a/src/test/resources/eager/handles-higher-scope-reference-modification.expected.jinja b/src/test/resources/eager/handles-higher-scope-reference-modification.expected.jinja
@@ -1,10 +1,10 @@
 {% set b_list = ['a'] %}{% set a_list = ['a'] %}{% set b_list = a_list %}{% do b_list.append(deferred ? 'b' : '') %}
 {% set b_list = a_list %}{% set b_list = a_list %}{% macro c(c_list) %}{% do c_list.append(deferred ? 'c' : '') %}
 C: {{ c_list }}.{% endmacro %}{% set b_list = a_list %}{% set b_list = a_list %}{{ c(b_list) }}{% set b_list = a_list %}{% set b_list = a_list %}{% set b_list = a_list %}{% set b_list = a_list %}{% do b_list.append(deferred ? 'B' : '') %}
-B: {% set b_list = a_list %}{% set b_list = a_list %}{% set b_list = a_list %}{% set b_list = a_list %}{{ b_list }}.{% do a_list.append(deferred ? 'a' : '') %}
+B: {% set b_list = a_list %}{% set b_list = a_list %}{% set b_list = a_list %}{% set b_list = a_list %}{{ b_list }}.{% do a_list.append(deferred ? 'A' : '') %}
 A: {{ a_list }}.
 ---
 {% set a_list = ['a'] %}{% set b_list = a_list %}{% for i in [0] %}{% set b_list = a_list %}{% set b_list = a_list %}{% do b_list.append('b') %}{% for __ignored__ in [0] %}{% set c_list = b_list %}{% do c_list.append(deferred ? 'c' : '') %}
-C: {{ c_list }}.{% endfor %}{% set b_list = a_list %}{% do b_list.append(deferred ? 'b' : '') %}
-B: {% set b_list = a_list %}{{ b_list }}.{% endfor %}{% do a_list.append(deferred ? 'a' : '') %}
-A: {{ a_list }}.
+C: {{ c_list }}.{% endfor %}{% set b_list = a_list %}{% do b_list.append(deferred ? 'B' : '') %}
+B: {% set b_list = a_list %}{{ b_list }}.{% endfor %}{% do a_list.append(deferred ? 'A' : '') %}
+A: {{ a_list }}.
diff --git a/src/test/resources/eager/handles-higher-scope-reference-modification.jinja b/src/test/resources/eager/handles-higher-scope-reference-modification.jinja
@@ -11,7 +11,7 @@ C: {{ c_list }}.
 B: {{ b_list }}.
 {%- endmacro %}
 {{ b(a_list) }}
-{%- do a_list.append(deferred ? 'a' : '') %}
+{%- do a_list.append(deferred ? 'A' : '') %}
 A: {{ a_list }}.
 ---
 {% set a_list = [] %}
@@ -24,8 +24,8 @@ A: {{ a_list }}.
 {%- do c_list.append(deferred ? 'c' : '') %}
 C: {{ c_list }}.
 {%- endfor %}
-{%- do b_list.append(deferred ? 'b' : '') %}
+{%- do b_list.append(deferred ? 'B' : '') %}
 B: {{ b_list }}.
 {%- endfor %}
-{%- do a_list.append(deferred ? 'a' : '') %}
+{%- do a_list.append(deferred ? 'A' : '') %}
 A: {{ a_list }}.

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,6 @@ private String eagerResolveExpression(`
`44`	`44`	`.withPartialMacroEvaluation(`
`45`	`45`	`interpreter.getConfig().isNestedInterpretationEnabled()`
`46`	`46`	`)`
`47`		`- .withCheckForContextChanges(interpreter.getContext().isDeferredExecutionMode())`
`48`	`47`	`.build()`
`49`	`48`	`);`
`50`	`49`
Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,6 @@ public String eagerInterpret(`
`44`	`44`	`interpreter,`
`45`	`45`	`EagerChildContextConfig`
`46`	`46`	`.newBuilder()`
`47`		`- .withCheckForContextChanges(true)`
`48`	`47`	`.withForceDeferredExecutionMode(true)`
`49`	`48`	`.build()`
`50`	`49`	`)`
Original file line number	Diff line number	Diff line change
`@@ -114,7 +114,6 @@ public String eagerInterpret(`
`114`	`114`	`EagerChildContextConfig`
`115`	`115`	`.newBuilder()`
`116`	`116`	`.withForceDeferredExecutionMode(true)`
`117`		`- .withCheckForContextChanges(true)`
`118`	`117`	`.build()`
`119`	`118`	`)`
`120`	`119`	`.asTemplateString()`