Fix multiple object comprehension bugs #358

JoshRosen · 2025-05-06T01:56:02Z

This PR fixes multiple bugs in object comprehensions (fixes #357, fixes #331, plus another bug).

Mis-handling of `self` and `super` in object comprehension locals when inherited

Object comprehensions may define local variables and those variables may reference self or super.

Consider the reproduction reported in #357:

local lib = {
  foo()::
    {
      local global = self,

      [iterParam]: global.base {
        foo: iterParam
      }
      for iterParam in ["foo"]
    },
};

{
 base:: {}
}
+ lib.foo()

This is supposed to output

{
  "foo": {
    "foo": "foo"
  }
}

but sjsonnet outputs

sjsonnet.Error: Field does not exist: base
      at [Select base].(:7:26)

This bug occurs because of how bindings were managed: there's some tricky circular reference and variable rebinding logic (which I believe was added to handle references between locals) and that was rebinding the self reference to the comprehension result itself: that is wrong because the self reference may change if an object is inherited or extended.

While digging into this, I also discovered a related bug where super wasn't being properly re-bound in object comprehension locals or values:

local lib = {
  foo():: {
    local sx = super.x,
    [k]: sx + 1
    for k in ["x"]
  },
};

{ x: 2 }
+ lib.foo()

was failing with

java.lang.Exception: sjsonnet.Error: Attempt to use `super` when there is no super class
      at [SelectSuper x].(:4:21)
      at [ValidId sx].(:5:10)
      at [BinaryOp +].(:5:13)

Silent dropping of fields if key is not a string

The object comprehension

{[k]: k for k in [1]}

fails in regular jsonnet with an error

RUNTIME ERROR: field must be string, got: number
        <cmdline>:1:1-22

but in sjsonnet it was silently returning {}. This regression was introduced in #226 by adding a default case _ => (presumably to fix a compiler or IDE warning).

Support function definition via object comprehensions

This fixes #331 by adding parser support for defining functions using method syntax, e.g.

{
  [a](x): x * 2
  for a in ["f1", "f2", "f3"]
}

Fixes

This PR fixes the above bugs by changing logic within Evaluator.visitObjComp. It is best reviewed commit-by-commit or using a whitespace-free diff.

sjsonnet/src/sjsonnet/Evaluator.scala

JoshRosen · 2025-05-06T01:58:48Z

sjsonnet/src/sjsonnet/Evaluator.scala

+            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)


There are two changes here:

use self instead of newSelf, which allows for bindings' references to self to be properly updated when re-evaluated.

Pass in sup to allow super references to resolve properly (in both local variables and in the values of the resulting object).

JoshRosen · 2025-05-06T02:10:01Z

sjsonnet/src/sjsonnet/Evaluator.scala

+                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)


On closer look, I see that this is still passing null for newSuper here and that probably indicates that there's still a latent bug. Let me see if I can devise a test case.

Indeed, there was an issue:

Test: 55bcb26

Fix (a net code simplification!): fd038d8

JoshRosen · 2025-05-06T02:27:49Z

I noticed that ValScope.extend and Evaluator.visitBindings now each only have a single callsite, which might permit some further simplification.

Edit: now done in b74a493

…ndings()

He-Pin · 2025-05-07T03:42:13Z

#331

I tested with

      eval(
        """
          |local funcs = {
          |  [a](x): x * 2
          |  for a in ["f1", "f2", "f3"]
          |};
          |funcs.f1(10)
          |""".stripMargin) ==> ujson.Num(20)

gets:

sjsonnet.StaticError: Unknown variable: x
    at [Id x].(:3:11)

java.lang.Exception: sjsonnet.StaticError: Unknown variable: x
    at [Id x].(:3:11)

He-Pin · 2025-05-07T15:37:48Z

sjsonnet/src/sjsonnet/Evaluator.scala

