From c2867da98ad194706c026136a42e89db4522e4b6 Mon Sep 17 00:00:00 2001 From: He-Pin Date: Mon, 27 Apr 2026 04:10:52 +0800 Subject: [PATCH] fix: Align clamp and splitLimit edge semantics Motivation: Jsonnet 0.22.0 accepts comparable non-number values in std.clamp and treats negative std.splitLimit/std.splitLimitR maxsplits as unlimited. sjsonnet rejected both cases, which left stdlib behavior out of sync with the official implementation. Modification: Implement std.clamp with Jsonnet-compatible lazy comparison over numbers, strings, and arrays, including official-style array comparison failures for non-comparable element types. Allow all negative splitLimit maxsplits values to use the unlimited split path and update direction/golden tests. Result: std.clamp and splitLimit edge cases now match Jsonnet 0.22.0 for the covered semantics. std.reverse is intentionally unchanged in this PR. References: #802, #803 --- sjsonnet/src/sjsonnet/stdlib/MathModule.scala | 169 +++++++++++++++++- .../src/sjsonnet/stdlib/StringModule.scala | 5 +- .../builtinSplitLimitR5.jsonnet.golden | 9 +- .../stdlib_followup_official_edges.jsonnet | 14 ++ ...lib_followup_official_edges.jsonnet.golden | 38 ++++ .../src/sjsonnet/Std0150FunctionsTests.scala | 21 +++ 6 files changed, 245 insertions(+), 11 deletions(-) create mode 100644 sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet create mode 100644 sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet.golden diff --git a/sjsonnet/src/sjsonnet/stdlib/MathModule.scala b/sjsonnet/src/sjsonnet/stdlib/MathModule.scala index 9c71d6126..221bbc5c7 100644 --- a/sjsonnet/src/sjsonnet/stdlib/MathModule.scala +++ b/sjsonnet/src/sjsonnet/stdlib/MathModule.scala @@ -6,6 +6,170 @@ import sjsonnet.functions.AbstractFunctionModule object MathModule extends AbstractFunctionModule { def name = "math" + private object Clamp extends Val.Builtin3("clamp", "x", "minVal", "maxVal") { + private def applyClamp( + x: Eval, + minVal: Eval, + maxVal: Eval, + ev: EvalScope, + pos: Position): Val = { + val xValue = x.value + val minValue = minVal.value + if (compareForClamp("<", xValue, minValue, pos)(ev) < 0) { + minValue + } else { + val maxValue = maxVal.value + if (compareForClamp(">", xValue, maxValue, pos)(ev) > 0) maxValue else xValue + } + } + + def evalRhs(x: Eval, minVal: Eval, maxVal: Eval, ev: EvalScope, pos: Position): Val = + applyClamp(x, minVal, maxVal, ev, pos) + + override def apply3(x: Eval, minVal: Eval, maxVal: Eval, outerPos: Position)(implicit + ev: EvalScope, + tailstrictMode: TailstrictMode): Val = { + if (tailstrictMode == TailstrictModeEnabled) { + x.value + minVal.value + maxVal.value + } + applyClamp(x, minVal, maxVal, ev, outerPos) + } + + override def apply(argsL: Array[? <: Eval], namedNames: Array[String], outerPos: Position)( + implicit + ev: EvalScope, + tailstrictMode: TailstrictMode): Val = { + val args = new Array[Eval](3) + val positionalArgCount = + if (namedNames == null) argsL.length else argsL.length - namedNames.length + + if (argsL.length > 3) Error.fail("Too many args, has 3 parameter(s)", outerPos) + + var i = 0 + while (i < positionalArgCount) { + args(i) = argsL(i) + i += 1 + } + + if (namedNames != null) { + var namedIndex = 0 + var argIndex = positionalArgCount + while (namedIndex < namedNames.length) { + val paramIndex = namedNames(namedIndex) match { + case "x" => 0 + case "minVal" => 1 + case "maxVal" => 2 + case name => Error.fail(s"has no parameter $name", outerPos) + } + if (args(paramIndex) != null) { + Error.fail( + s"binding parameter a second time: ${namedNames(namedIndex)} in function clamp", + outerPos + ) + } + args(paramIndex) = argsL(argIndex) + namedIndex += 1 + argIndex += 1 + } + } + + var missing: String = null + var missingCount = 0 + i = 0 + while (i < args.length) { + if (args(i) == null) { + if (missing == null) missing = params.names(i) else missing += ", " + params.names(i) + missingCount += 1 + } + i += 1 + } + if (missingCount > 0) { + val plural = if (missingCount > 1) "s" else "" + Error.fail(s"parameter$plural $missing not bound in call", outerPos) + } + + if (tailstrictMode == TailstrictModeEnabled) args.foreach(_.value) + applyClamp(args(0), args(1), args(2), ev, outerPos) + } + } + + private def compareForClamp(op: String, left: Val, right: Val, pos: Position)(implicit + ev: EvalScope): Int = { + (left, right) match { + case (l: Val.Num, r: Val.Num) => compareNumbers(l.asDouble, r.asDouble) + case (l: Val.Str, r: Val.Str) => Util.compareStringsByCodepoint(l.str, r.str) + case (l: Val.Arr, r: Val.Arr) => compareArraysForClamp(l, r, pos) + case (_: Val.Bool, _: Val.Bool) => + Error.fail(s"binary operator $op does not operate on booleans.", pos) + case (_: Val.Null, _: Val.Null) => + Error.fail(s"binary operator $op does not operate on null.", pos) + case (_: Val.Obj, _: Val.Obj) => + Error.fail(s"binary operator $op does not operate on objects.", pos) + case (_: Val.Func, _: Val.Func) => + Error.fail(s"binary operator $op does not operate on functions.", pos) + case _ => + Error.fail( + s"binary operator $op requires matching types, got ${left.prettyName} and ${right.prettyName}.", + pos + ) + } + } + + private def compareArrayValuesForClamp(left: Val, right: Val, pos: Position)(implicit + ev: EvalScope): Int = { + (left, right) match { + case (l: Val.Num, r: Val.Num) => compareNumbers(l.asDouble, r.asDouble) + case (l: Val.Str, r: Val.Str) => Util.compareStringsByCodepoint(l.str, r.str) + case (l: Val.Arr, r: Val.Arr) => compareArraysForClamp(l, r, pos) + case (_: Val.Null, _: Val.Null) => + Error.fail("binary operator < does not operate on null.", pos) + case (_: Val.Bool, _: Val.Bool) => + Error.fail("Values of type boolean are not comparable.", pos) + case (_: Val.Obj, _: Val.Obj) => + Error.fail("Values of type object are not comparable.", pos) + case (_: Val.Func, _: Val.Func) => + Error.fail("Values of type function are not comparable.", pos) + case _ => + Error.fail( + s"Comparison requires matching types. Got ${left.prettyName} and ${right.prettyName}", + pos + ) + } + } + + private def compareArraysForClamp(left: Val.Arr, right: Val.Arr, pos: Position)(implicit + ev: EvalScope): Int = { + val leftLength = left.length + val rightLength = right.length + val length = math.min(leftLength, rightLength) + var i = 0 + while (i < length) { + val leftEval = left.eval(i) + val rightEval = right.eval(i) + val comparison = + if ( + (leftEval eq rightEval) && + leftEval.isInstanceOf[Val] && + isPrimitiveComparable(leftEval.asInstanceOf[Val]) + ) { + 0 + } else { + compareArrayValuesForClamp(leftEval.value, rightEval.value, pos) + } + if (comparison != 0) return comparison + i += 1 + } + Integer.compare(leftLength, rightLength) + } + + @inline private def compareNumbers(left: Double, right: Double): Int = + if (left < right) -1 else if (left > right) 1 else 0 + + @inline private def isPrimitiveComparable(value: Val): Boolean = + value.isInstanceOf[Val.Num] || value.isInstanceOf[Val.Str] + val functions: Seq[(String, Val.Func)] = Seq( builtin("sqrt", "x") { (pos, ev, x: Double) => math.sqrt(x) @@ -30,10 +194,7 @@ object MathModule extends AbstractFunctionModule { builtin("modulo", "a", "b") { (pos, ev, a: Double, b: Double) => a % b }, - builtin("clamp", "x", "minVal", "maxVal") { - (pos, ev, x: Double, minVal: Double, maxVal: Double) => - math.max(minVal, math.min(x, maxVal)) - }, + builtin(Clamp), builtin("pow", "x", "n") { (pos, ev, x: Double, n: Double) => math.pow(x, n) }, diff --git a/sjsonnet/src/sjsonnet/stdlib/StringModule.scala b/sjsonnet/src/sjsonnet/stdlib/StringModule.scala index f4a7c9762..b2e7fc9fd 100644 --- a/sjsonnet/src/sjsonnet/stdlib/StringModule.scala +++ b/sjsonnet/src/sjsonnet/stdlib/StringModule.scala @@ -303,15 +303,12 @@ object StringModule extends AbstractFunctionModule { } private def splitLimit(pos: Position, str: String, cStr: String, maxSplits: Int): Array[Eval] = { - if (maxSplits < 0 && maxSplits != -1) { - Error.fail("maxSplits should be -1 or non-negative, got " + maxSplits) - } if (cStr.isEmpty) { Error.fail("Cannot split by an empty string") } val b = new mutable.ArrayBuilder.ofRef[Eval] - b.sizeHint(maxSplits) + if (maxSplits >= 0 && maxSplits < Int.MaxValue) b.sizeHint(maxSplits + 1) var sz = 0 var i = 0 var start = 0 diff --git a/sjsonnet/test/resources/go_test_suite/builtinSplitLimitR5.jsonnet.golden b/sjsonnet/test/resources/go_test_suite/builtinSplitLimitR5.jsonnet.golden index 4e72a3d78..054ad80ff 100644 --- a/sjsonnet/test/resources/go_test_suite/builtinSplitLimitR5.jsonnet.golden +++ b/sjsonnet/test/resources/go_test_suite/builtinSplitLimitR5.jsonnet.golden @@ -1,3 +1,6 @@ -sjsonnet.Error: [std.splitLimitR] maxSplits should be -1 or non-negative, got -2 - at [].(builtinSplitLimitR5.jsonnet:1:16) - +[ + "foo", + "bar", + "baz", + "qux" +] diff --git a/sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet b/sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet new file mode 100644 index 000000000..789328a8a --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet @@ -0,0 +1,14 @@ +{ + clampStringMiddle: std.clamp("b", "a", "c"), + clampStringLow: std.clamp("a", "b", "c"), + clampStringHigh: std.clamp("d", "b", "c"), + clampArrayMiddle: std.clamp([2], [1], [3]), + clampArrayLow: std.clamp([0], [1], [3]), + clampArrayHigh: std.clamp([4], [1], [3]), + clampLazyMax: std.clamp(0, 1, error "maxVal forced"), + splitLimitNegative: std.splitLimit("a,b,c", ",", -2), + splitLimitFractionalNegative: std.splitLimit("a,b,c", ",", -2.5), + splitLimitRFractionalNegative: std.splitLimitR("a,b,c", ",", -2.5), + splitLimitFractionalPositive: std.splitLimit("a,b,c", ",", 1.5), + splitLimitRFractionalPositive: std.splitLimitR("a,b,c", ",", 1.5), +} diff --git a/sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet.golden new file mode 100644 index 000000000..cda422449 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/stdlib_followup_official_edges.jsonnet.golden @@ -0,0 +1,38 @@ +{ + "clampArrayHigh": [ + 3 + ], + "clampArrayLow": [ + 1 + ], + "clampArrayMiddle": [ + 2 + ], + "clampLazyMax": 1, + "clampStringHigh": "c", + "clampStringLow": "b", + "clampStringMiddle": "b", + "splitLimitFractionalNegative": [ + "a", + "b", + "c" + ], + "splitLimitFractionalPositive": [ + "a", + "b,c" + ], + "splitLimitNegative": [ + "a", + "b", + "c" + ], + "splitLimitRFractionalNegative": [ + "a", + "b", + "c" + ], + "splitLimitRFractionalPositive": [ + "a,b", + "c" + ] +} diff --git a/sjsonnet/test/src/sjsonnet/Std0150FunctionsTests.scala b/sjsonnet/test/src/sjsonnet/Std0150FunctionsTests.scala index ab906b7db..d005f557d 100644 --- a/sjsonnet/test/src/sjsonnet/Std0150FunctionsTests.scala +++ b/sjsonnet/test/src/sjsonnet/Std0150FunctionsTests.scala @@ -9,6 +9,18 @@ object Std0150FunctionsTests extends TestSuite { eval("std.clamp(-3, 0, 5)") ==> ujson.Num(0) eval("std.clamp(4, 0, 5)") ==> ujson.Num(4) eval("std.clamp(7, 0, 5)") ==> ujson.Num(5) + eval("""std.clamp("b", "a", "c")""") ==> ujson.Str("b") + eval("""std.clamp("a", "b", "c")""") ==> ujson.Str("b") + eval("""std.clamp("d", "b", "c")""") ==> ujson.Str("c") + eval("""std.clamp([2], [1], [3])""") ==> ujson.Arr(2) + eval("""std.clamp([0], [1], [3])""") ==> ujson.Arr(1) + eval("""std.clamp([4], [1], [3])""") ==> ujson.Arr(3) + eval("""std.clamp(0, 1, error "maxVal forced")""") ==> ujson.Num(1) + eval("""std.clamp(x=0, minVal=1, maxVal=error "maxVal forced")""") ==> ujson.Num(1) + assert(evalErr("""std.clamp(true, false, true)""").contains("booleans")) + assert(evalErr("""std.clamp(1, "a", 2)""").contains("requires matching types")) + assert(evalErr("""std.clamp([true], [true], [true])""").contains("boolean")) + assert(evalErr("""local p = [true]; std.clamp(p + [1], p + [0], p + [2])""").contains("boolean")) } test("member") { eval("std.member('foo', 'o')") ==> ujson.True @@ -48,6 +60,15 @@ object Std0150FunctionsTests extends TestSuite { eval("""std.slice("jsonnet", 0, 4, 1)""") ==> ujson.Str("json") } + test("splitLimit negative maxsplits") { + eval("""std.splitLimit("a,b,c", ",", -2)""") ==> ujson.Arr("a", "b", "c") + eval("""std.splitLimit("a,b,c", ",", -2.5)""") ==> ujson.Arr("a", "b", "c") + eval("""std.splitLimitR("a,b,c", ",", -2)""") ==> ujson.Arr("a", "b", "c") + eval("""std.splitLimitR("a,b,c", ",", -2.5)""") ==> ujson.Arr("a", "b", "c") + eval("""std.splitLimit("a,b,c", ",", 1.5)""") ==> ujson.Arr("a", "b,c") + eval("""std.splitLimitR("a,b,c", ",", 1.5)""") ==> ujson.Arr("a,b", "c") + } + test("manifestJsonMinified") { eval( """std.manifestJsonMinified( { x: [1, 2, 3, true, false, null, "string\nstring", []], y: { a: 1, b: 2, c: [1, 2], d: {} }, })"""