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) + } + }) + } +}