Skip to content

internal/vm: a live instance variable is never hidden by the order list - #807

Merged
tannevaled merged 4 commits into
mainfrom
fix/ivar-order
Oct 9, 2026
Merged

tannevaled merged 4 commits into
mainfrom
fix/ivar-order

Conversation

@tannevaled

Copy link
Copy Markdown
Contributor

Closes #787.

What it was

A YAML-loaded object answered o.a == 1 and instance_variables == [].

That is the worst version of this bug. The ivars were there and readable by
name, so every direct check passes; what breaks is anything that walks the
object — marshalling, inspect, a serialiser, an equality written over
instance_variables — and it breaks by seeing an empty object, silently.

Cause

For an *RObject, ivarStoreOf returns order: &o.ivarOrder — the address
of a struct field
, so the order pointer is never nil and ivarNamesInOrder
always takes the ordered branch. A constructor that fills the ivars map directly
and leaves ivarOrder empty therefore produces ivars that read back and do not
enumerate, with nothing anywhere to say so.

exception.go:377 builds its RObject with an ivarOrder, which is why
exceptions enumerate correctly and this stayed invisible.

Two changes, because one is the fix and the other is a floor

yaml_bind.go convObject now fills ivarOrder alongside ivars, in the
sequence the yaml package documents for emission (value.go:86-89 — the names
in o.Order that exist, then the rest lexicographically). That is the
document's order, so the result matches MRI:

document z, a, m MRI [:@z, :@a, :@m]
rbgo after [:@z, :@a, :@m], identical over three runs

z, a, m is neither alphabetical nor reversed, and Go's map iteration is
randomised per run, so reproducing it by accident is not plausible.

ivarNamesInOrder now also reports live names missing from the order list,
sorted. Constructors that fill ivarOrder never reach that path, so creation
order is preserved where it is known; what it buys is that the next
constructor to fill a map directly is wrong visibly rather than silently.

What the coverage gate asked, and the two different answers

The gate named two functions, and they needed opposite treatment — which is
exactly why it is worth it asking rather than me deciding in advance.

  • ivarNamesInOrder's new arm is unreached, not dead. Nothing gets there any
    more precisely because the constructor was fixed. It guards a shape any new
    code can produce in one line, so it gets an internal test that builds that
    shape: an empty order list (all names, sorted), a half-filled one (the listed
    name keeps its place, the rest follow sorted), and a name in the list whose
    value is gone — which stays gone, since a floor that resurrected a removed
    ivar would be worse than no floor.
  • convObject's missing-key arm is dead: yamlIVarOrder returns only keys
    present in o.IVars, so the lookup cannot fail. Removed rather than tested,
    because a test for an unreachable branch asserts nothing.

A note on the writing of it

yamlIVarOrder already existed in the same file, eleven lines from where I
wrote a second copy of it. The duplicate is not in this branch.

🤖 Generated with Claude Code

tannevaled and others added 4 commits October 9, 2026 09:35
A YAML-loaded object answered `o.a == 1` and `instance_variables == []`. The
ivars were there and readable by name; only the enumeration could not see them,
which is the worst version of this bug -- every direct check passes, and
anything that WALKS the object (marshalling, inspect, a serialiser, an equality
written over instance_variables) silently sees an empty object.

Cause: for an *RObject, ivarStoreOf returns `order: &o.ivarOrder`, the ADDRESS
of a struct field, so the order pointer is never nil and ivarNamesInOrder always
takes the ordered branch. A constructor that fills the ivars map directly and
leaves ivarOrder empty therefore produces ivars that read back and do not
enumerate, and nothing anywhere says so.

Two changes, because one of them is a floor and the other is the fix:

  yaml_bind.go convObject now fills ivarOrder alongside ivars, in the sequence
  the yaml package documents for emission (value.go:86-89: the names in o.Order
  that exist, then the rest lexicographically). That is the document's order,
  so instance_variables matches MRI -- measured [:@z, :@A, :@m] for a document
  written z, a, m, which is neither alphabetical nor reversed, and stable over
  three runs where Go's map order is randomised per run.

  ivarNamesInOrder now also reports live names missing from the order list,
  sorted. Constructors that fill ivarOrder never reach that path, so creation
  order is preserved where it is known; what it buys is that the NEXT
  constructor to fill a map directly is wrong visibly rather than silently.

yamlIVarOrder already existed in the same file, eleven lines from where I wrote
a second copy of it; the duplicate is not in this commit.

Closes #787.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… found

Two answers to the coverage gate, and they are opposite ones, which is the
point of the gate asking rather than telling.

ivarNamesInOrder's new arm is UNREACHED, not dead. Nothing in the VM gets there
any more, because the constructor that used to leave ivarOrder empty now fills
it -- that is the fix. The arm is what keeps the NEXT such constructor from
being silent, and the shape it guards is a plain *RObject with a populated map,
which any new code can produce in one line. So it gets a test that builds
exactly that shape, with three cases: an empty order list (all names, sorted), a
half-filled one (the listed name keeps its place, the rest follow sorted), and a
name in the list whose value is gone (which stays gone -- a floor that
resurrected a removed ivar would be worse than none).

convObject's missing-key arm IS dead: yamlIVarOrder returns only keys present in
o.IVars, so the lookup cannot fail. Removed rather than tested, because a test
for an unreachable branch is a test that asserts nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
core/random/bytes_spec.rb went 8 -> 9 yesterday as one of twenty deltas applied
from a CI run. It should not have moved: it is the KNOWN FLAKE this file records
at its observed minimum, and its own header says why, in these words --

    Recording a flake at its high value would fail the lane on a coin toss

Which is what happened on the next run: 23512 (baseline 23512, +0) with one
REGRESSED and one improved, net zero, red lane. Measured here afterwards on the
same binary, six runs: 9 9 9 9 8 9.

Two earlier waves applied deltas to this file and deliberately left this path
alone, saying so each time (2026-09-29 and 2026-09-30, both in the header).
Mine did not, and the reason is worth writing down rather than just fixing:

All three checks that commit lists were ARITHMETIC. The pre-edit sum matched the
README, every one of CI's "baseline" columns matched the recorded value, and the
post-edit sum equalled CI's total. Every one passed. None of them asked whether
a number SHOULD move -- which is the question this file answers in its own
header, three times, about this exact path. I read seven lines of a header I was
editing and then edited past the rest of it.

Back to 8, and the README's three claims follow to 23,511. The note is in the
header next to the mistake rather than in a commit message nobody reads while
applying the next delta.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tannevaled
tannevaled merged commit 5fd41bb into main Oct 9, 2026
9 checks passed
@tannevaled
tannevaled deleted the fix/ivar-order branch October 9, 2026 19:09
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.

YAML-built objects have readable ivars that instance_variables does not list

1 participant