From 5c45fec03204e578110f9d43afd4398026712f55 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 5 Nov 2026 13:30:03 +0100 Subject: [PATCH] Finish the message-quoting migration, and gate it MRI stopped writing `name' and writes 'name' since 3.4. v4.0.9 is straight-quoted throughout -- numeric.c:3207 "integer %lu too big to convert to 'unsigned int'", file.c:3908 "couldn't find HOME environment -- expanding '~'", both read from the tag. rbgo migrated most of its messages and missed thirteen sites. It was only visible because the SAME message ended up in both spellings in one package: builtins.go:3321 "bignum too big to convert into 'long'" arith.go:675 "bignum too big to convert into `long'" That is not a style choice, it is half a migration. ## What was missed arith.go:675 bignum too big to convert into `long' builtins.go:6028 bignum too big to convert into `long' builtins.go:10288 `%s' is not allowed as a class variable name dir.go:503 couldn't find HOME environment -- expanding `~' file.go:1010 couldn't find HOME environment -- expanding `~' ivar_reflect.go:71 `%s' is not allowed as an instance variable name modulefunc.go:534 `%s' is not allowed as a class variable name numeric_edges.go:49 integer %d too big to convert to `int' numeric_edges.go:53 bignum too big to convert into `long' oauth2_client.go:91 undefined method `authorize_url' for %s opentelemetry.go:116 undefined method `add_span_processor' for ... rspec.go:252 undefined method `of' for matcher rspec.go:260 undefined method `with' for matcher The last four imitate NoMethodError, which rbgo's own dispatcher already writes with straight quotes -- they disagreed with the engine they sit in. ## How it was found, and how the search was widened A probe of twelve NameError shapes against MRI 4.0.7 named FIVE. Fixing those five would have left eight, so the search was widened to every raise() in the package carrying a backtick: 47 lines, of which 13 were this family. ## The two tests moved with the quoting, not because they were wrong module_wave16_internal_test.go pinned `foo' and `@@'. Both now pin the 4.0 form with the two citations above beside them, so a reader finding this diff later knows the expectation changed because MRI's did. ## The gate A note saying "finish the migration" would be forgotten exactly the way the first half was, so message_quoting_test.go scans every non-test source file in the package and fails on the old form. It logs how many files it read (542) and refuses below 50: a scan that cannot say what it looked at cannot be told from a scan that could not look. ## The gate's own first version was wrong, and its test says so Matching the whole line reported string_undump.go twice. That message is a Go RAW string, where the backtick is the DELIMITER and not part of the message. A detector that cannot tell a delimiter from content reports defects that are not there, so the detector is now a function with six rows of its own -- including that raw string, prose that pairs its backticks, and an escaped quote inside a message. ## Measured rubydiff against /opt/homebrew/opt/ruby@4.0/bin/ruby on twelve NameError and RangeError shapes: DIVERGE on 5 before, AGREE after. go vet ./... clean. Co-Authored-By: Claude Opus 5 --- internal/vm/arith.go | 2 +- internal/vm/builtins.go | 4 +- internal/vm/dir.go | 2 +- internal/vm/file.go | 2 +- internal/vm/ivar_reflect.go | 2 +- internal/vm/message_quoting_test.go | 139 +++++++++++++++++++++ internal/vm/module_wave16_internal_test.go | 8 +- internal/vm/modulefunc.go | 2 +- internal/vm/numeric_edges.go | 4 +- internal/vm/oauth2_client.go | 2 +- internal/vm/opentelemetry.go | 2 +- internal/vm/rspec.go | 4 +- 12 files changed, 158 insertions(+), 15 deletions(-) create mode 100644 internal/vm/message_quoting_test.go diff --git a/internal/vm/arith.go b/internal/vm/arith.go index bfabb142..86b4ea75 100644 --- a/internal/vm/arith.go +++ b/internal/vm/arith.go @@ -672,7 +672,7 @@ func (vm *VM) repeatLong(b object.Value) int64 { if v.I.IsInt64() { return v.I.Int64() } - raise("RangeError", "bignum too big to convert into `long'") + raise("RangeError", "bignum too big to convert into 'long'") case object.Float: f := float64(v) if math.IsNaN(f) { diff --git a/internal/vm/builtins.go b/internal/vm/builtins.go index 3818ac0a..bc7ce107 100644 --- a/internal/vm/builtins.go +++ b/internal/vm/builtins.go @@ -6025,7 +6025,7 @@ func (vm *VM) bootstrap() { if i, ok := self.(object.Integer); ok { return int64(i) } - raise("RangeError", "bignum too big to convert into `long'") + raise("RangeError", "bignum too big to convert into 'long'") return 0 } vm.cInteger.defineArgc("abs", 0, func(_ *VM, self object.Value, _ []object.Value, _ *Proc) object.Value { @@ -10285,7 +10285,7 @@ func cvarNameArg(v object.Value) string { raise("TypeError", "%s is not a symbol nor a string", v.Inspect()) } if !strings.HasPrefix(name, "@@") || len(name) == 2 { - raise("NameError", "`%s' is not allowed as a class variable name", name) + raise("NameError", "'%s' is not allowed as a class variable name", name) } return name } diff --git a/internal/vm/dir.go b/internal/vm/dir.go index c0d43cc5..621e60fa 100644 --- a/internal/vm/dir.go +++ b/internal/vm/dir.go @@ -500,7 +500,7 @@ func dirHomeStr() string { if h, err := osCurrentUserHome(); err == nil && h != "" { return h } - raise("ArgumentError", "couldn't find HOME environment -- expanding `~'") + raise("ArgumentError", "couldn't find HOME environment -- expanding '~'") return "" } diff --git a/internal/vm/file.go b/internal/vm/file.go index 23ff1847..80809944 100644 --- a/internal/vm/file.go +++ b/internal/vm/file.go @@ -1007,7 +1007,7 @@ func expandTildePath(p string) string { if name == "" { // "~" or "~/..." -> $HOME home := os.Getenv("HOME") if home == "" { - raise("ArgumentError", "couldn't find HOME environment -- expanding `~'") + raise("ArgumentError", "couldn't find HOME environment -- expanding '~'") } if !isAbsPath(toSlash(home)) { raise("ArgumentError", "non-absolute home") diff --git a/internal/vm/ivar_reflect.go b/internal/vm/ivar_reflect.go index e7896845..41bcad66 100644 --- a/internal/vm/ivar_reflect.go +++ b/internal/vm/ivar_reflect.go @@ -68,7 +68,7 @@ func (vm *VM) ivarNameArg(arg object.Value) string { } } if !validIvarName(name) { - raise("NameError", "`%s' is not allowed as an instance variable name", name) + raise("NameError", "'%s' is not allowed as an instance variable name", name) } return name } diff --git a/internal/vm/message_quoting_test.go b/internal/vm/message_quoting_test.go new file mode 100644 index 00000000..c804f43c --- /dev/null +++ b/internal/vm/message_quoting_test.go @@ -0,0 +1,139 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import ( + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// oldQuoting is MRI's pre-3.4 form: a backtick, then at least one character +// that is neither a quote nor another backtick, then an apostrophe. +var oldQuoting = regexp.MustCompile("`[^`\"'\n]+'") + +// doubleQuotedSegments returns the contents of each "..." string on a line. +// +// ⛔ It exists because the first version of this gate matched the whole line and +// reported string_undump.go twice: that message is a Go RAW string, where the +// backtick is the delimiter and not part of the message at all. A detector that +// cannot tell a delimiter from content reports defects that are not there. +func doubleQuotedSegments(line string) []string { + var out []string + var cur strings.Builder + inside, escaped := false, false + for _, r := range line { + switch { + case escaped: + cur.WriteRune(r) + escaped = false + case inside && r == '\\': + escaped = true + case r == '"': + if inside { + out = append(out, cur.String()) + cur.Reset() + } + inside = !inside + case inside: + cur.WriteRune(r) + } + } + return out +} + +// ⛔ MRI stopped writing `name' and writes 'name' since 3.4; v4.0.9 is +// straight-quoted throughout (numeric.c:3207, file.c:3908). rbgo migrated most +// of its messages and missed thirteen, which was only visible because the SAME +// message existed in both spellings in this one package: +// +// builtins.go:3321 "bignum too big to convert into 'long'" +// arith.go:675 "bignum too big to convert into `long'" +// +// A note saying "finish the migration" would be forgotten exactly the way the +// first half was, so this is a gate instead. It reports the number of files it +// read: a scan that cannot say what it looked at cannot be told from a scan +// that could not look. +func TestErrorMessagesUseStraightQuotesNotTheOldBacktickForm(t *testing.T) { + entries, err := os.ReadDir(".") + if err != nil { + t.Fatal(err) + } + var offenders []string + read := 0 + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + b, err := os.ReadFile(name) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + read++ + for i, line := range strings.Split(string(b), "\n") { + if !strings.Contains(line, "raise(") { + continue + } + for _, seg := range doubleQuotedSegments(line) { + if oldQuoting.MatchString(seg) { + offenders = append(offenders, + filepath.Join("internal/vm", name)+":"+strconv.Itoa(i+1)+" "+strings.TrimSpace(line)) + break + } + } + } + } + // ⛔ The denominator. If a refactor moves these messages out of this package + // the scan would find nothing and quietly pass, so assert it read something. + if read < 50 { + t.Fatalf("only %d source files scanned — the gate is looking in the wrong place", read) + } + t.Logf("scanned %d non-test source files in internal/vm", read) + if len(offenders) > 0 { + t.Errorf("%d message(s) still use MRI's pre-3.4 `x' quoting; v4.0.9 writes 'x':\n%s", + len(offenders), strings.Join(offenders, "\n")) + } +} + +// The judge and its subject must count alike, so the detector is tested on the +// shapes it has to separate rather than only on the tree as it stands today. +func TestTheQuotingGateSeparatesAMessageFromADelimiter(t *testing.T) { + for _, tc := range []struct { + name string + line string + want bool + }{ + {"the old form inside a double-quoted message", + "\t\traise(\"NameError\", \"`%s' is not allowed as a class variable name\", name)", true}, + {"the new form", "\t\traise(\"NameError\", \"'%s' is not allowed as a class variable name\", name)", false}, + {"a backtick that delimits a Go RAW string is not content", + "\t\traise(\"RuntimeError\", `invalid dumped string; not wrapped with '\"' nor '\"...\"' form`)", false}, + {"prose that pairs its backticks", + "\traise(\"NotImplementedError\", \"`%%x`/backtick command execution is not supported on wasm\")", false}, + {"a line with no raise at all", "\t// see `foo' in the notes", false}, + {"an escaped quote does not end the segment", + "\t\traise(\"ArgumentError\", \"say \\\"`x'\\\" please\")", true}, + } { + t.Run(tc.name, func(t *testing.T) { + got := false + if strings.Contains(tc.line, "raise(") { + for _, seg := range doubleQuotedSegments(tc.line) { + if oldQuoting.MatchString(seg) { + got = true + break + } + } + } + if got != tc.want { + t.Errorf("flagged=%v want=%v for:\n%s\nsegments: %q", + got, tc.want, tc.line, doubleQuotedSegments(tc.line)) + } + }) + } +} diff --git a/internal/vm/module_wave16_internal_test.go b/internal/vm/module_wave16_internal_test.go index bf902371..fb80f9d5 100644 --- a/internal/vm/module_wave16_internal_test.go +++ b/internal/vm/module_wave16_internal_test.go @@ -33,8 +33,12 @@ func TestWave16NameCoercion(t *testing.T) { "can't convert Object to String (Object#to_str gives Integer)"}, {"Module.new.const_set(\"x\", 1)", "NameError", "wrong constant name x"}, {"Module.new.const_set(\"Name=\", 1)", "NameError", "wrong constant name Name="}, - {"Class.new.class_variable_get(:foo)", "NameError", "`foo' is not allowed as a class variable name"}, - {"Class.new.class_variable_get(:@@)", "NameError", "`@@' is not allowed as a class variable name"}, + // ⛔ These two moved with the quoting, not because they were wrong. MRI + // writes 'foo' and not `foo' since 3.4, and v4.0.9 is straight-quoted + // throughout (numeric.c:3207, file.c:3908). Measured on 4.0.7: + // "'nope' is not allowed as an instance variable name". + {"Class.new.class_variable_get(:foo)", "NameError", "'foo' is not allowed as a class variable name"}, + {"Class.new.class_variable_get(:@@)", "NameError", "'@@' is not allowed as a class variable name"}, } for _, c := range errs { if cls, msg := evalErr(t, c.src); cls != c.class || msg != c.msg { diff --git a/internal/vm/modulefunc.go b/internal/vm/modulefunc.go index 7d337659..3e062bf6 100644 --- a/internal/vm/modulefunc.go +++ b/internal/vm/modulefunc.go @@ -531,7 +531,7 @@ func (vm *VM) coerceConstName(v object.Value) string { func (vm *VM) coerceCvarName(v object.Value) string { name := vm.coerceNameArg(v) if len(name) < 3 || name[0] != '@' || name[1] != '@' { - raise("NameError", "`%s' is not allowed as a class variable name", name) + raise("NameError", "'%s' is not allowed as a class variable name", name) } return name } diff --git a/internal/vm/numeric_edges.go b/internal/vm/numeric_edges.go index f2431ac7..ba3b1d2c 100644 --- a/internal/vm/numeric_edges.go +++ b/internal/vm/numeric_edges.go @@ -46,11 +46,11 @@ func (vm *VM) toRoundInt(a object.Value) int { switch v := a.(type) { case object.Integer: if int64(v) < math.MinInt32 || int64(v) > math.MaxInt32 { - raise("RangeError", "integer %d too big to convert to `int'", int64(v)) + raise("RangeError", "integer %d too big to convert to 'int'", int64(v)) } return clampInt(v) case *object.Bignum: - raise("RangeError", "bignum too big to convert into `long'") + raise("RangeError", "bignum too big to convert into 'long'") case object.Float: f := math.Trunc(float64(v)) if math.IsNaN(float64(v)) || math.IsInf(float64(v), 0) || f < math.MinInt32 || f > math.MaxInt32 { diff --git a/internal/vm/oauth2_client.go b/internal/vm/oauth2_client.go index 8b50214a..2f5ffcdb 100644 --- a/internal/vm/oauth2_client.go +++ b/internal/vm/oauth2_client.go @@ -88,7 +88,7 @@ func (vm *VM) registerOAuth2StrategyMethods(cls *RClass) { cls.define("authorize_url", func(vm *VM, self object.Value, args []object.Value, _ *Proc) object.Value { s := self.(*OAuth2Strategy) if s.grant != "auth_code" { - raise("NoMethodError", "undefined method `authorize_url' for %s", s.className) + raise("NoMethodError", "undefined method 'authorize_url' for %s", s.className) } return object.NewString(s.client.AuthCode().AuthorizeURL(oauth2Params(args))) }) diff --git a/internal/vm/opentelemetry.go b/internal/vm/opentelemetry.go index 8a5c6612..fa976f2b 100644 --- a/internal/vm/opentelemetry.go +++ b/internal/vm/opentelemetry.go @@ -113,7 +113,7 @@ func (vm *VM) registerOpenTelemetryProvider(cls *RClass) { cls.define("add_span_processor", func(_ *VM, v object.Value, args []object.Value, _ *Proc) object.Value { p := v.(*OTelTracerProvider) if p.sdk == nil { - raise("NoMethodError", "undefined method `add_span_processor' for a non-SDK tracer provider") + raise("NoMethodError", "undefined method 'add_span_processor' for a non-SDK tracer provider") } if len(args) == 0 { raise("ArgumentError", "wrong number of arguments (given 0, expected 1)") diff --git a/internal/vm/rspec.go b/internal/vm/rspec.go index ac8ca23d..9fdbc869 100644 --- a/internal/vm/rspec.go +++ b/internal/vm/rspec.go @@ -249,7 +249,7 @@ func (vm *VM) registerRSpecMatcherClass(mod *RClass) { d("of", func(vm *VM, v object.Value, args []object.Value, _ *Proc) object.Value { m := self(v) if m.chain == nil || m.chain.within == nil { - raise("NoMethodError", "undefined method `of' for matcher") + raise("NoMethodError", "undefined method 'of' for matcher") } return &RSpecMatcher{m: rspec.BeWithin(m.chain.within.delta).Of(rspecFloatArg(args))} }) @@ -257,7 +257,7 @@ func (vm *VM) registerRSpecMatcherClass(mod *RClass) { d("with", func(vm *VM, v object.Value, args []object.Value, _ *Proc) object.Value { m := self(v) if m.chain == nil || m.chain.respTo == nil { - raise("NoMethodError", "undefined method `with' for matcher") + raise("NoMethodError", "undefined method 'with' for matcher") } n := clampInt(rspecIntArg(args)) return &RSpecMatcher{m: rspec.RespondTo(m.chain.respTo.names...).With(n)}