Revert "Preserve identifiers when reconstructing a tag which sets a value"

src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java

@@ -1,10 +1,6 @@
 package com.hubspot.jinjava.el.ext.eager;
 
 import com.hubspot.jinjava.el.ext.DeferredParsingException;
-import com.hubspot.jinjava.el.ext.ExtendedParser;
-import com.hubspot.jinjava.interpret.DeferredValueException;
-import com.hubspot.jinjava.lib.filter.Filter;
-import com.hubspot.jinjava.util.EagerExpressionResolver;
 import de.odysseus.el.tree.Bindings;
 import de.odysseus.el.tree.impl.ast.AstIdentifier;
 import javax.el.ELContext;
@@ -20,21 +16,7 @@ public EagerAstIdentifier(String name, int index, boolean ignoreReturnType) {
   @Override
   public Object eval(Bindings bindings, ELContext context) {
     return EvalResultHolder.super.eval(
-      () -> {
-        Object result = super.eval(bindings, context);
-        if (
-          !ExtendedParser.INTERPRETER.equals(getName()) &&
-          !EagerExpressionResolver.isPrimitive(result) &&
-          !(result instanceof Filter) &&
-          EvalResultHolder
-            .getJinjavaInterpreter(context)
-            .getContext()
-            .isPreserveAllIdentifiers()
-        ) {
-          throw new DeferredValueException("Preserving all non-primitive identifiers");
-        }
-        return result;
-      },
+      () -> super.eval(bindings, context),
       bindings,
       context
     );

src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java

@@ -44,7 +44,12 @@ default Object checkEvalResultSize(ELContext context) {
     if (
       evalResult instanceof Collection &&
       ((Collection<?>) evalResult).size() > 100 && // TODO make size configurable
-      getJinjavaInterpreter(context).getContext().isDeferLargeObjects()
+      (
+        (JinjavaInterpreter) context
+          .getELResolver()
+          .getValue(context, null, ExtendedParser.INTERPRETER)
+      ).getContext()
+        .isDeferLargeObjects()
     ) {
       throw new DeferredValueException("Collection too big");
     }
@@ -58,12 +63,6 @@ String getPartiallyResolved(
     boolean preserveIdentifier
   );
 
-  static JinjavaInterpreter getJinjavaInterpreter(ELContext context) {
-    return (JinjavaInterpreter) context
-      .getELResolver()
-      .getValue(context, null, ExtendedParser.INTERPRETER);
-  }
-
   static String reconstructNode(
     Bindings bindings,
     ELContext context,

src/main/java/com/hubspot/jinjava/interpret/Context.java

@@ -107,8 +107,6 @@ public enum Library {
   private boolean validationMode = false;
   private boolean deferredExecutionMode = false;
   private boolean deferLargeObjects = false;
-
-  private boolean preserveAllIdentifiers = false;
   private boolean throwInterpreterErrors = false;
   private boolean partialMacroEvaluation = false;
   private boolean unwrapRawOverride = false;
@@ -213,7 +211,6 @@ public Context(
       this.dynamicVariableResolver = parent.dynamicVariableResolver;
       this.deferredExecutionMode = parent.deferredExecutionMode;
       this.deferLargeObjects = parent.deferLargeObjects;
-      this.preserveAllIdentifiers = parent.preserveAllIdentifiers;
       this.throwInterpreterErrors = parent.throwInterpreterErrors;
     }
   }
@@ -732,26 +729,6 @@ public TemporaryValueClosable<Boolean> withDeferLargeObjects(
     return temporaryValueClosable;
   }
 
-  public boolean isPreserveAllIdentifiers() {
-    return preserveAllIdentifiers;
-  }
-
-  public Context setPreserveAllIdentifiers(boolean preserveAllIdentifiers) {
-    this.preserveAllIdentifiers = preserveAllIdentifiers;
-    return this;
-  }
-
-  public TemporaryValueClosable<Boolean> withPreserveAllIdentifiers(
-    boolean preserveAllIdentifiers
-  ) {
-    TemporaryValueClosable<Boolean> temporaryValueClosable = new TemporaryValueClosable<>(
-      this.preserveAllIdentifiers,
-      this::setPreserveAllIdentifiers
-    );
-    this.preserveAllIdentifiers = preserveAllIdentifiers;
-    return temporaryValueClosable;
-  }
-
   public boolean getThrowInterpreterErrors() {
     return throwInterpreterErrors;
   }

src/main/java/com/hubspot/jinjava/lib/fn/MacroFunction.java

@@ -77,6 +77,7 @@ public Object doEvaluate(
     JinjavaInterpreter interpreter = JinjavaInterpreter.getCurrent();
     Optional<String> importFile = getImportFile(interpreter);
     try (InterpreterScopeClosable c = interpreter.enterScope()) {
+      interpreter.getContext().setDeferredExecutionMode(false);
       String result = getEvaluationResult(argMap, kwargMap, varArgs, interpreter);
 
       if (

src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerBlockSetTagStrategy.java

@@ -55,16 +55,6 @@ protected EagerExecutionResult getEagerExecutionResult(
     return result;
   }
 
-  @Override
-  protected EagerExecutionResult getDeferredEagerExecutionResult(
-    TagNode tagNode,
-    String expression,
-    JinjavaInterpreter interpreter,
-    EagerExecutionResult firstResult
-  ) {
-    return firstResult;
-  }
-
   @Override
   protected Optional<String> resolveSet(
     TagNode tagNode,

src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerExecutionResult.java

@@ -55,6 +55,20 @@ public String getPrefixToPreserveState(boolean registerDeferredToken) {
     }
     JinjavaInterpreter interpreter = JinjavaInterpreter.getCurrent();
     prefixToPreserveState =
+      speculativeBindings
+        .entrySet()
+        .stream()
+        .filter(entry -> entry.getValue() instanceof PyishBlockSetSerializable)
+        .map(
+          entry ->
+            buildBlockSetTag(
+              entry.getKey(),
+              ((PyishBlockSetSerializable) entry.getValue()).getBlockSetBody(),
+              interpreter,
+              registerDeferredToken
+            )
+        )
+        .collect(Collectors.joining()) +
       buildSetTag(
         speculativeBindings
           .entrySet()
@@ -70,20 +84,6 @@ public String getPrefixToPreserveState(boolean registerDeferredToken) {
         interpreter,
         registerDeferredToken
       ) +
-      speculativeBindings
-        .entrySet()
-        .stream()
-        .filter(entry -> entry.getValue() instanceof PyishBlockSetSerializable)
-        .map(
-          entry ->
-            buildBlockSetTag(
-              entry.getKey(),
-              ((PyishBlockSetSerializable) entry.getValue()).getBlockSetBody(),
-              interpreter,
-              registerDeferredToken
-            )
-        )
-        .collect(Collectors.joining()) +
       speculativeBindings
         .entrySet()
         .stream()

src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java

@@ -58,7 +58,7 @@ public String innerInterpret(TagNode tagNode, JinjavaInterpreter interpreter) {
         !result.getSpeculativeBindings().isEmpty()
       )
     ) {
-      EagerReconstructionUtils.resetSpeculativeBindings(interpreter, result);
+      EagerIfTag.resetBindingsForNextBranch(interpreter, result);
       interpreter.getContext().removeDeferredTokens(addedTokens);
       throw new DeferredValueException(
         result.getResult().getResolutionState() == ResolutionState.NONE
@@ -98,34 +98,28 @@ public String eagerInterpret(
           interpreter.getContext().isDeferLargeObjects()
         )
     ) {
-      try (
-        TemporaryValueClosable<Boolean> c1 = interpreter
-          .getContext()
-          .withPreserveAllIdentifiers(true)
-      ) {
-        // separate getEagerImage from renderChildren because the token gets evaluated once
-        // while the children are evaluated 0...n times.
-        result.append(
-          EagerContextWatcher
-            .executeInChildContext(
-              eagerInterpreter ->
-                EagerExpressionResult.fromString(
-                  getEagerImage(
-                    buildToken(
-                      tagNode,
-                      e,
-                      interpreter.getLineNumber(),
-                      interpreter.getPosition()
-                    ),
-                    eagerInterpreter
-                  )
-                ),
-              interpreter,
-              EagerContextWatcher.EagerChildContextConfig.newBuilder().build()
-            )
-            .asTemplateString()
-        );
-      }
+      // separate getEagerImage from renderChildren because the token gets evaluated once
+      // while the children are evaluated 0...n times.
+      result.append(
+        EagerContextWatcher
+          .executeInChildContext(
+            eagerInterpreter ->
+              EagerExpressionResult.fromString(
+                getEagerImage(
+                  buildToken(
+                    tagNode,
+                    e,
+                    interpreter.getLineNumber(),
+                    interpreter.getPosition()
+                  ),
+                  eagerInterpreter
+                )
+              ),
+            interpreter,
+            EagerContextWatcher.EagerChildContextConfig.newBuilder().build()
+          )
+          .asTemplateString()
+      );
     }
 
     EagerExecutionResult firstRunResult = runLoopOnce(tagNode, interpreter);

src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerIfTag.java

@@ -121,9 +121,7 @@ public String eagerRenderBranches(
             .build()
         );
         sb.append(result.getResult());
-        bindingsToDefer.addAll(
-          EagerReconstructionUtils.resetSpeculativeBindings(interpreter, result)
-        );
+        bindingsToDefer.addAll(resetBindingsForNextBranch(interpreter, result));
       }
       if (branchEnd >= childrenSize || definitelyExecuted) {
         break;
@@ -182,6 +180,16 @@ public String eagerRenderBranches(
     return sb.toString();
   }
 
+  public static Set<String> resetBindingsForNextBranch(
+    JinjavaInterpreter interpreter,
+    EagerExecutionResult result
+  ) {
+    result
+      .getSpeculativeBindings()
+      .forEach((k, v) -> interpreter.getContext().replace(k, v));
+    return result.getSpeculativeBindings().keySet();
+  }
+
   private String evaluateBranch(
     TagNode tagNode,
     int startIdx,

src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerInlineSetTagStrategy.java

@@ -1,6 +1,5 @@
 package com.hubspot.jinjava.lib.tag.eager;
 
-import com.hubspot.jinjava.interpret.Context.TemporaryValueClosable;
 import com.hubspot.jinjava.interpret.DeferredMacroValueImpl;
 import com.hubspot.jinjava.interpret.DeferredValueException;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
@@ -9,7 +8,6 @@
 import com.hubspot.jinjava.tree.parse.TagToken;
 import com.hubspot.jinjava.util.EagerContextWatcher;
 import com.hubspot.jinjava.util.EagerExpressionResolver;
-import com.hubspot.jinjava.util.EagerExpressionResolver.EagerExpressionResult;
 import com.hubspot.jinjava.util.EagerReconstructionUtils;
 import com.hubspot.jinjava.util.LengthLimitingStringJoiner;
 import com.hubspot.jinjava.util.WhitespaceUtils;
@@ -34,7 +32,8 @@ public EagerExecutionResult getEagerExecutionResult(
     JinjavaInterpreter interpreter
   ) {
     return EagerContextWatcher.executeInChildContext(
-      eagerInterpreter -> getEagerExpressionResult(expression, interpreter),
+      eagerInterpreter ->
+        EagerExpressionResolver.resolveExpression('[' + expression + ']', interpreter),
       interpreter,
       EagerContextWatcher
         .EagerChildContextConfig.newBuilder()
@@ -43,39 +42,6 @@ public EagerExecutionResult getEagerExecutionResult(
     );
   }
 
-  @Override
-  protected EagerExecutionResult getDeferredEagerExecutionResult(
-    TagNode tagNode,
-    String expression,
-    JinjavaInterpreter interpreter,
-    EagerExecutionResult firstResult
-  ) {
-    EagerReconstructionUtils.resetSpeculativeBindings(interpreter, firstResult);
-    // Preserve identifiers when reconstructing to maintain proper object references
-    try (
-      TemporaryValueClosable<Boolean> c = interpreter
-        .getContext()
-        .withPreserveAllIdentifiers(true)
-    ) {
-      return EagerContextWatcher.executeInChildContext(
-        eagerInterpreter -> getEagerExpressionResult(expression, interpreter),
-        interpreter,
-        EagerContextWatcher
-          .EagerChildContextConfig.newBuilder()
-          .withTakeNewValue(true)
-          .withForceDeferredExecutionMode(true)
-          .build()
-      );
-    }
-  }
-
-  private static EagerExpressionResult getEagerExpressionResult(
-    String expression,
-    JinjavaInterpreter interpreter
-  ) {
-    return EagerExpressionResolver.resolveExpression('[' + expression + ']', interpreter);
-  }
-
   @Override
   public Optional<String> resolveSet(
     TagNode tagNode,

src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagStrategy.java

@@ -61,13 +61,6 @@ public String run(TagNode tagNode, JinjavaInterpreter interpreter) {
         return maybeResolved.get();
       }
     }
-    eagerExecutionResult =
-      getDeferredEagerExecutionResult(
-        tagNode,
-        expression,
-        interpreter,
-        eagerExecutionResult
-      );
     Triple<String, String, String> triple = getPrefixTokenAndSuffix(
       tagNode,
       variables,
@@ -89,13 +82,6 @@ protected abstract EagerExecutionResult getEagerExecutionResult(
     JinjavaInterpreter interpreter
   );
 
-  protected abstract EagerExecutionResult getDeferredEagerExecutionResult(
-    TagNode tagNode,
-    String expression,
-    JinjavaInterpreter interpreter,
-    EagerExecutionResult firstResult
-  );
-
   protected abstract Optional<String> resolveSet(
     TagNode tagNode,
     String[] variables,
@@ -131,10 +117,7 @@ protected String getPrefixToPreserveState(
     JinjavaInterpreter interpreter
   ) {
     StringBuilder prefixToPreserveState = new StringBuilder();
-    if (
-      interpreter.getContext().isDeferredExecutionMode() ||
-      !eagerExecutionResult.getResult().isFullyResolved()
-    ) {
+    if (interpreter.getContext().isDeferredExecutionMode()) {
       prefixToPreserveState.append(eagerExecutionResult.getPrefixToPreserveState());
     } else {
       interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());

src/main/java/com/hubspot/jinjava/util/EagerReconstructionUtils.java

@@ -653,14 +653,4 @@ public static String reconstructDeferredReferences(
       )
     );
   }
-
-  public static Set<String> resetSpeculativeBindings(
-    JinjavaInterpreter interpreter,
-    EagerExecutionResult result
-  ) {
-    result
-      .getSpeculativeBindings()
-      .forEach((k, v) -> interpreter.getContext().replace(k, v));
-    return result.getSpeculativeBindings().keySet();
-  }
 }