From 05ed13c7c48eff78da02862594b7547dc9231a57 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 17:56:01 -0700 Subject: [PATCH 01/22] Add more test cases covering existing behaviors. --- sjsonnet/test/src/sjsonnet/EvaluatorTests.scala | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala index 1f4b757a..3860c9d4 100644 --- a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala +++ b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala @@ -145,6 +145,15 @@ object EvaluatorTests extends TestSuite { """{local y = $["2"], [x]: if x == "1" then y else 0, for x in ["1", "2"]}["1"]""", useNewEvaluator = useNewEvaluator ) ==> ujson.Num(0) + // References between locals in an object comprehension: + eval( + """{local a = 1, local b = a + 1, [k]: b + 1 for k in ["x"]}""", + useNewEvaluator = useNewEvaluator + ) ==> ujson.Obj("x" -> ujson.Num(3)) + // Locals which reference variables from the comprehension: + eval( + """{local x2 = k*2, [std.toString(k)]: x2 for k in [1]}""" + ) ==> ujson.Obj("1" -> ujson.Num(2)) } test("super") { test("implicit") { From 280b679a7948af5acd7d6185b350fa871013ab00 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 17:57:12 -0700 Subject: [PATCH 02/22] Add (failing) regression test for reported bug. --- .../test/src/sjsonnet/EvaluatorTests.scala | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala index 3860c9d4..33ebc280 100644 --- a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala +++ b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala @@ -154,6 +154,29 @@ object EvaluatorTests extends TestSuite { eval( """{local x2 = k*2, [std.toString(k)]: x2 for k in [1]}""" ) ==> ujson.Obj("1" -> ujson.Num(2)) + // Regression test for https://github.com/databricks/sjsonnet/issues/357 + // self references in object comprehension locals are properly rebound during inheritance: + eval( + """ + |local lib = { + | foo():: + | { + | local global = self, + | + | [iterParam]: global.base { + | foo: iterParam + | } + | for iterParam in ["foo"] + | }, + |}; + | + |{ + | base:: {} + |} + |+ lib.foo() + |""".stripMargin, + useNewEvaluator = useNewEvaluator + ) ==> ujson.Obj("foo" -> ujson.Obj("foo" -> "foo")) } test("super") { test("implicit") { From 2b76aeb6882beaaeec45969c51e5fd9314639f30 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 17:58:31 -0700 Subject: [PATCH 03/22] Fix reported bug. --- sjsonnet/src/sjsonnet/Evaluator.scala | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index e52232ae..56e0f6d0 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -845,20 +845,19 @@ class Evaluator( lazy val newSelf: Val.Obj = { val builder = new java.util.LinkedHashMap[String, Val.Obj.Member] for (s <- visitComp(e.first :: e.rest, Array(compScope))) { - lazy val newScope: ValScope = s.extend(newBindings, newSelf, null) - - lazy val newBindings = visitBindings(binds, (self, sup) => newScope) - visitExpr(e.key)(s) match { case Val.Str(_, k) => val prev_length = builder.size() builder.put( k, new Val.Obj.Member(e.plus, Visibility.Normal) { - def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = + def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = { + lazy val newScope: ValScope = s.extend(newBindings, self, null) + lazy val newBindings = visitBindings(binds, (self, sup) => newScope) visitExpr(e.value)( s.extend(newBindings, self, null) ) + } } ) if (prev_length == builder.size() && settings.noDuplicateKeysInComprehension) { From 64edef1023540b970867cdb13c2a7a3e620bd76d Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 18:02:39 -0700 Subject: [PATCH 04/22] Add (failing) regression test for related prexisting bug for super refs. --- sjsonnet/test/src/sjsonnet/EvaluatorTests.scala | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala index 33ebc280..8967ec3d 100644 --- a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala +++ b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala @@ -177,6 +177,22 @@ object EvaluatorTests extends TestSuite { |""".stripMargin, useNewEvaluator = useNewEvaluator ) ==> ujson.Obj("foo" -> ujson.Obj("foo" -> "foo")) + // Regression test for a related bug involving local references to `super`: + eval( + """ + |local lib = { + | foo():: { + | local sx = super.x, + | [k]: sx + 1 + | for k in ["x"] + | }, + |}; + | + |{ x: 2 } + |+ lib.foo() + |""".stripMargin, + useNewEvaluator = useNewEvaluator + ) ==> ujson.Obj("x" -> ujson.Num(3)) } test("super") { test("implicit") { From ee94db1101ec434676942bd9695ce3c2ec2fff93 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 18:02:54 -0700 Subject: [PATCH 05/22] Fix related bug for super refs. --- sjsonnet/src/sjsonnet/Evaluator.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 56e0f6d0..e7738be3 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -852,7 +852,7 @@ class Evaluator( k, new Val.Obj.Member(e.plus, Visibility.Normal) { def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = { - lazy val newScope: ValScope = s.extend(newBindings, self, null) + lazy val newScope: ValScope = s.extend(newBindings, self, sup) lazy val newBindings = visitBindings(binds, (self, sup) => newScope) visitExpr(e.value)( s.extend(newBindings, self, null) From 7a281cbdf8e6f9f42e17b5da5d6cc824245bd7a8 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 18:11:08 -0700 Subject: [PATCH 06/22] Add (failing) regression test for invalid obj comp key types. --- sjsonnet/test/src/sjsonnet/EvaluatorTests.scala | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala index 8967ec3d..4a815592 100644 --- a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala +++ b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala @@ -193,6 +193,10 @@ object EvaluatorTests extends TestSuite { |""".stripMargin, useNewEvaluator = useNewEvaluator ) ==> ujson.Obj("x" -> ujson.Num(3)) + // Regression test for a bug in handling of non-string field names: + evalErr("{[k]: k for k in [1]}", useNewEvaluator = useNewEvaluator) ==> + """sjsonnet.Error: Field name must be string or null, not number + |at .(:1:2)""".stripMargin } test("super") { test("implicit") { From 856c193d0560d4073958ecf3a1162d4bacc45a38 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 18:11:21 -0700 Subject: [PATCH 07/22] Error on invalid obj comp key types. --- sjsonnet/src/sjsonnet/Evaluator.scala | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index e7738be3..b68363df 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -864,7 +864,11 @@ class Evaluator( Error.fail(s"Duplicate key ${k} in evaluated object comprehension.", e.pos); } case Val.Null(_) => // do nothing - case _ => + case x => + Error.fail( + s"Field name must be string or null, not ${x.prettyName}", + e.pos + ) } } val valueCache = if (sup == null) { From 32393996f49d3294313dadab8f9dcc80c89837b9 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 18:12:53 -0700 Subject: [PATCH 08/22] Remove now-unused newSelf variable. --- sjsonnet/src/sjsonnet/Evaluator.scala | 65 +++++++++++++-------------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index b68363df..11bd6f15 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -841,45 +841,40 @@ class Evaluator( def visitObjComp(e: ObjBody.ObjComp, sup: Val.Obj)(implicit scope: ValScope): Val.Obj = { val binds = e.preLocals ++ e.postLocals val compScope: ValScope = scope // .clearSuper - - lazy val newSelf: Val.Obj = { - val builder = new java.util.LinkedHashMap[String, Val.Obj.Member] - for (s <- visitComp(e.first :: e.rest, Array(compScope))) { - visitExpr(e.key)(s) match { - case Val.Str(_, k) => - val prev_length = builder.size() - builder.put( - k, - new Val.Obj.Member(e.plus, Visibility.Normal) { - def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = { - lazy val newScope: ValScope = s.extend(newBindings, self, sup) - lazy val newBindings = visitBindings(binds, (self, sup) => newScope) - visitExpr(e.value)( - s.extend(newBindings, self, null) - ) - } + val builder = new java.util.LinkedHashMap[String, Val.Obj.Member] + for (s <- visitComp(e.first :: e.rest, Array(compScope))) { + visitExpr(e.key)(s) match { + case Val.Str(_, k) => + val prev_length = builder.size() + builder.put( + k, + new Val.Obj.Member(e.plus, Visibility.Normal) { + def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = { + lazy val newScope: ValScope = s.extend(newBindings, self, sup) + lazy val newBindings = visitBindings(binds, (self, sup) => newScope) + visitExpr(e.value)( + s.extend(newBindings, self, null) + ) } - ) - if (prev_length == builder.size() && settings.noDuplicateKeysInComprehension) { - Error.fail(s"Duplicate key ${k} in evaluated object comprehension.", e.pos); } - case Val.Null(_) => // do nothing - case x => - Error.fail( - s"Field name must be string or null, not ${x.prettyName}", - e.pos - ) - } - } - val valueCache = if (sup == null) { - Val.Obj.getEmptyValueCacheForObjWithoutSuper(builder.size()) - } else { - new java.util.HashMap[Any, Val]() + ) + if (prev_length == builder.size() && settings.noDuplicateKeysInComprehension) { + Error.fail(s"Duplicate key ${k} in evaluated object comprehension.", e.pos); + } + case Val.Null(_) => // do nothing + case x => + Error.fail( + s"Field name must be string or null, not ${x.prettyName}", + e.pos + ) } - new Val.Obj(e.pos, builder, false, null, sup, valueCache) } - - newSelf + val valueCache = if (sup == null) { + Val.Obj.getEmptyValueCacheForObjWithoutSuper(builder.size()) + } else { + new java.util.HashMap[Any, Val]() + } + new Val.Obj(e.pos, builder, false, null, sup, valueCache) } @tailrec From 1c51f116a8fad36bc04175d264d30178cd880868 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 19:03:50 -0700 Subject: [PATCH 09/22] Add missing useNewEvaluator in eval. --- sjsonnet/test/src/sjsonnet/EvaluatorTests.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala index 4a815592..403653dd 100644 --- a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala +++ b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala @@ -152,7 +152,8 @@ object EvaluatorTests extends TestSuite { ) ==> ujson.Obj("x" -> ujson.Num(3)) // Locals which reference variables from the comprehension: eval( - """{local x2 = k*2, [std.toString(k)]: x2 for k in [1]}""" + """{local x2 = k*2, [std.toString(k)]: x2 for k in [1]}""", + useNewEvaluator = useNewEvaluator ) ==> ujson.Obj("1" -> ujson.Num(2)) // Regression test for https://github.com/databricks/sjsonnet/issues/357 // self references in object comprehension locals are properly rebound during inheritance: From 55bcb2684a39a386981cb1f5ded712a92dcc3f82 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 19:12:09 -0700 Subject: [PATCH 10/22] Add regression test for another `super` bug. --- sjsonnet/test/src/sjsonnet/EvaluatorTests.scala | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala index 403653dd..6f8c851e 100644 --- a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala +++ b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala @@ -194,6 +194,21 @@ object EvaluatorTests extends TestSuite { |""".stripMargin, useNewEvaluator = useNewEvaluator ) ==> ujson.Obj("x" -> ujson.Num(3)) + // Yet another related bug involving super references _not_ in locals: + eval( + """ + |local lib = { + | foo():: { + | [k]: super.x + 1 + | for k in ["x"] + | }, + |}; + | + |{ x: 2 } + |+ lib.foo() + |""".stripMargin, + useNewEvaluator = useNewEvaluator + ) ==> ujson.Obj("x" -> ujson.Num(3)) // Regression test for a bug in handling of non-string field names: evalErr("{[k]: k for k in [1]}", useNewEvaluator = useNewEvaluator) ==> """sjsonnet.Error: Field name must be string or null, not number From fd038d84823f521c6213435730dcd373a7add26a Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 19:12:41 -0700 Subject: [PATCH 11/22] Fix super in value calculations. --- sjsonnet/src/sjsonnet/Evaluator.scala | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 11bd6f15..9e926c4c 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -852,9 +852,7 @@ class Evaluator( def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = { lazy val newScope: ValScope = s.extend(newBindings, self, sup) lazy val newBindings = visitBindings(binds, (self, sup) => newScope) - visitExpr(e.value)( - s.extend(newBindings, self, null) - ) + visitExpr(e.value)(newScope) } } ) From b74a493c31ef673974e3c88a01cd8e3ecb8d0bff Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 19:39:20 -0700 Subject: [PATCH 12/22] Simplify thunks / laziness in ValScope.extend() and Evaluator.visitBindings() --- sjsonnet/src/sjsonnet/Evaluator.scala | 12 +++++------- sjsonnet/src/sjsonnet/ValScope.scala | 14 +++----------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 9e926c4c..fadf73f5 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -699,18 +699,16 @@ class Evaluator( def visitBindings( bindings: Array[Bind], - scope: (Val.Obj, Val.Obj) => ValScope): Array[(Val.Obj, Val.Obj) => Lazy] = { - val arrF = new Array[(Val.Obj, Val.Obj) => Lazy](bindings.length) + scope: => ValScope): Array[Lazy] = { + val arrF = new Array[Lazy](bindings.length) var i = 0 while (i < bindings.length) { val b = bindings(i) arrF(i) = b.args match { case null => - (self: Val.Obj, sup: Val.Obj) => - new LazyWithComputeFunc(() => visitExpr(b.rhs)(scope(self, sup))) + new LazyWithComputeFunc(() => visitExpr(b.rhs)(scope)) case argSpec => - (self: Val.Obj, sup: Val.Obj) => - new LazyWithComputeFunc(() => visitMethod(b.rhs, argSpec, b.pos)(scope(self, sup))) + new LazyWithComputeFunc(() => visitMethod(b.rhs, argSpec, b.pos)(scope)) } i += 1 } @@ -851,7 +849,7 @@ class Evaluator( new Val.Obj.Member(e.plus, Visibility.Normal) { def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = { lazy val newScope: ValScope = s.extend(newBindings, self, sup) - lazy val newBindings = visitBindings(binds, (self, sup) => newScope) + lazy val newBindings = visitBindings(binds, newScope) visitExpr(e.value)(newScope) } } diff --git a/sjsonnet/src/sjsonnet/ValScope.scala b/sjsonnet/src/sjsonnet/ValScope.scala index a03ea433..eea15d50 100644 --- a/sjsonnet/src/sjsonnet/ValScope.scala +++ b/sjsonnet/src/sjsonnet/ValScope.scala @@ -19,22 +19,14 @@ final class ValScope private (val bindings: Array[Lazy]) extends AnyVal { def length: Int = bindings.length def extend( - newBindingsF: Array[(Val.Obj, Val.Obj) => Lazy] = null, + newBindings: Array[Lazy], newSelf: Val.Obj, newSuper: Val.Obj): ValScope = { - val by = if (newBindingsF == null) 2 else newBindingsF.length + 2 + val by = newBindings.length + 2 val b = Arrays.copyOf(bindings, bindings.length + by) b(bindings.length) = newSelf b(bindings.length + 1) = newSuper - if (newBindingsF != null) { - var i = 0 - var j = bindings.length + 2 - while (i < newBindingsF.length) { - b(j) = newBindingsF(i).apply(newSelf, newSuper) - i += 1 - j += 1 - } - } + System.arraycopy(newBindings, 0, b, bindings.length + 2, newBindings.length) new ValScope(b) } From fd3e9f402e23a9181c29c6f1bedc4e91c306f457 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 19:48:28 -0700 Subject: [PATCH 13/22] remove one unnecessary local --- sjsonnet/src/sjsonnet/ValScope.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sjsonnet/src/sjsonnet/ValScope.scala b/sjsonnet/src/sjsonnet/ValScope.scala index eea15d50..c3f59ded 100644 --- a/sjsonnet/src/sjsonnet/ValScope.scala +++ b/sjsonnet/src/sjsonnet/ValScope.scala @@ -22,8 +22,7 @@ final class ValScope private (val bindings: Array[Lazy]) extends AnyVal { newBindings: Array[Lazy], newSelf: Val.Obj, newSuper: Val.Obj): ValScope = { - val by = newBindings.length + 2 - val b = Arrays.copyOf(bindings, bindings.length + by) + val b = Arrays.copyOf(bindings, bindings.length + newBindings.length + 2) b(bindings.length) = newSelf b(bindings.length + 1) = newSuper System.arraycopy(newBindings, 0, b, bindings.length + 2, newBindings.length) From cb5593ec3d4fca209c0ca5a7d4f66a7294657250 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Mon, 5 May 2025 19:50:11 -0700 Subject: [PATCH 14/22] scalafmt fixes --- sjsonnet/src/sjsonnet/Evaluator.scala | 4 +--- sjsonnet/src/sjsonnet/ValScope.scala | 5 +---- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index fadf73f5..e6c7f4ad 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -697,9 +697,7 @@ class Evaluator( override def evalDefault(expr: Expr, vs: ValScope, es: EvalScope) = visitExpr(expr)(vs) } - def visitBindings( - bindings: Array[Bind], - scope: => ValScope): Array[Lazy] = { + def visitBindings(bindings: Array[Bind], scope: => ValScope): Array[Lazy] = { val arrF = new Array[Lazy](bindings.length) var i = 0 while (i < bindings.length) { diff --git a/sjsonnet/src/sjsonnet/ValScope.scala b/sjsonnet/src/sjsonnet/ValScope.scala index c3f59ded..a6452402 100644 --- a/sjsonnet/src/sjsonnet/ValScope.scala +++ b/sjsonnet/src/sjsonnet/ValScope.scala @@ -18,10 +18,7 @@ final class ValScope private (val bindings: Array[Lazy]) extends AnyVal { def length: Int = bindings.length - def extend( - newBindings: Array[Lazy], - newSelf: Val.Obj, - newSuper: Val.Obj): ValScope = { + def extend(newBindings: Array[Lazy], newSelf: Val.Obj, newSuper: Val.Obj): ValScope = { val b = Arrays.copyOf(bindings, bindings.length + newBindings.length + 2) b(bindings.length) = newSelf b(bindings.length + 1) = newSuper From bd4e1b2fb2efe578cf2b3277d4c5806e79cc0263 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 11:57:54 -0700 Subject: [PATCH 15/22] DRY up the error message. --- sjsonnet/src/sjsonnet/Evaluator.scala | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index e6c7f4ad..76703a8d 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -680,15 +680,15 @@ class Evaluator( visitExpr(k) match { case Val.Str(_, k1) => k1 case Val.Null(_) => null - case x => - Error.fail( - s"Field name must be string or null, not ${x.prettyName}", - pos - ) + case x => fieldNameTypeError(x, pos) } } } + private def fieldNameTypeError(fieldName: Val, pos: Position): Nothing = { + Error.fail(s"Field name must be string or null, not ${fieldName.prettyName}", pos) + } + def visitMethod(rhs: Expr, params: Params, outerPos: Position)(implicit scope: ValScope): Val.Func = new Val.Func(outerPos, scope, params) { @@ -856,11 +856,7 @@ class Evaluator( Error.fail(s"Duplicate key ${k} in evaluated object comprehension.", e.pos); } case Val.Null(_) => // do nothing - case x => - Error.fail( - s"Field name must be string or null, not ${x.prettyName}", - e.pos - ) + case x => fieldNameTypeError(x, e.pos) } } val valueCache = if (sup == null) { From f5e40c2b8e172825438e396f1cc5ee5577dc20ad Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 12:26:19 -0700 Subject: [PATCH 16/22] See if inlining method avoids over-sensitivity in stack overflow test. --- sjsonnet/src/sjsonnet/Evaluator.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 76703a8d..b1ed8a22 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -685,7 +685,7 @@ class Evaluator( } } - private def fieldNameTypeError(fieldName: Val, pos: Position): Nothing = { + @inline private def fieldNameTypeError(fieldName: Val, pos: Position): Nothing = { Error.fail(s"Field name must be string or null, not ${fieldName.prettyName}", pos) } From 4867920137b70a92b6366afe1edfdc308f8147c9 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 12:44:07 -0700 Subject: [PATCH 17/22] Revert "See if inlining method avoids over-sensitivity in stack overflow test." This reverts commit f5e40c2b8e172825438e396f1cc5ee5577dc20ad. --- sjsonnet/src/sjsonnet/Evaluator.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index b1ed8a22..76703a8d 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -685,7 +685,7 @@ class Evaluator( } } - @inline private def fieldNameTypeError(fieldName: Val, pos: Position): Nothing = { + private def fieldNameTypeError(fieldName: Val, pos: Position): Nothing = { Error.fail(s"Field name must be string or null, not ${fieldName.prettyName}", pos) } From 63cad73d91d80d1b94faf7cbe7d69a7b274ae5d3 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 12:44:15 -0700 Subject: [PATCH 18/22] Revert "DRY up the error message." This reverts commit bd4e1b2fb2efe578cf2b3277d4c5806e79cc0263. --- sjsonnet/src/sjsonnet/Evaluator.scala | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 76703a8d..e6c7f4ad 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -680,15 +680,15 @@ class Evaluator( visitExpr(k) match { case Val.Str(_, k1) => k1 case Val.Null(_) => null - case x => fieldNameTypeError(x, pos) + case x => + Error.fail( + s"Field name must be string or null, not ${x.prettyName}", + pos + ) } } } - private def fieldNameTypeError(fieldName: Val, pos: Position): Nothing = { - Error.fail(s"Field name must be string or null, not ${fieldName.prettyName}", pos) - } - def visitMethod(rhs: Expr, params: Params, outerPos: Position)(implicit scope: ValScope): Val.Func = new Val.Func(outerPos, scope, params) { @@ -856,7 +856,11 @@ class Evaluator( Error.fail(s"Duplicate key ${k} in evaluated object comprehension.", e.pos); } case Val.Null(_) => // do nothing - case x => fieldNameTypeError(x, e.pos) + case x => + Error.fail( + s"Field name must be string or null, not ${x.prettyName}", + e.pos + ) } } val valueCache = if (sup == null) { From 7df617e28fc9bb7e611045420ad54e615d748690 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 11:57:54 -0700 Subject: [PATCH 19/22] DRY up the error message. --- sjsonnet/src/sjsonnet/Evaluator.scala | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index e6c7f4ad..76703a8d 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -680,15 +680,15 @@ class Evaluator( visitExpr(k) match { case Val.Str(_, k1) => k1 case Val.Null(_) => null - case x => - Error.fail( - s"Field name must be string or null, not ${x.prettyName}", - pos - ) + case x => fieldNameTypeError(x, pos) } } } + private def fieldNameTypeError(fieldName: Val, pos: Position): Nothing = { + Error.fail(s"Field name must be string or null, not ${fieldName.prettyName}", pos) + } + def visitMethod(rhs: Expr, params: Params, outerPos: Position)(implicit scope: ValScope): Val.Func = new Val.Func(outerPos, scope, params) { @@ -856,11 +856,7 @@ class Evaluator( Error.fail(s"Duplicate key ${k} in evaluated object comprehension.", e.pos); } case Val.Null(_) => // do nothing - case x => - Error.fail( - s"Field name must be string or null, not ${x.prettyName}", - e.pos - ) + case x => fieldNameTypeError(x, e.pos) } } val valueCache = if (sup == null) { From e06e7a7fc286ae4b8b5590cd654715a5124f7364 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 13:38:30 -0700 Subject: [PATCH 20/22] Support function definitions via object comprehensions. --- sjsonnet/src/sjsonnet/Parser.scala | 14 ++++++++++- .../test/src/sjsonnet/EvaluatorTests.scala | 23 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Parser.scala b/sjsonnet/src/sjsonnet/Parser.scala index fcccbef5..58964b34 100644 --- a/sjsonnet/src/sjsonnet/Parser.scala +++ b/sjsonnet/src/sjsonnet/Parser.scala @@ -430,8 +430,20 @@ class Parser( val preLocals = exprs .takeWhile(_.isInstanceOf[Expr.Bind]) .map(_.asInstanceOf[Expr.Bind]) - val Expr.Member.Field(offset, Expr.FieldName.Dyn(lhs), plus, args, Visibility.Normal, rhs) = + val Expr.Member.Field( + offset, + Expr.FieldName.Dyn(lhs), + plus, + args, + Visibility.Normal, + rhsBody + ) = exprs(preLocals.length): @unchecked + val rhs = if (args == null) { + rhsBody + } else { + Expr.Function(offset, args, rhsBody) + } val postLocals = exprs .drop(preLocals.length + 1) .takeWhile(_.isInstanceOf[Expr.Bind]) diff --git a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala index 6f8c851e..f8470626 100644 --- a/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala +++ b/sjsonnet/test/src/sjsonnet/EvaluatorTests.scala @@ -213,6 +213,29 @@ object EvaluatorTests extends TestSuite { evalErr("{[k]: k for k in [1]}", useNewEvaluator = useNewEvaluator) ==> """sjsonnet.Error: Field name must be string or null, not number |at .(:1:2)""".stripMargin + // Basic function support: + eval( + """ + |local funcs = { + | [a](x): x * 2 + | for a in ["f1", "f2", "f3"] + |}; + |funcs.f1(10) + |""".stripMargin, + useNewEvaluator = useNewEvaluator + ) ==> ujson.Num(20) + // Functions which use locals from the comprehension: + eval( + """ + |local funcs = { + | local y = b, + | [a](x): x * y + | for a in ["f1", "f2", "f3"] for b in [2] + |}; + |funcs.f1(10) + |""".stripMargin, + useNewEvaluator = useNewEvaluator + ) ==> ujson.Num(20) } test("super") { test("implicit") { From 5cd4ed8dbd321561ce4583762f11cda407aa5c4f Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 14:02:38 -0700 Subject: [PATCH 21/22] Hack to address CI flakiness. --- .../test/src-jvm/sjsonnet/ErrorTestsJvmOnly.scala | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/sjsonnet/test/src-jvm/sjsonnet/ErrorTestsJvmOnly.scala b/sjsonnet/test/src-jvm/sjsonnet/ErrorTestsJvmOnly.scala index c4466ac7..3430f750 100644 --- a/sjsonnet/test/src-jvm/sjsonnet/ErrorTestsJvmOnly.scala +++ b/sjsonnet/test/src-jvm/sjsonnet/ErrorTestsJvmOnly.scala @@ -22,6 +22,17 @@ object ErrorTestsJvmOnly extends TestSuite { } val tests: Tests = Tests { + // Hack: this test suite may flakily fail if this suite runs prior to other error tests: + // if classloading or linking happens inside the StackOverflowError handling then that + // may will trigger a secondary StackOverflowError and cause the test to fail. + // As a temporary solution, we redundantly run one of the other error tests first. + // A better long term solution would be to change how we handle StackOverflowError + // to avoid this failure mode, but for now we add this hack to avoid CI flakiness: + test("02") - check( + """sjsonnet.Error: Foo. + | at [Error].(sjsonnet/test/resources/test_suite/error.02.jsonnet:17:1) + |""".stripMargin + ) test("array_recursive_manifest") - check( """sjsonnet.Error: Stackoverflow while materializing, possibly due to recursive value | at .(sjsonnet/test/resources/test_suite/error.array_recursive_manifest.jsonnet:17:12) From 0b2fce295d1357e5e871b6bd540163911f1e6d6a Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Wed, 7 May 2025 14:13:41 -0700 Subject: [PATCH 22/22] Add comment calling out circular reference --- sjsonnet/src/sjsonnet/Evaluator.scala | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 76703a8d..bfdc50f6 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -846,6 +846,9 @@ class Evaluator( k, new Val.Obj.Member(e.plus, Visibility.Normal) { def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = { + // There is a circular dependency between `newScope` and `newBindings` because + // bindings may refer to other bindings (e.g. chains of locals that build on + // each other): lazy val newScope: ValScope = s.extend(newBindings, self, sup) lazy val newBindings = visitBindings(binds, newScope) visitExpr(e.value)(newScope)