Repository: google/jsonnet (language: python)

Issue:
Different behavior of hidden status inheritance between Jsonnet and Go-Jsonnet
## 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:

```jsonnet
// `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:

```python
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`):

```console
(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": { }
}

```



Discussion / hints:
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

```c++
        } 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](https://github.com/google/go-jsonnet/blob/master/internal/program/desugarer.go#L206-L237) to a call to a builtin function ([builtinUglyObjectFlatMerge](https://github.com/google/go-jsonnet/blob/master/builtins.go#L1429-L1473)) 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.
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](https://github.com/google/jsonnet/blob/master/core/vm.cpp#L747), instead of `ObjectField::VISIBLE`. No need to track visibility in the `HeapComprehensionObject` struct.

Fix patch:
diff --git a/core/vm.cpp b/core/vm.cpp
index 21418c475..62966e1d0 100644
--- a/core/vm.cpp
+++ b/core/vm.cpp
@@ -748,7 +748,7 @@ class Interpreter {
 
         } else if (auto *obj = dynamic_cast<const HeapComprehensionObject *>(obj_)) {
             for (const auto &f : obj->compValues)
-                r[f.first] = ObjectField::VISIBLE;
+                r[f.first] = ObjectField::INHERIT;
         }
         return r;
     }


Test patch:
diff --git a/test_suite/object.jsonnet b/test_suite/object.jsonnet
index 096bac402..5ed939600 100644
--- a/test_suite/object.jsonnet
+++ b/test_suite/object.jsonnet
@@ -80,6 +80,11 @@ local obj = {
 
 std.assertEqual(obj, { ["f" + x + y + z]: { x: x, y: y, z: z } for x in [1, 2, 3] for y in [1, 4, 6] if x + 2 < y for z in [true, false] }) &&
 
+// Tests for #1111 - object comprehension fields should use "inherit" visibility.
+std.assertEqual(std.objectFields({ x: 1 } + { [k]: 2 for k in ['x'] }), ['x']) &&
+std.assertEqual(std.objectFields({ x:: 1 } + { [k]: 2 for k in ['x'] }), []) &&
+std.assertEqual(std.objectFields({ x::: 1 } + { [k]: 2 for k in ['x'] }), ['x']) &&
+
 std.assertEqual({ f: { foo: 7, bar: 1 } { [self.name]+: 3, name:: "foo" }, name:: "bar" },
                 { f: { foo: 7, bar: 4 } }) &&
 
diff --git a/test_suite/object.jsonnet.fmt.golden b/test_suite/object.jsonnet.fmt.golden
index 64055f64a..85f9119b1 100644
--- a/test_suite/object.jsonnet.fmt.golden
+++ b/test_suite/object.jsonnet.fmt.golden
@@ -80,6 +80,11 @@ local obj = {
 
 std.assertEqual(obj, { ['f' + x + y + z]: { x: x, y: y, z: z } for x in [1, 2, 3] for y in [1, 4, 6] if x + 2 < y for z in [true, false] }) &&
 
+// Tests for #1111 - object comprehension fields should use "inherit" visibility.
+std.assertEqual(std.objectFields({ x: 1 } + { [k]: 2 for k in ['x'] }), ['x']) &&
+std.assertEqual(std.objectFields({ x:: 1 } + { [k]: 2 for k in ['x'] }), []) &&
+std.assertEqual(std.objectFields({ x::: 1 } + { [k]: 2 for k in ['x'] }), ['x']) &&
+
 std.assertEqual({ f: { foo: 7, bar: 1 } { [self.name]+: 3, name:: 'foo' }, name:: 'bar' },
                 { f: { foo: 7, bar: 4 } }) &&