fix(encoding/json): honor omitempty and unwrap boxed interface{} on marshal

[mck]

Part of #142 (items 1 and 2). Independent of #143 (different area of the codebase). Touches gs/encoding/json/index.ts like #145 and #146, but a different area (Marshal's encode path vs their Unmarshal decode path); index.ts itself merges cleanly, only the shared index.test.ts has a trivial fixture-insertion conflict (see #145's note).

Two separate Marshal bugs, grouped into one PR because both live in marshalValue/assignStructFields and are small on their own.

First, ,omitempty struct tags were ignored: fields tagged ,omitempty were emitted unconditionally, even when holding a Go zero value (zero number, empty string, false, empty slice/map, nil pointer/interface).

type Config struct {
    Name string `json:"name,omitempty"`
}
b, _ := json.Marshal(Config{})
// got:  {"name":""}
// want: {}

Second, a boxed interface{} value leaked its wrapper into the output. The runtime boxes a named/defined type's value stored in an interface as { __goType, __goValue, ... } so it can carry methods (namedValueInterfaceValue in gs/builtin/type.ts), and marshalValue serialized that wrapper object directly instead of the value it holds.

type Status int
var v any = Status(2) // boxed as {__goType, __goValue: 2, ...}
b, _ := json.Marshal(v)
// got:  {"__goType":"...","__goValue":2,...} plus invalid-JSON garbage
//       from the wrapper's non-JSON-able method/valueOf/toString props
// want: 2

The fix adds jsonOmitEmpty (parses the tag's option list) and isEmptyValue (mirrors Go's zero-value rules for numbers, strings, bools, slices, maps, Uint8Array as []byte, and nil); a field is skipped when both hold. marshalValue now unwraps __goValue (recursively, in case of nesting) before falling through to primitive/slice/map/struct handling, guarded by !isStructValue(v) so a real struct value is never mistaken for a wrapper.

Tests extend gs/encoding/json/index.test.ts with zero-valued vs populated omitempty fields (string/int/slice/[]byte) and a boxed primitive at the top level and nested inside a struct field. All new assertions fail on master and pass with this fix.

Verify:

bun run vitest run gs/encoding/json/index.test.ts

20 passed. Full suite: bun run typecheck clean, bun run test:js 1273 passed / 4 skipped / 0 failed, go test ./... all packages green, bun run lint:js clean.

Found while transpiling and running a real ~100k-LOC Go interpreter/parser package through goscript.

[Copilot]

Pull request overview

This PR fixes two correctness issues in the GoScript gs/encoding/json runtime’s Marshal path: it now honors ,omitempty on struct tags and unwraps GoScript’s boxed interface{} wrapper ({ __goType, __goValue, ... }) so the underlying value is serialized instead of the wrapper object.

Changes:

• Add boxed-interface{} unwrapping in marshalValue to serialize __goValue (recursively) rather than the wrapper.
• Implement ,omitempty handling for struct fields via jsonOmitEmpty + isEmptyValue.
• Extend JSON runtime tests to cover omitempty and boxed interface marshaling (top-level and nested).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
gs/encoding/json/index.ts	Adds boxed interface unwrapping and omitempty skipping logic during struct marshaling.
gs/encoding/json/index.test.ts	Adds regression tests for omitempty and boxed interface{} marshaling behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mck]

Good catch, fixed in df3de9d: isEmptyValue now treats a zero-length Uint8Array as empty, and the omitempty test struct has a []byte field covering both the omit-when-empty and include-when-populated cases.

[mck]

Fixed in df3de9d: replaced the double-cast with a cast on the VarRef itself, keeping the boxed wrapper value intact.

[paralin]

The direction is right, but I found two marshal correctness cases that should be fixed before merge. I verified the current head is still 88ebd2f before re-checking this.

First, the boxed-interface unwrap is too broad. The new code treats any non-struct object with a __goValue property as a GoScript box:

if (
  typeof v === 'object' &&
  v !== null &&
  '__goValue' in v &&
  !isStructValue(v)
) {
  return marshalValue((v as { __goValue: unknown }).__goValue)
}

The runtime box created by namedValueInterfaceValue also carries a string __goType. __goValue alone is not enough evidence that the object is a box. Because Marshal otherwise accepts ordinary JS objects and lets them serialize as JSON objects, this currently changes accepted input like this:

const [data, err] = Marshal({ __goValue: 5 })

expect(err).toBeNull()
expect($.bytesToString(data)).toBe('{"__goValue":5}')

Current result on this PR head:

Received: "5"

Suggested fix: gate this path on the actual boxed shape, at least typeof __goType === 'string' && '__goValue' in v, or put a shared boxed-value predicate near the boxed-value owner and call that from JSON.

Second, omitempty still misses empty byte slices. marshalValue serializes Uint8Array as the runtime byte-slice representation, but isEmptyValue only treats JS arrays and Map as empty collections. A json:",omitempty" byte-slice field still emits an empty base64 string:

class ByteOmitEmptyStruct {
  public _fields = {
    Data: $.varRef(new Uint8Array(0)),
  }

