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 b64e8ec5..d7983b86 100644 --- a/internal/vm/builtins.go +++ b/internal/vm/builtins.go @@ -6036,7 +6036,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 { @@ -10359,7 +10359,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)}