From b443c04cf9f711502faefc252f3e3f61a721e1e2 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 5 Nov 2026 12:01:14 +0100 Subject: [PATCH] The separator globals accept a String subclass, as MRI's T_STRING test does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #884. `language/predefined_spec.rb` goes 143 → 145, errors 6 → 4. ## How this was found By counting, not by reading the backlog. #799's table credits #886 with 4 examples of `language/predefined_spec.rb`; running the file against current main says otherwise: RBGO_RESULT pass=143 fail=16 error=6 RBGO_DETAIL error TypeError Predefined global $/ | makes a new frozen String from the assigned String value of $/ must be String RBGO_DETAIL error TypeError Predefined global $-0 | makes a new frozen String from the assigned String value of $-0 must be String #886 closed TWO of the four, not four, and said so at the time — the String subclass example was named in its body as a known limitation and filed as #884. The tally in the issue kept the old number, and a pass count cannot tell the difference. ## The cause is one assertion, and it was in five places if _, ok := v.(*object.String); !ok { raise("TypeError", ...) } An exact-type assertion. MRI's are RB_TYPE_P(val, T_STRING) in rb_str_setter and rb_check_string_type in rb_fs_check, and both are TRUE for a subclass. rbgo represents an instance of `Class.new(String)` as an RObject wrapping a String, so the assertion missed every one of them. stringTyped is that test. ## The five globals do not do the same thing afterwards Measured on MRI 4.0.7, assigning the same `Class.new(String).new("abc")` with an @ivar: | global | setter | result | |---|---|---| | `$/`, `$-0` | deprecated_rs_setter | a frozen BARE String — class String, no ivar | | `$,`, `$\` | rb_str_setter | the object as handed over, unfrozen | | `$;` | rb_fs_setter | the object as handed over, unfrozen | A fix that froze and copied everywhere would pass the two spec examples and silently rewrite the other three. The test table asserts each separately. ## $; was wrong differently, and worse It did not refuse the subclass — it fell through to #to_str, which a String subclass answers. The assignment SUCCEEDED and quietly replaced the object with a plain String. The others at least raised. ## frozenBareString's comment had outlived its reason It said only the ivar half of BARE_STRING_P was tested, because a subclass "never reaches this function at all". That was true while checkStrGvar rejected it. Now it does reach it, and without the class test the subclass instance would be returned AS IS — ivar and all. The function takes the original value as well as the String inside it, and tests both halves. ## Measured rubydiff against MRI 4.0.7 on all five globals plus the non-String refusals: AGREE. specdelta, provenanced, over language, core/string, core/io and core/kernel: 442 files, control 9666, treatment 9668, +2, 1 file moved, 0 regressed. Co-Authored-By: Claude Opus 5 --- internal/vm/globals.go | 83 ++++++++++++++++------ internal/vm/separator_subclass_test.go | 98 ++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 22 deletions(-) create mode 100644 internal/vm/separator_subclass_test.go diff --git a/internal/vm/globals.go b/internal/vm/globals.go index 9826458c..64715dbb 100644 --- a/internal/vm/globals.go +++ b/internal/vm/globals.go @@ -453,13 +453,38 @@ func (vm *VM) checkStrGvar(name string, v object.Value) object.Value { if object.IsNil(v) { return object.NilV } - if _, ok := v.(*object.String); !ok { + if _, ok := stringTyped(v); !ok { raise("TypeError", "value of %s must be String", name) } vm.warnDeprecatedGvar("non-nil '" + name + "' is deprecated") + // MRI's rb_str_setter stores what it was handed, subclass and all: + // `$, = Class.new(String).new("x")` keeps that object, unfrozen. Only $/ + // and $-0 replace it, and they do it in checkRecordSepGvar. return v } +// stringTyped is RB_TYPE_P(val, T_STRING): it reports whether v is a String, +// INCLUDING an instance of a String subclass — which rbgo represents as an +// RObject wrapping one, not as an *object.String. (regexp.go's stringLike is a +// different question: it coerces a Symbol too, and answers with the bytes.) +// +// ⛔ Every separator global refused `Class.new(String).new("x")` because the +// test here was `v.(*object.String)`, an exact-type assertion. MRI's are +// RB_TYPE_P(val, T_STRING) in rb_str_setter and rb_check_string_type in +// rb_fs_check, and BOTH are true for a subclass. Measured on MRI 4.0.7: $/, +// $-0, $,, $\ and $; all accept one, and only $/ and $-0 convert it. +func stringTyped(v object.Value) (*object.String, bool) { + switch s := v.(type) { + case *object.String: + return s, true + case *RObject: + if b, ok := s.builtin.(*object.String); ok { + return b, true + } + } + return nil, false +} + // checkRecordSepGvar applies io.c v4.0.9 deprecated_rs_setter to $/ and $-0, // which from Ruby 4.0 no longer store the object they were handed: // @@ -486,7 +511,7 @@ func (vm *VM) checkRecordSepGvar(name string, v object.Value) object.Value { if object.IsNil(v) { return v } - s := v.(*object.String) + s, _ := stringTyped(v) // checkStrGvar has already refused anything else // rb_str_equal against a one-byte US-ASCII "\n": byte equality decides it. // Any ASCII-compatible encoding holding that single byte IS == "\n", and an // ASCII-incompatible one (UTF-16, UTF-32) cannot hold it in one byte at all, @@ -494,7 +519,7 @@ func (vm *VM) checkRecordSepGvar(name string, v object.Value) object.Value { if s.Str() == defaultRecordSeparator.Str() { return defaultRecordSeparator } - return frozenBareString(s) + return frozenBareString(v, s) } // frozenBareString is string.c v4.0.9 rb_str_frozen_bare_string: @@ -508,18 +533,24 @@ func (vm *VM) checkRecordSepGvar(name string, v object.Value) object.Value { // and a frozen plain String passes both, which is precisely what the three // examples in language/predefined_spec.rb distinguish. // -// ⛔ ONLY THE IVAR HALF IS TESTED HERE, and that is not an omission. In rbgo a +// ⛔ BOTH HALVES ARE TESTED, and they are tested on DIFFERENT values. orig is +// what Ruby handed over — an *object.String, or an RObject wrapping one when +// the assignment came from a String subclass — and s is the String inside it. +// The class half is `orig` being an *object.String at all, because in rbgo a // *object.String is class String by construction (classOf maps the Go type -// straight to vm.cString), and an instance of a String SUBCLASS never reaches -// this function at all — checkStrGvar's type assertion rejects it first, with -// "value of $/ must be String". MRI accepts one, because its check is T_STRING -// and a subclass is still T_STRING. That divergence is a defect of its own, -// filed rather than papered over here: it is why -// predefined_spec.rb's "makes a new frozen String from the assigned String" -// still fails, and fixing it belongs with the assertion, not with this rule. -func frozenBareString(s *object.String) *object.String { - if s.Frozen && genericIvarsOf(s, false) == nil { - return s +// straight to vm.cString) while a subclass instance is an RObject. The ivar +// half is read from s. +// +// Until the subclass was accepted at all this function only ever saw the exact +// type, so the class half had nothing to test and was left out with a note +// saying so. The note outlived its reason: `checkStrGvar` now admits a +// subclass, and without the `orig` test the subclass instance would be +// returned AS IS — frozen or not, @ivar and all — which is the opposite of +// what predefined_spec.rb's "makes a new frozen String from the assigned +// String" asserts. +func frozenBareString(orig object.Value, s *object.String) *object.String { + if bare, ok := orig.(*object.String); ok && bare.Frozen && genericIvarsOf(bare, false) == nil { + return bare } out := object.NewString(s.Str()) out.Enc = s.Enc @@ -535,16 +566,24 @@ func (vm *VM) checkFieldSepGvar(name string, v object.Value) object.Value { if object.IsNil(v) { return object.NilV } - switch v.(type) { - case *object.String, *Regexp: - default: - if vm.respondsToDynamic(v, "to_str") { - if s, ok := vm.send(v, "to_str", nil, nil).(*object.String); ok { - v = s - break + // ⛔ rb_check_string_type answers YES for a String subclass and hands the + // object back unchanged, so `$; = Class.new(String).new("x")` keeps that + // object in MRI. Testing only *object.String sent it down the #to_str path + // instead, which a String subclass answers — so the assignment succeeded + // and silently replaced the object with a plain String. A wrong answer is + // worse here than the TypeError the other separators gave. + if _, ok := stringTyped(v); !ok { + switch v.(type) { + case *Regexp: + default: + if vm.respondsToDynamic(v, "to_str") { + if s, ok := vm.send(v, "to_str", nil, nil).(*object.String); ok { + v = s + break + } } + raise("TypeError", "value of %s must be String or Regexp", name) } - raise("TypeError", "value of %s must be String or Regexp", name) } vm.warnDeprecatedGvar("non-nil '" + name + "' is deprecated") return v diff --git a/internal/vm/separator_subclass_test.go b/internal/vm/separator_subclass_test.go new file mode 100644 index 00000000..37df54cc --- /dev/null +++ b/internal/vm/separator_subclass_test.go @@ -0,0 +1,98 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import "testing" + +// ⛔ Every separator global refused an instance of a String SUBCLASS. The test +// was `v.(*object.String)`, an exact-type assertion, where MRI's are +// RB_TYPE_P(val, T_STRING) (rb_str_setter) and rb_check_string_type +// (rb_fs_check) — both true for a subclass. rbgo represents such an instance as +// an RObject wrapping a String, so the assertion missed every one of them. +// +// The five globals do NOT all do the same thing with it afterwards, and that is +// the point of the table: $, $\ and $; keep the object as handed over, while $/ +// and $-0 replace it with a frozen plain String. Every row was measured against +// MRI 4.0.7 on this host. +func TestSeparatorGlobalsAcceptAStringSubclass(t *testing.T) { + const mk = ` +sc = Class.new(String) +str = sc.new("abc") +str.instance_variable_set(:@ivar, 1) +` + for _, tc := range []struct{ name, src, want string }{ + // $/ and $-0: deprecated_rs_setter makes a frozen BARE String, so the + // class becomes String and the ivar is gone. These two rows are + // language/predefined_spec.rb's "makes a new frozen String from the + // assigned String", one per global. + {"$/ converts a subclass to a frozen bare String", + mk + `$/ = str +p [$/.instance_of?(String), $/.frozen?, $/.instance_variable_defined?(:@ivar), $/ == str]`, + `[true, true, false, true]`}, + {"$-0 converts a subclass to a frozen bare String", + mk + `$-0 = str +p [$-0.instance_of?(String), $-0.frozen?, $-0.instance_variable_defined?(:@ivar), $-0 == str]`, + `[true, true, false, true]`}, + + // ⛔ $, $\ and $; must NOT convert: rb_str_setter stores what it was + // handed. A fix that froze-and-copied everywhere would pass the two + // rows above and silently rewrite these three. + {"$, keeps the subclass object", + mk + `$, = str +p [$,.equal?(str), $,.frozen?]`, + `[true, false]`}, + {"$\\ keeps the subclass object", + mk + `$\ = str +p [$\.equal?(str), $\.frozen?]`, + `[true, false]`}, + {"$; keeps the subclass object", + mk + `$; = str +p [$;.equal?(str), $;.frozen?]`, + `[true, false]`}, + + // ⛔ $; went down a DIFFERENT wrong path from the others: not a + // TypeError but #to_str, which a String subclass answers — so the + // assignment succeeded and quietly replaced the object with a plain + // String. A wrong answer, where the others at least refused. + {"$; no longer silently converts through #to_str", + mk + `$; = str +p $;.class.equal?(sc)`, + `true`}, + + // Controls: the refusals that must survive. + {"$/ still refuses a non-String", + `begin; $/ = 1; rescue TypeError => e; p e.message; end`, + `"value of $/ must be String"`}, + {"$, still refuses a non-String", + `begin; $, = 1; rescue TypeError => e; p e.message; end`, + `"value of $, must be String"`}, + {"$; still refuses a non-String non-Regexp", + `begin; $; = 1; rescue TypeError => e; p e.message; end`, + `"value of $; must be String or Regexp"`}, + {"$; still accepts a Regexp", `$; = /x/; p $;`, `/x/`}, + {"$; still accepts a #to_str duck", + `class D; def to_str = "dd"; end +$; = D.new; p $;`, `"dd"`}, + {"nil is still accepted", `$/ = nil; p $/`, `nil`}, + + // The frozen-bare short-circuit still applies to an exact String, and + // the default separator still wins by identity over it. + {"a frozen bare String is kept by identity", + `s = "abc".freeze; $/ = s; p $/.equal?(s)`, `true`}, + {"the default separator wins over a frozen bare equal String", + `was = $/; s = "\n".dup.freeze; $/ = s; p [$/.equal?(s), $/.equal?(was)]`, + `[false, true]`}, + {"an exact String with an ivar is copied", + `s = "abc".dup; s.instance_variable_set(:@i, 1); $/ = s +p [$/.equal?(s), $/.frozen?, $/.instance_variable_defined?(:@i)]`, + `[false, true, false]`}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := runSrc(t, tc.src); got != tc.want { + t.Errorf("%s\n got %s\nwant %s", tc.src, got, tc.want) + } + }) + } +}