  static __typeInfo = $.registerStructType(
    'test.ByteOmitEmptyStruct',
    new ByteOmitEmptyStruct(),
    [],
    ByteOmitEmptyStruct,
    [
      {
        name: 'Data',
        key: 'Data',
        type: {
          kind: $.TypeKind.Slice,
          elemType: { kind: $.TypeKind.Basic, name: 'uint8' },
        },
        tag: 'json:"data,omitempty"',
      },
    ],
  )
}

const [data, err] = Marshal(new ByteOmitEmptyStruct())

expect(err).toBeNull()
expect($.bytesToString(data)).toBe('{}')

Current result on this PR head:

Received: "{\"data\":\"\"}"

Suggested fix: include t instanceof Uint8Array && t.length === 0 in isEmptyValue. If complex slice proxies can reach this path, cover those through the slice helper rather than adding another ad hoc shape check.

One smaller cleanup: the new JSON test uses namedString as unknown as string. Please avoid the double-cast. It hides the runtime boundary the test is trying to prove. A fixture field typed as unknown/any, or a test that asserts the boxed value at the interface boundary directly, would keep the contract clearer.

Checks I ran:

bun run vitest run gs/encoding/json/index.test.ts
# 20 passed

bun run vitest run gs/encoding/json/pr-review.test.ts
# 2 failed: the two cases above

[mck]

Thanks for the thorough review, all three points are addressed.

The boxed-shape gate: fixed in f0043e4 using your second suggestion, a shared predicate next to the box creator. isNamedValueBox (gs/builtin/type.ts, right after namedValueInterfaceValue) requires both markers, a string __goType plus __goValue, and the JSON marshal path now calls that instead of checking __goValue alone. Your Marshal({ __goValue: 5 }) case is added as a regression test and passes ({"__goValue":5}).

The empty byte-slice omitempty and the double-cast: both were fixed in df3de9d, which crossed with your review (it landed for Copilot's overlapping comments a few minutes after you reviewed 88ebd2f). isEmptyValue treats a zero-length Uint8Array as empty, with omit-when-empty and include-when-populated cases on a []byte field, and the double-cast is gone.

Verify:

bun run vitest run gs/encoding/json/index.test.ts

21 passed. Full suite: typecheck clean, test:js 1274 passed / 4 skipped / 0 failed, go test ./... all packages ok, lint clean.

out of date

[paralin]

Requesting changes: the direction is right, but the current omitempty implementation still has two Go semantics regressions.

1. Non-nil pointer/interface fields are omitted when their contents are zero.

At gs/encoding/json/index.ts:929, the skip decision calls isEmptyValue(ref.value) without looking at field.type. isEmptyValue unwraps VarRefs before testing zero-ness, so a non-nil *int pointing at 0 is treated as empty. Interface fields have the same issue: any(0), any(""), any(false), and any([]T{}) are non-nil interfaces and should still be emitted under omitempty.

Focused repro on this head:

expected {"ptr":0,"any":0}
received {}

Fix direction: make the empty check type-aware. For TypeKind.Pointer and TypeKind.Interface, omit only when the pointer/interface value itself is nil/null/undefined. Do not unwrap a non-nil pointer VarRef or inspect an interface’s dynamic value for the omitempty decision.

2. Zero bigint values are not treated as empty.

At gs/encoding/json/index.ts:960, isEmptyValue handles numeric zero only for typeof t === "number". The runtime already emits bigint JSON values, and GoScript uses bigint for int64/uint64, so int64 with json:",omitempty" and value 0n currently emits {"count":0} instead of {}.

Focused repro on this head:

expected {}
received {"count":0}

Fix direction: add a typeof t === "bigint" branch returning t === 0n, while preserving the pointer/interface rule above so any(0n) remains non-empty when the interface itself is non-nil.

[mck]

Thanks for the careful review. Both omitempty regressions are fixed in 7756eb9.

isEmptyValue now takes the field type. For TypeKind.Pointer and TypeKind.Interface it is empty only when the pointer or interface itself is nil (null/undefined), so a non-nil *int pointing at 0, and any(0), any(""), any(false), any([]T{}), are all still emitted. Non-nil pointer VarRefs and interface dynamic values are no longer unwrapped for the omitempty decision, exactly per your fix direction. Marshal({"ptr":0,"any":0}) now comes out as {"ptr":0,"any":0} instead of {}.

For zero bigint, added a typeof t === "bigint" branch returning t === 0n, ordered after the pointer/interface rule so int64 with omitempty and value 0n now yields {} while any(0n) stays non-empty because the interface is non-nil.

New regression tests in gs/encoding/json/index.test.ts cover nil vs non-nil pointer/interface holding a zero value, zero vs non-zero int64, and a non-nil interface holding a zero bigint. All fail on the prior head and pass with this change.

For completeness on the earlier round: the too-broad boxed-interface unwrap is gated on the real box shape via isNamedValueBox in gs/builtin/type.ts (requires both a string __goType and __goValue), the empty []byte case is handled in isEmptyValue via the Uint8Array length check, and the test double-cast was removed.

Verify:
bun run typecheck
bun run vitest run gs/encoding/json/index.test.ts # 23 passed
bun run test:js # 154 files, 1276 passed, 4 skipped
go test ./... # all ok
bun run lint:js

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.