diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 12fe61df..4f2e3078 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -1502,14 +1502,19 @@ class Evaluator( } } - // And/Or rhs tail-position helpers — extracted to preserve @tailrec on visitExprWithTailCallSupport. - // TailCall sentinels pass through without a boolean type check: this is a deliberate semantic - // relaxation matching google/jsonnet behavior (where `&&` is simply `if a then b else false`). - // Direct non-boolean rhs values (e.g. `true && "hello"`) are still caught. + // And/Or rhs tail-position helpers - extracted to preserve @tailrec on + // visitExprWithTailCallSupport. TailCall sentinels keep the boolean type check as delayed + // continuation state so the trampoline can remain stack-safe while preserving normal &&/|| + // result validation. + // + // Do not resolve the TailCall here and then call .asBoolean. That would re-enter the trampoline + // from inside the current function body, restoring recursive stack growth for deep &&/|| chains + // and moving the lazy evaluation boundary. Instead, attach the check and let the outer + // TailCall.resolve loop apply it once the final value is produced. private def visitAndRhsTailPos(rhs: Expr, andPos: Position)(implicit scope: ValScope): Val = { visitExprWithTailCallSupport(rhs) match { case b: Val.Bool => b - case tc: TailCall => tc + case tc: TailCall => tc.withBooleanCheck(new TailCall.BooleanCheck("&&", andPos)) case unknown => Error.fail(s"binary operator && does not operate on ${unknown.prettyName}s.", andPos) } @@ -1518,7 +1523,7 @@ class Evaluator( private def visitOrRhsTailPos(rhs: Expr, orPos: Position)(implicit scope: ValScope): Val = { visitExprWithTailCallSupport(rhs) match { case b: Val.Bool => b - case tc: TailCall => tc + case tc: TailCall => tc.withBooleanCheck(new TailCall.BooleanCheck("||", orPos)) case unknown => Error.fail(s"binary operator || does not operate on ${unknown.prettyName}s.", orPos) } @@ -1530,8 +1535,8 @@ class Evaluator( * `TailCall.resolve` in `visitApply*` to iterate rather than grow the JVM stack. * * Potential tail positions are propagated through: IfElse (both branches), LocalExpr (returned), - * AssertExpr (returned), And (rhs), Or (rhs), and Expr.Error (value). All other expression types - * delegate to normal `visitExpr`. + * AssertExpr (returned), And (rhs), and Or (rhs). All other expression types delegate to normal + * `visitExpr`. */ @tailrec private def visitExprWithTailCallSupport(e: Expr)(implicit scope: ValScope): Val = e match { diff --git a/sjsonnet/src/sjsonnet/StaticOptimizer.scala b/sjsonnet/src/sjsonnet/StaticOptimizer.scala index b467a2a7..3c7c2fc3 100644 --- a/sjsonnet/src/sjsonnet/StaticOptimizer.scala +++ b/sjsonnet/src/sjsonnet/StaticOptimizer.scala @@ -658,7 +658,7 @@ class StaticOptimizer( // when lhs is true). We must still check rhs for non-recursive exits. hasNonRecursiveExit(e.rhs, selfName, selfIdx, paramCount) case _: Expr.Error => - // error value is the last thing evaluated before throwing → non-recursive exit + // error value is the last thing evaluated before throwing -> non-recursive exit true case e if isSelfTailCall(e) => false // this path IS a self-recursive tail call → not a non-recursive exit diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 0cf1efab..657da14a 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -2033,15 +2033,33 @@ final class TailCall( val args: Array[Eval], val namedNames: Array[String], val callSiteExpr: Expr, - val strict: Boolean = false) + val strict: Boolean = false, + private var booleanCheck: TailCall.BooleanCheck = null) extends Val { private[sjsonnet] def valTag: Byte = -1 var pos: Position = callSiteExpr.pos def prettyName = "tailcall" override def exprErrorString: String = callSiteExpr.exprErrorString + + def withBooleanCheck(check: TailCall.BooleanCheck): TailCall = { + if (booleanCheck eq null) booleanCheck = check + this + } + + private[sjsonnet] def pendingBooleanCheck: TailCall.BooleanCheck = booleanCheck } object TailCall { + // Delayed result validation for tail-position &&/||. It deliberately lives on the TailCall rather + // than resolving nested TailCalls inside Evaluator.visitAndRhsTailPos/visitOrRhsTailPos, because + // nested resolution would undermine stack-safety and could force work at a different lazy boundary. + final class BooleanCheck(val op: String, val pos: Position) { + def check(result: Val)(implicit ev: EvalErrorScope): Val = result match { + case b: Val.Bool => b + case unknown => + Error.fail(s"binary operator $op does not operate on ${unknown.prettyName}s.", pos) + } + } /** * Iteratively resolve a [[TailCall]] chain (trampoline loop). If `current` is not a TailCall, it @@ -2057,21 +2075,27 @@ object TailCall { * Error frames preserve the original call-site expression name (e.g. "Apply2") so that TCO does * not alter user-visible stack traces. */ + def resolve(current: Val)(implicit ev: EvalScope): Val = resolve(current, null) + @tailrec - def resolve(current: Val)(implicit ev: EvalScope): Val = current match { - case tc: TailCall => - val mode: TailstrictMode = - if (tc.strict) TailstrictModeEnabled else TailstrictModeAutoTCO - val next = - try { - tc.func.apply(tc.args, tc.namedNames, tc.callSiteExpr.pos)(ev, mode) - } catch { - case e: Error => - throw e.addFrame(tc.callSiteExpr.pos, tc.callSiteExpr) - } - resolve(next) - case result => result - } + private def resolve(current: Val, booleanCheck: BooleanCheck)(implicit ev: EvalScope): Val = + current match { + case tc: TailCall => + val mode: TailstrictMode = + if (tc.strict) TailstrictModeEnabled else TailstrictModeAutoTCO + val nextCheck = + if (tc.pendingBooleanCheck != null) tc.pendingBooleanCheck else booleanCheck + val next = + try { + tc.func.apply(tc.args, tc.namedNames, tc.callSiteExpr.pos)(ev, mode) + } catch { + case e: Error => + throw e.addFrame(tc.callSiteExpr.pos, tc.callSiteExpr) + } + resolve(next, nextCheck) + case result => + if (booleanCheck == null) result else booleanCheck.check(result) + } } /** diff --git a/sjsonnet/test/resources/new_test_suite/auto_tco_patterns.jsonnet b/sjsonnet/test/resources/new_test_suite/auto_tco_patterns.jsonnet index fb0513b8..5910651f 100644 --- a/sjsonnet/test/resources/new_test_suite/auto_tco_patterns.jsonnet +++ b/sjsonnet/test/resources/new_test_suite/auto_tco_patterns.jsonnet @@ -89,11 +89,11 @@ local f_four(a, b, c, d) = // This tests that hasNonRecursiveExit correctly identifies the outer if's base case // (not the &&/|| rhs which always recurses). local f_and_with_outer_base(n) = - if n <= 0 then 0 // outer base case + if n <= 0 then true // outer base case else (true && f_and_with_outer_base(n - 1)); // rhs always recurses, but outer if provides exit local f_or_with_outer_base(n) = - if n <= 0 then 0 + if n <= 0 then true else (false || f_or_with_outer_base(n - 1)); // Run tests @@ -117,7 +117,7 @@ std.assertEqual(f_deep_if(10000), 2) && std.assertEqual(f_zero(), 42) && std.assertEqual(f_four(10000, 0, 0, 0), 10000) && // Edge case: &&/|| with outer base case — auto-TCO should work because outer if provides exit -std.assertEqual(f_and_with_outer_base(10000), 0) && -std.assertEqual(f_or_with_outer_base(10000), 0) && +std.assertEqual(f_and_with_outer_base(10000), true) && +std.assertEqual(f_or_with_outer_base(10000), true) && true diff --git a/sjsonnet/test/resources/new_test_suite/error.auto_tco_and_tailcall_bool_check.jsonnet b/sjsonnet/test/resources/new_test_suite/error.auto_tco_and_tailcall_bool_check.jsonnet new file mode 100644 index 00000000..aa7608dc --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.auto_tco_and_tailcall_bool_check.jsonnet @@ -0,0 +1,6 @@ +// The rhs returns through a TailCall, so the boolean check must be delayed until +// the trampoline finishes. This should match direct `true && 0` behavior. +local f(n) = + if n <= 0 then 0 + else true && f(n - 1); +f(1000) diff --git a/sjsonnet/test/resources/new_test_suite/error.auto_tco_and_tailcall_bool_check.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/error.auto_tco_and_tailcall_bool_check.jsonnet.golden new file mode 100644 index 00000000..9e0cd92f --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.auto_tco_and_tailcall_bool_check.jsonnet.golden @@ -0,0 +1 @@ +sjsonnet.Error: binary operator && does not operate on numbers. diff --git a/sjsonnet/test/resources/new_test_suite/error.auto_tco_or_tailcall_bool_check.jsonnet b/sjsonnet/test/resources/new_test_suite/error.auto_tco_or_tailcall_bool_check.jsonnet new file mode 100644 index 00000000..70390b0f --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.auto_tco_or_tailcall_bool_check.jsonnet @@ -0,0 +1,6 @@ +// The rhs returns through a TailCall, so the boolean check must be delayed until +// the trampoline finishes. This should match direct `false || 0` behavior. +local f(n) = + if n <= 0 then 0 + else false || f(n - 1); +f(1000) diff --git a/sjsonnet/test/resources/new_test_suite/error.auto_tco_or_tailcall_bool_check.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/error.auto_tco_or_tailcall_bool_check.jsonnet.golden new file mode 100644 index 00000000..85d5e1ba --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/error.auto_tco_or_tailcall_bool_check.jsonnet.golden @@ -0,0 +1 @@ +sjsonnet.Error: binary operator || does not operate on numbers.