Different behavior of hidden status inheritance between Jsonnet and Go-Jsonnet

[simu]

Summary

I've been evaluating switching to Go-Jsonnet for our tooling and stumbled across a difference in how hidden status inheritance (https://jsonnet.org/ref/spec.html#hidden_inheritance) is implemented in Jsonnet and Go-Jsonnet.

Here's an example Jsonnet script which demonstrates the difference:

// `makeMergeable` is taken from our tooling. We use this extensively to make user inputs loaded from YAML mergeable
local makeMergeable(o) = {
  [key]+: makeMergeable(o[key])
  for key in std.objectFields(o)
  if std.isObject(o[key])
} + {
  [key]+: o[key]
  for key in std.objectFields(o)
  if std.isArray(o[key])
} + {
  [key]: o[key]
  for key in std.objectFields(o)
  if !std.isObject(o[key]) && !std.isArray(o[key])

};

local base = { field:: 'data' };

{
  regular: base { field: 'other' }, // hidden status of `field` is inherited in both implementations
  makeMergeable: base + makeMergeable({
    field: 'other',
  }), // hidden status of `field` is inherited through the object comprehension in go-jsonnet, but the hidden status of the field is lost in jsonnet
}

From what I can tell, the go-jsonnet implementation implements the specification correctly.

Example script evaluation and output

I've wrapped the example in Python (because that's what I had readily available) to run both implementations side-by-side:

import _jsonnet
import _gojsonnet

prog = """
// `makeMergeable` is taken from our tooling. We use this extensively to make user inputs mergeable
local makeMergeable(o) = {
  [key]+: makeMergeable(o[key])
  for key in std.objectFields(o)
  if std.isObject(o[key])
} + {
  [key]+: o[key]
  for key in std.objectFields(o)
  if std.isArray(o[key])
} + {
  [key]: o[key]
  for key in std.objectFields(o)
  if !std.isObject(o[key]) && !std.isArray(o[key])

};

local base = { field:: 'data' };

{
  regular: base { field: 'other' }, // hidden status of `field` is inherited in both implementations
  makeMergeable: base + makeMergeable({
    field: 'other',
  }), // hidden status of `field` is inherited through the object comprehension in go-jsonnet, but the hidden status of the field is lost in jsonnet
}
"""

print("Jsonnet:\n" + _jsonnet.evaluate_snippet("test.jsonnet", prog))
print("Go-Jsonnet:\n" + _gojsonnet.evaluate_snippet("test.jsonnet", prog))

This gives the following in a virtualenv (setup with python3 -m venv venv && source venv/bin/activate && pip install jsonnet gojsonnet):

(venv) simon@phoenix:~/tmp/jsonnet-issue $ pip list
Package    Version
---------- -------
gojsonnet  0.20.0
jsonnet    0.20.0
pip        22.0.2
setuptools 59.6.0
(venv) simon@phoenix:~/tmp/jsonnet-issue $ python test.py
Jsonnet:
{
   "makeMergeable": {
      "field": "other"
   },
   "regular": { }
}

Go-Jsonnet:
{
   "makeMergeable": { },
   "regular": { }
}

[johnbartholomew]

Thanks for the bug report! Starting to investigate this.

The C++ implementation implements object comprehensions through an internal object type HeapComprehensionObject. core/vm.cpp objectFieldsAux iteration over the members of a comprehension:

https://github.com/google/jsonnet/blob/master/core/vm.cpp#L745-L748

        } else if (auto *obj = dynamic_cast<const HeapComprehensionObject *>(obj_)) {
            for (const auto &f : obj->compValues)
                r[f.first] = ObjectField::VISIBLE;
        }

All fields are forced to 'VISIBLE', which probably accounts for the behaviour you're seeing here.

The Go version implements object comprehensions differently, desugaring them to a call to a builtin function (builtinUglyObjectFlatMerge) which evaluates to a 'simple' object. That function appears to copy the field visibility to its output object.

There may be details that I haven't grokked properly, particularly in the Go version, as I haven't really looked at the go-jsonnet implementation before.

I think it's possible to make the C++ version respect field visibility in object comprehensions by storing visibility in the HeapComprehensionObject and applying that as appropriate in objectFieldsAux. However I'm fairly new to the Jsonnet codebase and would need to spend more time on it to be confident that I haven't missed other code-paths that need to be changed.

[johnbartholomew]

I will also note that my reading of the actual language abstract syntax at https://jsonnet.org/ref/spec.html#abstract_syntax is that object comprehensions as specified don't actually support either differing visibility, or the object-merge (+:) sugar. The object-merge and visibility parts of the abstract syntax are in 'field', but object comprehension syntax is not actually defined in terms of 'field' but rather in terms of two 'expr' (that is, the spec has: [ expr ] : expr).

Regardless, if the merge sugar +: is supported within object comprehensions in practice, it would seem a bad regression to get rid of it.

But it seems hidden or force-visible fields really aren't supported in object comprehensions. This simplifies a fix on the C++ side, where I think objectFieldsAux can just use ObjectField::INHERIT on vm.cpp#L47, instead of ObjectField::VISIBLE. No need to track visibility in the HeapComprehensionObject struct.

[sparkprime]

Even if +: was not supported then you could use super directly in the object comprehension to get the same effect.

[sparkprime]

It looks like the semantics declare that the object comprehension should use INHERIT (single :) so go is right and c++ is wrong

[sparkprime]

As an aside, I often thought it would be nice if an object comprehension could choose :: or ::: but that leads you to wanting a different kind of visibility inheritance for each field and then it's hard to imagine how that would be expressed in the code without some complex new syntax. Hence, no support beyond the simple : for every field.