+        case x =>
+          Error.fail(
+            s"Field name must be string or null, not ${x.prettyName}",
+            e.pos


Field name must be string or null, got ${x.prettyName}

I copied this wording from

sjsonnet/sjsonnet/src/sjsonnet/Evaluator.scala

Lines 683 to 687 in 84ac320

case x =>

Error.fail(

s"Field name must be string or null, not ${x.prettyName}",

pos

)

further up in this file; I suppose I can factor it out into a helper function.

I factored this out in bd4e1b2 but, curiously, this appears to cause a test failure in the JDK 17 build: https://github.com/databricks/sjsonnet/actions/runs/14891478275/job/41824335648 :

java.lang.StackOverflowError java.lang.String.rangeCheck(String.java:304) java.lang.String.<init>(String.java:300) jdk.internal.org.objectweb.asm.ClassReader.readUtf(ClassReader.java:3717) jdk.internal.org.objectweb.asm.ClassReader.readUtf(ClassReader.java:3685) jdk.internal.org.objectweb.asm.ClassReader.readUTF8(ClassReader.java:3666) jdk.internal.org.objectweb.asm.ClassReader.readConst(ClassReader.java:3841) java.lang.invoke.MethodHandles$Lookup$ClassFile.newInstance(MethodHandles.java:2258) java.lang.invoke.MethodHandles$Lookup.makeHiddenClassDefiner(MethodHandles.java:2359) java.lang.invoke.MethodHandles$Lookup.defineHiddenClass(MethodHandles.java:2127) java.lang.invoke.InnerClassLambdaMetafactory.generateInnerClass(InnerClassLambdaMetafactory.java:407) java.lang.invoke.InnerClassLambdaMetafactory.spinInnerClass(InnerClassLambdaMetafactory.java:315) java.lang.invoke.InnerClassLambdaMetafactory.buildCallSite(InnerClassLambdaMetafactory.java:228) java.lang.invoke.LambdaMetafactory.altMetafactory(LambdaMetafactory.java:536) java.lang.invoke.BootstrapMethodInvoker.invoke(BootstrapMethodInvoker.java:147) java.lang.invoke.CallSite.makeSite(CallSite.java:315) java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:281) java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:271) os.Path.relativeTo(Path.scala:584) sjsonnet.OsPath.relativeToString(OsPath.scala:6) sjsonnet.Error$Frame.<init>(Error.scala:51) sjsonnet.Error$.fail(Error.scala:71) sjsonnet.Materializer.apply0(Materializer.scala:72) sjsonnet.Materializer.apply0(Materializer.scala:53) sjsonnet.Materializer.apply0(Materializer.scala:53) [...]

I think the problem is that we have code within the Materializer which catches StackOverflowError and then tries to call Error.fail and that, in turn, hits a secondary StackOverflow because the stack is already so deep:

sjsonnet/sjsonnet/src/sjsonnet/Materializer.scala

Lines 70 to 73 in 84ac320

} catch {

case _: StackOverflowError =>

Error.fail("Stackoverflow while materializing, possibly due to recursive value", v.pos)

}

I tried adding an @inline to the new helper to see if that would somehow hack around this (f5e40c2) but it didn't help.

This preexisting design seems very fragile.

If we want to more fundamentally fix this and remove the core fragility, I think we probably need to allow the StackOverflowError to bubble higher rather than catching it so deep, but that introduces some new challenges around making sure that we preserve the v which triggered the error. I'm thinking that we might want to re-throw a wrapped exception containing a pointer to v and then catch it and transform it to an Error.fail at a higher level of the callstack (e.g. the outermost apply0 call). That's a larger refactoring / change, though, and I don't want to co-mingle it here.

Given this, I'm going to back out of the error message DRY and will re-attempt it in a separate PR.

Hmm, I think this might just be nondeterministically broken because the current reverted commit fails but its checkout tree is identical to a commit which passed two days ago.

I think that this is a test ordering issue: the current test for the stack overflow handling fails if it runs prior to other tests: it was implicitly relying on other tests to perform certain loading and initialization steps.

We should fix this properly (e.g. by hardening the actual implementation, as this is a real bug), but as a temporary "unblock the test flakiness" hack I'm trying to force a particular loading step: 5cd4ed8

@inline is just a hint; we can't rely on it.
For StackOverflow, I raised #284 once, but it may hurt some performance; there is some TCO in other implementations, eg : google/jsonnet#1142

He-Pin · 2025-05-07T15:44:16Z

sjsonnet/src/sjsonnet/ValScope.scala

-    val by = if (newBindingsF == null) 2 else newBindingsF.length + 2
-    val b = Arrays.copyOf(bindings, bindings.length + by)
+  def extend(newBindings: Array[Lazy], newSelf: Val.Obj, newSuper: Val.Obj): ValScope = {
+    val b = Arrays.copyOf(bindings, bindings.length + newBindings.length + 2)


bindings.length and newbindings.length can be extracted to a local variable?

I would prefer to leave this as-is for now, given that the other methods (including extendSimple) have similar repetition of .length. My understanding is that JIT should already be optimizing this nicely.

He-Pin · 2025-05-07T15:57:04Z

sjsonnet/src/sjsonnet/Evaluator.scala

+            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, newScope)


This code is a little weird, it references each others.
It would be nice to add some comments here.

…low test." This reverts commit f5e40c2.

This reverts commit bd4e1b2.

JoshRosen · 2025-05-07T20:42:27Z

#331

I tested with

      eval(
        """
          |local funcs = {
          |  [a](x): x * 2
          |  for a in ["f1", "f2", "f3"]
          |};
          |funcs.f1(10)
          |""".stripMargin) ==> ujson.Num(20)

