Make std.get handle default lazily

[stephenamar-db]

Basically,

std.get({a: 1}, "a", default=error "a")

should not execute default unless needed.
In the current implementation, builtins evaluates all arguments by default. This change changes that. Now Builtins have to force arguments when they need it.

[JoshRosen]

Bug in nearby code below the fold:

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 927 to 959 in 877f5aa

	if (!rValue.isInstanceOf[Val.Null]) { // if we are not removing the key
	if (lValue != null && rValue == null) {
	// Preserve the LHS/target value:
	kvs(kvsIdx) = (key, new Val.Obj.ConstMember(false, Visibility.Normal, lValue))
	} else if (lValue.isInstanceOf[Val.Obj] && rValue.isInstanceOf[Val.Obj]) {
	// Recursively merge objects:
	kvs(kvsIdx) = (key, createLazyMember(recPair(lValue, rValue)))
	} else if (rValue != null) {
	// Use the RHS/patch value and recursively remove Null or hidden fields:
	kvs(kvsIdx) = (key, createLazyMember(recSingle(rValue)))
	} else {
	Error.fail("std.mergePatch: This should never happen")
	}
	kvsIdx += 1
	}
	i += 1
	}
	val trimmedKvs = if (kvsIdx == i) kvs else kvs.slice(0, kvsIdx)
	Val.Obj.mk(mergePosition, trimmedKvs)
	case (_, _) => recSingle(r)
	}
	def recSingle(v: Val): Val = v match{
	case obj: Val.Obj =>
	val keys: Array[String] = obj.visibleKeyNames
	val kvs: Array[(String, Val.Obj.Member)] = new Array[(String, Val.Obj.Member)](keys.length)
	var kvsIdx = 0
	var i = 0
	while (i < keys.length) {
	val key = keys(i)
	val value = obj.value(key, pos, obj)(ev)
	if (!value.isInstanceOf[Val.Null]) {

In mergePatch I think we want to do force on the members before the isInstanceOf checks, e.g. force after we call .valueRaw? Unless there's some invariant whereby we can never get lazy values there.

[JoshRosen]

I think we also need this in avg and sum:

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 1530 to 1544 in 877f5aa

	builtin("sum", "arr") { (_, _, arr: Val.Arr) =>
	if (!arr.forall(_.isInstanceOf[Val.Num])) {
	Error.fail("Argument must be an array of numbers")
	}
	arr.asLazyArray.map(_.force.asDouble).sum
	},
	builtin("avg", "arr") { (_, _, arr: Val.Arr) =>
	if (!arr.forall(_.isInstanceOf[Val.Num])) {
	Error.fail("Argument must be an array of numbers")
	}
	if (arr.length == 0) {
	Error.fail("Cannot calculate average of an empty array")
	}
	arr.asLazyArray.map(_.force.asDouble).sum/arr.length
	},

[JoshRosen]

Probably not required in

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 1588 to 1590 in 877f5aa

	builtin("primitiveEquals", "x", "y") { (_, ev, x: Val, y: Val) =>
	x.isInstanceOf[y.type] && ev.compare(x, y) == 0
	},

in primitiveEquals as long as the shorthand builtin continues to force its args.

[JoshRosen]

Perhaps required in uniqArr after function application? It looks like ev.equal doesn't force its arguments.

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 1742 to 1767 in 877f5aa

	private def uniqArr(pos: Position, ev: EvalScope, arr: Val, keyF: Val): Val = {
	val arrValue = toArrOrString(arr, pos, ev)
	if (arrValue.length <= 1) {
	return arr
	}
	val out = new mutable.ArrayBuffer[Lazy]
	for (v <- arrValue) {
	if (out.isEmpty) {
	out.append(v)
	} else if (keyF.isInstanceOf[Val.False]) {
	if (!ev.equal(out.last.force, v.force)) {
	out.append(v)
	}
	} else if (!keyF.isInstanceOf[Val.False]) {
	val keyFFunc = keyF.asInstanceOf[Val.Func]
	val o1Key = keyFFunc.apply1(v, pos.noOffset)(ev)
	val o2Key = keyFFunc.apply1(out.last, pos.noOffset)(ev)
	if (!ev.equal(o1Key, o2Key)) {
	out.append(v)
	}
	}
	}
	new Val.Arr(pos, out.toArray)
	}

[stephenamar-db]

Bug in nearby code below the fold:

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 927 to 959 in 877f5aa

	if (!rValue.isInstanceOf[Val.Null]) { // if we are not removing the key
	if (lValue != null && rValue == null) {
	// Preserve the LHS/target value:
	kvs(kvsIdx) = (key, new Val.Obj.ConstMember(false, Visibility.Normal, lValue))
	} else if (lValue.isInstanceOf[Val.Obj] && rValue.isInstanceOf[Val.Obj]) {
	// Recursively merge objects:
	kvs(kvsIdx) = (key, createLazyMember(recPair(lValue, rValue)))
	} else if (rValue != null) {
	// Use the RHS/patch value and recursively remove Null or hidden fields:
	kvs(kvsIdx) = (key, createLazyMember(recSingle(rValue)))
	} else {
	Error.fail("std.mergePatch: This should never happen")
	}
	kvsIdx += 1
	}
	i += 1
	}
	val trimmedKvs = if (kvsIdx == i) kvs else kvs.slice(0, kvsIdx)
	Val.Obj.mk(mergePosition, trimmedKvs)
	case (_, _) => recSingle(r)
	}
	def recSingle(v: Val): Val = v match{
	case obj: Val.Obj =>
	val keys: Array[String] = obj.visibleKeyNames
	val kvs: Array[(String, Val.Obj.Member)] = new Array[(String, Val.Obj.Member)](keys.length)
	var kvsIdx = 0
	var i = 0
	while (i < keys.length) {
	val key = keys(i)
	val value = obj.value(key, pos, obj)(ev)
	if (!value.isInstanceOf[Val.Null]) {

In mergePatch I think we want to do force on the members before the isInstanceOf checks, e.g. force after we call .valueRaw? Unless there's some invariant whereby we can never get lazy values there.

Isn't there a bug here:

            val lValue = if (l.containsVisibleKey(key)) l.valueRaw(key, l, pos)(ev) else null
            val rValue = if (r.containsVisibleKey(key)) r.valueRaw(key, r, pos)(ev) else null
            if (rValue.isInstanceOf[Val.Null]) { // if we are not removing the key
              if (lValue != null && rValue == null) {

rValue is never null?

[stephenamar-db]

I think we also need this in avg and sum:

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 1530 to 1544 in 877f5aa

	builtin("sum", "arr") { (_, _, arr: Val.Arr) =>
	if (!arr.forall(_.isInstanceOf[Val.Num])) {
	Error.fail("Argument must be an array of numbers")
	}
	arr.asLazyArray.map(_.force.asDouble).sum
	},
	builtin("avg", "arr") { (_, _, arr: Val.Arr) =>
	if (!arr.forall(_.isInstanceOf[Val.Num])) {
	Error.fail("Argument must be an array of numbers")
	}
	if (arr.length == 0) {
	Error.fail("Cannot calculate average of an empty array")
	}
	arr.asLazyArray.map(_.force.asDouble).sum/arr.length
	},

forall calls .force

[stephenamar-db]

Perhaps required in uniqArr after function application? It looks like ev.equal doesn't force its arguments.

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 1742 to 1767 in 877f5aa

	private def uniqArr(pos: Position, ev: EvalScope, arr: Val, keyF: Val): Val = {
	val arrValue = toArrOrString(arr, pos, ev)
	if (arrValue.length <= 1) {
	return arr
	}
	val out = new mutable.ArrayBuffer[Lazy]
	for (v <- arrValue) {
	if (out.isEmpty) {
	out.append(v)
	} else if (keyF.isInstanceOf[Val.False]) {
	if (!ev.equal(out.last.force, v.force)) {
	out.append(v)
	}
	} else if (!keyF.isInstanceOf[Val.False]) {
	val keyFFunc = keyF.asInstanceOf[Val.Func]
	val o1Key = keyFFunc.apply1(v, pos.noOffset)(ev)
	val o2Key = keyFFunc.apply1(out.last, pos.noOffset)(ev)
	if (!ev.equal(o1Key, o2Key)) {
	out.append(v)
	}
	}
	}
	new Val.Arr(pos, out.toArray)
	}

all callers of uniqArr and sortArr call force

[JoshRosen]

Isn't there a bug here:

            val lValue = if (l.containsVisibleKey(key)) l.valueRaw(key, l, pos)(ev) else null
            val rValue = if (r.containsVisibleKey(key)) r.valueRaw(key, r, pos)(ev) else null
            if (rValue.isInstanceOf[Val.Null]) { // if we are not removing the key
              if (lValue != null && rValue == null) {

rValue is never null?

I think you omitted a ! in the quoted code?

From

sjsonnet/sjsonnet/src/sjsonnet/Std.scala

Lines 925 to 928 in 877f5aa

	val lValue = if (l.containsVisibleKey(key)) l.valueRaw(key, l, pos)(ev) else null
	val rValue = if (r.containsVisibleKey(key)) r.valueRaw(key, r, pos)(ev) else null
	if (!rValue.isInstanceOf[Val.Null]) { // if we are not removing the key
	if (lValue != null && rValue == null) {

rValue could be null if right doesn't contain that key. An null.isInstanceOf[Val.Null] will be false, so its negation would be true hence a need to check for rValue nullness in that condition.

[stephenamar-db]

yeah a typo

[JoshRosen]

I took a careful look at the stdlib updates, paying special attention to whether we have .force calls in the right places. I specifically focused on places where we do .isInstanceOf or match on Vals.

I spotted a couple of potential bugs, some of which were pre-existing issues (e.g. inconsistency in forcing the results of function application or object field retrieval prior to comparison) and others which are new in this PR.

This looks good overall pending fixes to the places noted above.