diff --git a/README.md b/README.md index 52d51e77..055a4517 100644 --- a/README.md +++ b/README.md @@ -6,14 +6,14 @@ [![License](https://img.shields.io/badge/license-BSD--3--Clause-blue)](LICENSE) [![Go](https://img.shields.io/badge/go-1.27.1%2B-00ADD8)](https://go.dev/dl/) [![Release](https://img.shields.io/github/v/release/go-embedded-ruby/ruby?label=release&color=blue)](https://github.com/go-embedded-ruby/ruby/releases/latest) -[![ruby/spec](https://img.shields.io/badge/ruby%2Fspec-23%2C512%20examples%20passing-1a7f37)](#runtime-conformance--rubyspec) +[![ruby/spec](https://img.shields.io/badge/ruby%2Fspec-23%2C511%20examples%20passing-1a7f37)](#runtime-conformance--rubyspec) **A Ruby interpreter written in pure Go, with cgo disabled** — so you can embed it in a Go program with `import "github.com/go-embedded-ruby/ruby"`, or ship it as a single static binary that cross-compiles wherever Go does, with no C toolchain and no libruby. -**How complete is it?** It runs **23,512** of ruby/spec's `language/` + `core/` +**How complete is it?** It runs **23,511** of ruby/spec's `language/` + `core/` examples ([what that counts](#runtime-conformance--rubyspec)) — a large and growing subset of the language, and **not** a drop-in replacement for CRuby. Read [What does not work yet](#what-does-not-work-yet) before you depend on it; the @@ -988,7 +988,7 @@ specification of the language — through `rbgo` under a minimal MSpec-compatibl shim, and gates CI on a **per-file** baseline ([`scripts/conformance/rubyspec/`](scripts/conformance/rubyspec/), in `BASELINE`). No file may pass fewer examples than its record or stop loading, so -measured language conformance moves in one direction; the total, **23,512**, is +measured language conformance moves in one direction; the total, **23,511**, is a derived summary rather than the thing gated. Run it with `scripts/conformance/rubyspec/run.sh`, and see *Runtime conformance* under *Status* for the full breakdown. diff --git a/internal/vm/ivar_order_internal_test.go b/internal/vm/ivar_order_internal_test.go new file mode 100644 index 00000000..9e0810eb --- /dev/null +++ b/internal/vm/ivar_order_internal_test.go @@ -0,0 +1,71 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import ( + "testing" + + "github.com/go-embedded-ruby/ruby/internal/object" +) + +// TestIvarNamesInOrderReportsNamesMissingFromTheOrderList covers the floor under +// #787 directly, because nothing in the VM reaches it any more: the constructor +// that used to leave ivarOrder empty now fills it, which is the fix, and this +// arm is what keeps the NEXT one from being silent. +// +// The coverage gate named it, and the question a named function asks is whether +// the branch is dead or unreached. It is not dead -- the shape it guards is a +// plain *RObject built with a populated ivars map, which any new constructor can +// produce in one line -- so it gets a test that builds exactly that shape rather +// than being deleted. +func TestIvarNamesInOrderReportsNamesMissingFromTheOrderList(t *testing.T) { + // Exactly the shape yaml_bind.go produced before #787: a live map, an empty + // order list. + o := &RObject{ivars: map[string]object.Value{ + "@b": object.IntValue(1), + "@a": object.IntValue(2), + }} + got := namesOf(ivarNamesInOrder(o)) + // Sorted, because the order list says nothing and Go's map order is random: + // an enumeration that changes between runs would be its own defect. + if len(got) != 2 || got[0] != "@a" || got[1] != "@b" { + t.Errorf("ivarNamesInOrder = %v, want [@a @b] (sorted, since the order list is empty)", got) + } + + // A half-filled order list: the named one keeps its position, the rest are + // appended sorted. This is the arm that would be wrong if the floor simply + // replaced the ordered walk instead of extending it. + o2 := &RObject{ + ivars: map[string]object.Value{ + "@z": object.IntValue(1), + "@m": object.IntValue(2), + "@a": object.IntValue(3), + }, + ivarOrder: []string{"@z"}, + } + got = namesOf(ivarNamesInOrder(o2)) + if len(got) != 3 || got[0] != "@z" || got[1] != "@a" || got[2] != "@m" { + t.Errorf("ivarNamesInOrder = %v, want [@z @a @m]: the listed name keeps its place, the rest follow sorted", got) + } + + // And a name in the order list whose value is gone stays gone -- the floor + // must not resurrect what remove_instance_variable deleted. + o3 := &RObject{ + ivars: map[string]object.Value{"@keep": object.IntValue(1)}, + ivarOrder: []string{"@gone", "@keep"}, + } + got = namesOf(ivarNamesInOrder(o3)) + if len(got) != 1 || got[0] != "@keep" { + t.Errorf("ivarNamesInOrder = %v, want [@keep]: a removed ivar stays removed", got) + } +} + +func namesOf(vs []object.Value) []string { + out := make([]string, 0, len(vs)) + for _, v := range vs { + out = append(out, string(v.(object.Symbol))) + } + return out +} diff --git a/internal/vm/ivar_order_test.go b/internal/vm/ivar_order_test.go new file mode 100644 index 00000000..0980b2a8 --- /dev/null +++ b/internal/vm/ivar_order_test.go @@ -0,0 +1,67 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm_test + +import "testing" + +// TestYAMLBuiltIvarsAreEnumerable (#787). +// +// 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 the 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 the order list is consulted FIRST, and the pointer to +// it is the address of a struct field, so it is never nil. A constructor that +// fills the ivars map directly and leaves ivarOrder empty therefore produces +// ivars that read back and do not enumerate, with nothing to indicate it. +func TestYAMLBuiltIvarsAreEnumerable(t *testing.T) { + checkCases(t, []runCase{ + {`require "yaml" +class P; attr_accessor :a, :b; end +o = YAML.safe_load("--- !ruby/object:P\na: 1\nb: 2\n", permitted_classes: [P]) +p o.instance_variables`, "[:@a, :@b]\n"}, + + // The order is the DOCUMENT's, not the map's: z, a, m is neither + // alphabetical nor reversed, so a run that reproduced it by accident is + // unlikely, and Go's map iteration is randomised per run. + {`require "yaml" +class Q; attr_accessor :z, :a, :m; end +o = YAML.safe_load("--- !ruby/object:Q\nz: 1\na: 2\nm: 3\n", permitted_classes: [Q]) +p o.instance_variables`, "[:@z, :@a, :@m]\n"}, + + // Reading was never broken, and must stay unbroken -- a fix that rebuilt + // the object could satisfy the enumeration and lose the values. + {`require "yaml" +class R; attr_accessor :a, :b; end +o = YAML.safe_load("--- !ruby/object:R\na: 1\nb: 2\n", permitted_classes: [R]) +p [o.a, o.b, o.instance_variable_get(:@a)]`, "[1, 2, 1]\n"}, + }) +} + +// TestALiveIvarIsNeverHiddenByTheOrderList is the floor under the above: the +// enumeration reports a live ivar even when the order list does not mention it, +// so the next constructor that fills the map directly is wrong in a VISIBLE way +// rather than a silent one. +// +// It is exercised through Marshal, whose loader is a second object builder, and +// through an ordinary object to show the common path still reports creation +// order rather than the sorted fallback. +func TestALiveIvarIsNeverHiddenByTheOrderList(t *testing.T) { + checkCases(t, []runCase{ + // Creation order, not sorted: b before a. + {`class S; def initialize; @b = 1; @a = 2; end; end +p S.new.instance_variables`, "[:@b, :@a]\n"}, + // remove_instance_variable still drops the name. + {`class T; def initialize; @x = 1; @y = 2; end + def drop; remove_instance_variable(:@x); end +end +t = T.new; t.drop; p t.instance_variables`, "[:@y]\n"}, + {`class U; def initialize; @a = 1; @b = 2; end; end +u = Marshal.load(Marshal.dump(U.new)) +p u.instance_variables`, "[:@a, :@b]\n"}, + }) +} diff --git a/internal/vm/object_model.go b/internal/vm/object_model.go index f53b693a..e5b0ea18 100644 --- a/internal/vm/object_model.go +++ b/internal/vm/object_model.go @@ -4,6 +4,7 @@ import ( "fmt" "reflect" "runtime" + "sort" "strings" "sync" @@ -2778,13 +2779,36 @@ func ivarNamesInOrder(self object.Value) []object.Value { st := ivarStoreOf(self, false) if st.order != nil { out := make([]object.Value, 0, len(*st.order)) + seen := make(map[string]bool, len(*st.order)) for _, n := range *st.order { // A remove_instance_variable leaves the name in the order list; the map // says which names are still live. if _, live := st.tbl[n]; live { out = append(out, object.Symbol(n)) + seen[n] = true } } + // A LIVE ivar missing from the order list is still an ivar. For an + // *RObject the order pointer is the address of a struct field, so it is + // NEVER nil -- which means a constructor that fills the map directly and + // leaves ivarOrder empty produces ivars that are readable by name and + // invisible to instance_variables, with nothing to say so. That is #787: + // a YAML-built object answered `o.a == 1` and `instance_variables == []`. + // + // This is a floor rather than the fix for any one constructor: the ones + // that build ivarOrder (exception.go) never reach it, so MRI's creation + // order is preserved where it is known. Sorted, because Go's map order is + // random and an enumeration that changes between runs is its own defect. + var extra []string + for n := range st.tbl { + if !seen[n] { + extra = append(extra, n) + } + } + sort.Strings(extra) + for _, n := range extra { + out = append(out, object.Symbol(n)) + } return out } out := make([]object.Value, 0, len(st.tbl)) diff --git a/internal/vm/yaml_bind.go b/internal/vm/yaml_bind.go index 02232ae2..714fb8d7 100644 --- a/internal/vm/yaml_bind.go +++ b/internal/vm/yaml_bind.go @@ -557,8 +557,23 @@ func (c *yamlFromCtx) convObject(o *yaml.Object) object.Value { cls := c.vm.yamlResolveClass(name) obj := &RObject{class: cls, ivars: map[string]object.Value{}} c.seen[o] = obj - for k, val := range o.IVars { - obj.ivars["@"+k] = c.conv(val) + // ivarOrder has to be filled alongside ivars, not left to the map. An + // *RObject's order list is consulted FIRST by instance_variables, and the + // pointer to it is never nil, so an object built with a populated map and an + // empty order list has ivars that read back by name and do not appear in the + // enumeration -- #787, where a YAML-loaded object answered `o.a == 1` and + // `instance_variables == []`. + // + // The sequence is the library's own emission order (value.go:86-89): the + // names in o.Order that exist, then the rest lexicographically. Taking it + // from there rather than ranging the map keeps the result both MRI-like + // (document order) and stable across runs. + // No presence check on the lookup: yamlIVarOrder returns only keys that are + // in o.IVars, so a missing-key arm here would be dead code the coverage gate + // would then ask about -- and it did, on the first version of this. + for _, k := range yamlIVarOrder(o) { + obj.ivars["@"+k] = c.conv(o.IVars[k]) + obj.ivarOrder = append(obj.ivarOrder, "@"+k) } return obj } diff --git a/scripts/conformance/rubyspec/BASELINE b/scripts/conformance/rubyspec/BASELINE index 61ecb187..f8dbcdcd 100644 --- a/scripts/conformance/rubyspec/BASELINE +++ b/scripts/conformance/rubyspec/BASELINE @@ -18,6 +18,22 @@ # regression, no new file, no change of loading -- so the deltas are the whole # move. # +# ⛔ 2026-10-09: ONE of those twenty was wrong to apply, and this file said so. +# core/random/bytes_spec.rb went 8 -> 9, and it is the KNOWN FLAKE held at its +# observed minimum (see "One file is recorded at its OBSERVED MINIMUM" below, +# and the two earlier waves that applied deltas and deliberately left it alone). +# The next run read 8 and the lane went red, which is the coin toss that note +# predicts in those words. Put back to 8. +# +# All three checks that commit lists were ARITHMETIC -- sums and columns -- and +# every one of them passed. None of them asked whether a number SHOULD move, +# which is the question this file answers in its own header, three times over, +# about this exact path. Measured here afterwards, same binary, six runs: +# 9 9 9 9 8 9. +# +# So: when applying a ratchet delta, read the header for the FILE being moved +# before moving it. The sum being right is not evidence that the move was. +# # THE FRAME MATTERS, twice over, and both are why an earlier attempt at this # file was wrong: # @@ -1630,7 +1646,7 @@ OK core/queue/pop_spec.rb 20 OK core/queue/push_spec.rb 1 OK core/queue/shift_spec.rb 1 OK core/queue/size_spec.rb 1 -OK core/random/bytes_spec.rb 9 +OK core/random/bytes_spec.rb 8 OK core/random/default_spec.rb 1 OK core/random/equal_value_spec.rb 2 OK core/random/new_seed_spec.rb 3