gets:

sjsonnet.StaticError: Unknown variable: x
    at [Id x].(:3:11)

java.lang.Exception: sjsonnet.StaticError: Unknown variable: x
    at [Id x].(:3:11)

That StaticError is being thrown before we even begin evaluation, so it's not even hitting any of the Evaluator code modified in this PR.

As you noted in #337, the problem is that we're currently discarding any knowledge of RHS args (denoting that it's a function).

This turned out to be fairly straightforward to fix: I just needed to modify the parser to create an Expr.Function when args are present: e06e7a7 ; I'll update the PR description to reflect this fix.

stephenamar-db · 2025-05-11T02:57:03Z

Thanks @JoshRosen

He-Pin · 2025-05-16T05:54:23Z

@JoshRosen Thank you , helps a lot

JoshRosen added 8 commits May 5, 2025 17:56

Add more test cases covering existing behaviors.

05ed13c

Add (failing) regression test for reported bug.

280b679

Fix reported bug.

2b76aeb

Add (failing) regression test for related prexisting bug for super refs.

64edef1

Fix related bug for super refs.

ee94db1

Add (failing) regression test for invalid obj comp key types.

7a281cb

Error on invalid obj comp key types.

856c193

Remove now-unused newSelf variable.

3239399

JoshRosen commented May 6, 2025

View reviewed changes

sjsonnet/src/sjsonnet/Evaluator.scala Show resolved Hide resolved

JoshRosen commented May 6, 2025

View reviewed changes

JoshRosen mentioned this pull request May 6, 2025

Evaluator.visitObjComp doesn't propagate context correctly #357

Closed

Add missing useNewEvaluator in eval.

1c51f11

JoshRosen commented May 6, 2025

View reviewed changes

JoshRosen added 2 commits May 5, 2025 19:12

Add regression test for another super bug.

55bcb26

Fix super in value calculations.

fd038d8

JoshRosen added 3 commits May 5, 2025 19:45

Simplify thunks / laziness in ValScope.extend() and Evaluator.visitBi…

b74a493

…ndings()

remove one unnecessary local

fd3e9f4

scalafmt fixes

cb5593e

He-Pin reviewed May 7, 2025

View reviewed changes

DRY up the error message.

bd4e1b2

JoshRosen force-pushed the obj-comp-fixes branch from 1a31d18 to bd4e1b2 Compare May 7, 2025 19:17

JoshRosen added 5 commits May 7, 2025 12:26

See if inlining method avoids over-sensitivity in stack overflow test.

f5e40c2

Revert "See if inlining method avoids over-sensitivity in stack overf…

4867920

…low test." This reverts commit f5e40c2.

Revert "DRY up the error message."

63cad73

This reverts commit bd4e1b2.

DRY up the error message.

7df617e

Support function definitions via object comprehensions.

e06e7a7

JoshRosen added 2 commits May 7, 2025 14:03

Hack to address CI flakiness.

5cd4ed8

Add comment calling out circular reference

0b2fce2

JoshRosen force-pushed the obj-comp-fixes branch from 31e32be to 0b2fce2 Compare May 8, 2025 18:18

stephenamar-db approved these changes May 11, 2025

View reviewed changes

stephenamar-db merged commit b2690e6 into databricks:master May 11, 2025
6 checks passed

	case x =>
	Error.fail(
	s"Field name must be string or null, not ${x.prettyName}",
	pos
	)

	} catch {
	case _: StackOverflowError =>
	Error.fail("Stackoverflow while materializing, possibly due to recursive value", v.pos)
	}

Fix multiple object comprehension bugs #358

Fix multiple object comprehension bugs #358

Conversation

JoshRosen commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Mis-handling of self and super in object comprehension locals when inherited

Silent dropping of fields if key is not a string

Support function definition via object comprehensions

Fixes

Uh oh!

Uh oh!

JoshRosen May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshRosen May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

He-Pin commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

He-Pin May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

He-Pin May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenamar-db commented May 11, 2025

Uh oh!

Uh oh!

He-Pin commented May 16, 2025

Uh oh!

Uh oh!

JoshRosen commented May 6, 2025 •

edited

Loading

Mis-handling of `self` and `super` in object comprehension locals when inherited

JoshRosen May 6, 2025 •

edited

Loading

JoshRosen May 6, 2025 •

edited

Loading

JoshRosen commented May 6, 2025 •

edited

Loading

He-Pin commented May 7, 2025 •

edited

Loading

He-Pin May 8, 2025 •

edited

Loading

He-Pin May 7, 2025 •

edited

Loading

JoshRosen commented May 7, 2025 •

edited

Loading