Skip to content

fix(builtin/type): a plain array never matches a map type#143

Merged
paralin merged 2 commits into
s4wave:masterfrom
mck:fix/builtin-array-not-map
Jul 3, 2026
Merged

fix(builtin/type): a plain array never matches a map type#143
paralin merged 2 commits into
s4wave:masterfrom
mck:fix/builtin-array-not-map

Conversation

@mck

@mck mck commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Part of #142 (item 7).

What

matchesMapType and typeAssert's inline map-type branch accepted any
non-null object as a candidate map, including a plain JS Array.

Why

Object.entries() of an array yields string-indexed pairs
([['0', v0], ['1', v1], ...]), which pass the key/elem sampling used to
structurally match a map type. So a Go type switch that lists
case map[string]any: before case []any: captures slices in the map
branch, and ranging over the result calls Array.prototype.entries(),
producing numeric-keyed output instead of correct slice semantics:

var v any = []any{"a", "b"}
switch x := v.(type) {
case map[string]any:
    // wrongly taken; x behaves like {0: "a", 1: "b"}
case []any:
    // never reached
}

A Go slice is never a map, so both matchesMapType and typeAssert's map
branch now reject Array/Uint8Array values up front before doing any
key/elem sampling — no further type information is needed to know a slice
isn't a map.

Fix

Two mechanical guards, added right after the existing null/object checks:

  • matchesMapType: if (Array.isArray(value) || value instanceof Uint8Array) return false
  • typeAssert's inline map branch: same condition added to the guard so it
    falls through to the normal matchesType path (which now correctly
    rejects it via the fixed matchesMapType) instead of the special-cased
    map-assertion logic.

Test

Added gs/builtin/type.test.ts covering is(), typeAssert(), and
typeSwitch() against map[string]any vs []any, plus the empty-value
edge case: an empty map[string]any check previously matched an empty
array too, via the "empty map matches any map type" shortcut in both
functions.

All cases fail on master and pass with this fix.

Verify

bun run vitest run gs/builtin/type.test.ts

7 passed.

Full suite:

  • bun run typecheck — clean
  • bun run test:js (typecheck + vitest) — 1278 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.

🤖 Generated with Claude Code

matchesMapType and typeAssert's inline map-type branch accepted any
non-null object, including a plain JS Array. Object.entries() of an
array yields string-indexed pairs ([['0', v0], ['1', v1], ...]) that
pass the key/elem sampling used to structurally match a map type, so
a Go type switch listing `case map[string]any:` before `case []any:`
captured slices in the map branch. Ranging over the "map" then reads
Array.prototype.entries(), producing numeric-keyed output instead of
correct slice semantics — e.g.:

    var v any = []any{"a", "b"}
    switch x := v.(type) {
    case map[string]any:
        // wrongly taken; x behaves like {0: "a", 1: "b"}
    case []any:
        // never reached
    }

A Go slice is never a map, so rejecting arrays (and Uint8Array, the
runtime's byte-slice representation) up front in both matchesMapType
and typeAssert's map branch is strictly correct and needs no further
type information.

Adds gs/builtin/type.test.ts covering is(), typeAssert(), and
typeSwitch() against map[string]any vs []any, including the empty-
value case (an empty map[string]any check previously matched an
empty array too, via the "empty map matches any map type" shortcut).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Marco Christian Krenn <marco.krenn@gmail.com>
Copilot AI review requested due to automatic review settings July 3, 2026 04:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a correctness bug in GoScript’s runtime type system where JavaScript arrays (and Uint8Array byte-slice values) could be structurally misclassified as Go map types during is()/typeAssert()/typeSwitch() evaluation, causing type switches to take the wrong branch.

Changes:

  • Add an early guard in matchesMapType() to reject Array and Uint8Array candidates before map key/elem sampling.
  • Add the same guard to typeAssert()’s inline map-assertion fast path so arrays fall through to the normal matchesType() logic.
  • Add Vitest coverage to ensure slices don’t match map[string]any and that typeSwitch() selects the slice case.

Reviewed changes

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

File Description
gs/builtin/type.ts Prevents arrays/byte-slices from being treated as map candidates during structural map matching and map assertions.
gs/builtin/type.test.ts Adds regression tests for array-vs-map matching across is(), typeAssert(), and typeSwitch().

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

Comment thread gs/builtin/type.test.ts
Comment on lines +26 to +28
it('is(): a plain array does not match map[string]any', () => {
expect(is([1, 2, 3], mapStringAny)).toBe(false)
})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — added a Uint8Array case alongside the plain-array is()/typeAssert() rejection tests in 15b6345.

Comment thread gs/builtin/type.test.ts Outdated
Comment on lines +60 to +65
it('an empty array does not vacuously match an empty-map fast path', () => {
// matchesMapType/typeAssert's map branch special-cases zero entries as
// "matches any map type"; an empty slice must not hit that path.
expect(is([], mapStringAny)).toBe(false)
expect(is([], sliceAny)).toBe(true)
})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — added an explicit empty-Uint8Array assertion to the empty-map-fast-path test in 15b6345.

Addresses review feedback on s4wave#143: the fix rejects Uint8Array (the
runtime's []byte representation) from matching map types alongside plain
arrays, but the original tests only covered plain JS arrays. Adds:

- a Uint8Array case alongside the existing plain-array is()/typeAssert()
  rejection tests
- an empty-Uint8Array assertion in the empty-map-fast-path test, since
  Object.entries() of an empty Uint8Array is also empty and could
  vacuously match a map type the same way an empty array could

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Marco Christian Krenn <marco.krenn@gmail.com>

@paralin paralin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I verified the current head is still 15b63454, including the extra Uint8Array coverage added after Copilot's comments.

Rejecting Array and Uint8Array before the map structural matcher is the right fix: a Go slice or byte slice is never a map, so the runtime can reject those shapes before key/element sampling.

Focused check:

bun run vitest run gs/builtin/type.test.ts
# 7 passed

@paralin paralin merged commit d70e3aa into s4wave:master Jul 3, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants