diff --git a/internal/aot/codegen.go b/internal/aot/codegen.go index 7e5a963a..5c37ce43 100644 --- a/internal/aot/codegen.go +++ b/internal/aot/codegen.go @@ -207,7 +207,10 @@ func (g *gen) emit(pc int) (string, bool) { case bytecode.OpSetConst: return line("vm.consts[%q] = s%d", g.iseq.Names[in.A], d-1), true case bytecode.OpGetGVar: - return line("s%d = vm.gvar(%q)", d, g.iseq.Names[in.A]), true + // gvarGet, not gvar: it is the Ruby-level read, so it carries MRI's + // uninitialised-global warning. Emitting gvar here would make the warning + // depend on whether the method had been compiled yet. + return line("s%d = vm.gvarGet(%q)", d, g.iseq.Names[in.A]), true case bytecode.OpSplatToArray: return line("s%d = vm.aotSplat(s%d)", d-1, d-1), true case bytecode.OpConcatArray: diff --git a/internal/aot/codegen_test.go b/internal/aot/codegen_test.go index 7f0bccad..00f40e92 100644 --- a/internal/aot/codegen_test.go +++ b/internal/aot/codegen_test.go @@ -72,7 +72,7 @@ func TestCompileSupported(t *testing.T) { {"setivar", "def m(a)\n @x = a\nend\nm(1)", "m", "setIvar(self,"}, {"getconst", "def m = Foo\nm", "m", "vm.aotConst("}, {"setconst", "def m\n X = 1\n X\nend\nm", "m", "vm.consts["}, - {"getgvar", "def m = $g\nm", "m", "vm.gvar("}, + {"getgvar", "def m = $g\nm", "m", "vm.gvarGet("}, {"splat_concat", "def m(a, b) = [a, *[b, b]]\nm(1, 2)", "m", "aotConcat("}, {"splat", "def m(a, b) = [a, *[b, b]]\nm(1, 2)", "m", "aotSplat("}, {"regexp", "def m = /ab/\nm", "m", "compileLiteralRegexp("}, diff --git a/internal/aot/level2.go b/internal/aot/level2.go index f12dd865..5c26bffd 100644 --- a/internal/aot/level2.go +++ b/internal/aot/level2.go @@ -213,7 +213,9 @@ func (s *mscope) emit(pc int) (string, bool) { case bytecode.OpSetConst: return line("vm.consts[%q] = %s", s.iseq.Names[in.A], s.sv(d-1)), true case bytecode.OpGetGVar: - return line("%s = vm.gvar(%q)", s.sv(d), s.iseq.Names[in.A]), true + // gvarGet, not gvar — the Ruby-level read, with MRI's warning. See the + // same arm in codegen.go. + return line("%s = vm.gvarGet(%q)", s.sv(d), s.iseq.Names[in.A]), true case bytecode.OpReturn: if in.A != 0 { return "", false // an explicit non-local `return` from a block: not lowered diff --git a/internal/aot/level2_test.go b/internal/aot/level2_test.go index b090fa93..241adb38 100644 --- a/internal/aot/level2_test.go +++ b/internal/aot/level2_test.go @@ -48,7 +48,7 @@ func TestCompileMainSupported(t *testing.T) { {"setivar", "@x = 3; puts @x", `setIvar(self, "@x"`}, {"setconst", "K = 9; puts K", `vm.consts["K"]`}, {"getconst", "K = 9; puts K", `vm.aotConst("K")`}, - {"gvar", "puts $0", `vm.gvar("$0")`}, + {"gvar", "puts $0", `vm.gvarGet("$0")`}, {"send_noblock", "puts [3, 1, 2].sort.first", "vm.aotSend(&aotic"}, {"send_block", "t=0; 3.times { |i| t += i }; puts t", "&Proc{native: func(vm *VM, bargs"}, {"block_outerlocal", "t=0; 3.times { |i| t += i }; puts t", "l0_0"}, diff --git a/internal/vm/globals.go b/internal/vm/globals.go index 97f69079..fa71b7c4 100644 --- a/internal/vm/globals.go +++ b/internal/vm/globals.go @@ -2,10 +2,47 @@ package vm import ( "os" + "strings" "github.com/go-embedded-ruby/ruby/internal/object" ) +// mriPredefinedGvars is the set of global names the INTERPRETER defines, each +// of which therefore has a real getter and never reports itself uninitialised. +// gvarGet consults it, and it says something about MRI, not about this VM: nine +// of these names (`$;`, `$\`, `$,`, `$.`, `$_`, `$?`, `$FILENAME`, `$-F`, `$-i`) +// are not modelled here and read as nil, and that gap is a separate, older +// divergence. Conflating the two would turn a warning about a PROGRAM's variable +// into a report about this VM's coverage. +// +// ⛔ The list is measured, not remembered. An earlier draft of the warning was +// built from the names I could recall and it jabbered at every one of those +// nine, because "rbgo falls through to the user-global path" and "MRI has no +// getter" are different statements and only the second one licenses a warning. +// The enumeration is `global_variables` taken from the reference interpreter at +// startup (the installed /usr/bin/ruby, 2.6.10), minus `$SAFE`, `$KCODE` and +// `$-K`, which Ruby 3.0 removed, and minus the two names the probe script +// itself mentioned — `global_variables` lists every entry the parser CREATED, +// so merely naming a variable puts it in the answer. +// +// Reading the set from an interpreter older than the 4.0.9 we target is sound in +// this one direction, which is why it is allowed to stand: a name listed here +// that 4.0 has since dropped only costs a warning we do not print, while a name +// MISSING here costs a warning MRI never prints. The error that matters is the +// second one, and an over-complete list cannot make it. +var mriPredefinedGvars = map[string]bool{ + "$!": true, "$\"": true, "$$": true, "$&": true, "$'": true, + "$*": true, "$+": true, "$,": true, "$-0": true, "$-F": true, + "$-I": true, "$-W": true, "$-a": true, "$-d": true, "$-i": true, + "$-l": true, "$-p": true, "$-v": true, "$-w": true, "$.": true, + "$/": true, "$0": true, "$:": true, "$;": true, "$<": true, + "$=": true, "$>": true, "$?": true, "$@": true, "$DEBUG": true, + "$FILENAME": true, "$LOADED_FEATURES": true, "$LOAD_PATH": true, + "$PROGRAM_NAME": true, "$VERBOSE": true, "$\\": true, "$_": true, + "$`": true, "$stderr": true, "$stdin": true, "$stdout": true, + "$~": true, +} + // englishAlias maps the English library's readable global names to the cryptic // special they alias (require "English"). The aliases are always recognised: // reading one resolves the underlying special, so $ERROR_INFO and $! name the @@ -246,6 +283,58 @@ func (vm *VM) warnDeprecatedGvar(msg string) { vm.send(w, "warn", []object.Value{object.NewString("warning: " + msg + "\n")}, nil) } +// gvarGet is variable.c's rb_gvar_get (variable.c:1052 at v4.0.9): the +// RUBY-LEVEL read of a global, which is gvar plus the warning MRI emits for a +// global nothing ever assigned. The getgvar instruction and the two AOT tiers +// that compile it are its only callers, and that split is upstream's own. +// +// rb_gvar_get resolves the name to an entry and calls that entry's getter. A +// name nobody has assigned gets rb_gvar_undef_getter installed for it +// (variable.c:649), and the whole body of that getter is +// +// rb_warning("global variable '%"PRIsVALUE"' not initialized", QUOTE_ID(id)); +// return Qnil; +// +// so what is being reported is "never assigned", NOT "reads as nil": assigning +// replaces the getter, and `$x = nil; $x` is silent for good afterwards. It is +// rb_warning rather than rb_warn, so only a TRUE $VERBOSE prints it — which is +// why language/variables_spec.rb asks for it through `complain(verbose: true)`. +// +// ⛔ The internal reads must NOT come through here, and upstream's do not. This +// VM reads $_, $/ and $; with vm.gvar from print/gets/split and copies a value +// with it for `alias $new $old`; the C code those mirror touches +// rb_lastline_get, rb_rs and rb_fs directly instead of calling rb_gv_get, so +// none of them warns. Route one of them through gvarGet and every `puts` in a +// `ruby -w` run grows a warning about $_. +// +// The gate is first because it is one map lookup and false in any ordinary run, +// so the name resolution is walked once and the message is composed never. +func (vm *VM) gvarGet(name string) object.Value { + // ⛔ bytecode.ErrinfoCell ("#$!") is not a Ruby global, it is the cell the + // begin/rescue compiler reads the pending exception through, and it is spelled + // with a '#' precisely so no program can name it. vm.go's getgvar arm + // intercepts it before reaching here; the two AOT tiers do NOT, so without + // this line every compiled `rescue` would report an uninitialised global under + // `ruby -w`. No Ruby global can start with '#', so the whole family is covered + // rather than the one spelling that exists today. + if !vm.warningEnabled() || strings.HasPrefix(name, "#") { + return vm.gvar(name) + } + if mriPredefinedGvars[name] || englishAlias[name] != "" { + // A name the interpreter defines has a getter, so it is never + // uninitialised however little of it this VM models. The English library's + // readable aliases resolve to one of those names, so they inherit that. + return vm.gvar(name) + } + v, uninitialized := vm.gvarResolve(name) + if uninitialized { + // QUOTE_ID quotes the id rb_gvar_get was CALLED with, so the message + // names the spelling the program read, not a canonical slot. + vm.rbWarningf("global variable '%s' not initialized", name) + } + return v +} + // setGVar stores a global, applying the checks MRI's setter for that name // applies: a read-only variable raises NameError, the I/O-formatting specials // take only a String (or, for $;, a Regexp), $. takes anything #to_int accepts, diff --git a/internal/vm/gvar_uninitialized_warning_test.go b/internal/vm/gvar_uninitialized_warning_test.go new file mode 100644 index 00000000..39700a76 --- /dev/null +++ b/internal/vm/gvar_uninitialized_warning_test.go @@ -0,0 +1,143 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package vm + +import ( + "sort" + "strings" + "testing" +) + +// Reading a global nothing ever assigned warns, and the three things that are +// NOT that read stay silent. variable.c at v4.0.9 gives the mechanism: an id +// nobody has assigned gets rb_gvar_undef_getter installed for it +// (variable.c:649), whose whole body is +// +// rb_warning("global variable '%"PRIsVALUE"' not initialized", QUOTE_ID(id)); +// return Qnil; +// +// so the subject is "never assigned", not "reads as nil" — and it is rb_warning, +// not rb_warn, so only a TRUE $VERBOSE prints it. language/variables_spec.rb +// asks for it through `complain(verbose: true)` for exactly that reason. +// +// Every row was run through /usr/bin/ruby on the same source before it was +// written here. +func TestUninitializedGlobalWarning(t *testing.T) { + const probe = warnProbeFile + cases := []struct { + name, src, want string + }{ + { + name: "never assigned warns and names the variable", + src: "$VERBOSE = true\n$specs_uninitialized_global_variable\n", + want: probe + ":2: warning: global variable '$specs_uninitialized_global_variable' not initialized\n", + }, { + // ⛔ The distinction the whole fix turns on. Assigning replaces the + // undef getter, so the variable is initialised FOREVER after, even + // though every read of it still answers nil. + name: "assigned nil is not uninitialized", + src: "$VERBOSE = true\n$q = nil\n$q\n", + want: "", + }, { + // The getter is still rb_gvar_undef_getter on the second read, so MRI + // warns again rather than deduplicating. Measured: two lines. + name: "each read warns", + src: "$VERBOSE = true\n$twice\n$twice\n", + want: probe + ":2: warning: global variable '$twice' not initialized\n" + + probe + ":3: warning: global variable '$twice' not initialized\n", + }, { + // rb_warning's gate is RTEST(ruby_verbose): a FALSE $VERBOSE is the + // default and is already silent. This is the row that would catch the + // warning being wired to rb_warn by mistake. + name: "silent when $VERBOSE is false", + src: "$VERBOSE = false\n$quiet_one\n", + want: "", + }, { + name: "silent when $VERBOSE was never set", + src: "$quiet_two\n", + want: "", + }, { + name: "silent when $VERBOSE is nil", + src: "$VERBOSE = nil\n$quiet_three\n", + want: "", + }, { + // defined? compiles to its own instruction (OpDefinedGVar), so asking + // whether a global exists must not report that it does not. + name: "defined? does not warn", + src: "$VERBOSE = true\np defined?($not_there)\n", + want: "nil\n", + }, { + // ⛔ bytecode.ErrinfoCell ("#$!") is the cell begin/rescue reads the + // pending exception through, not a variable any program can name. The + // interpreter intercepts it before the read; the two AOT tiers do not, + // so without the '#' guard in gvarGet every compiled rescue would + // report an uninitialised global here. + name: "a rescue body is silent", + src: "$VERBOSE = true\nbegin\n raise \"x\"\nrescue => e\n e.message\nend\n", + want: "", + }, { + // The match-data family resolves through getters MRI defines, so no + // match means a nil result, not an uninitialised variable. + name: "match-data globals with no match are silent", + src: "$VERBOSE = true\np [$~, $&, $1, $`, $', $+]\n", + want: "[nil, nil, nil, nil, nil, nil]\n", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := runWarn(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + }) + } +} + +// ⛔ THE NINE NAMES AN EARLIER DRAFT OF THIS WARNING JABBERED AT. +// +// Measured, not remembered: the first version of the fix warned for `$;`, `$\`, +// `$,`, `$.`, `$_`, `$?`, `$FILENAME`, `$-F` and `$-i`, because this VM leaves +// them on the ordinary user-global path and the warning asked THIS VM whether a +// name was known. MRI defines all nine in the interpreter, so each has a real +// getter and none of them can be uninitialised; running the same probe through +// /usr/bin/ruby printed nothing for any of them. +// +// "rbgo falls through to the user-global path" and "MRI has no getter" are +// different statements, and only the second licenses a warning. That those nine +// read as nil here is a separate and older divergence, and a warning is the +// wrong channel to report it through: it would turn a diagnostic about the +// PROGRAM's variable into a report about our own coverage. +// +// This walks the whole table rather than the nine, so a name added to it is +// covered the day it is added. +func TestPredefinedGlobalsNeverReportUninitialized(t *testing.T) { + names := make([]string, 0, len(mriPredefinedGvars)) + for name := range mriPredefinedGvars { + names = append(names, name) + } + sort.Strings(names) + if len(names) < 40 { + t.Fatalf("the predefined table holds %d names, which is too few to be MRI's set", len(names)) + } + for _, name := range names { + t.Run(name, func(t *testing.T) { + got := runWarn(t, "$VERBOSE = true\n"+name+"\n") + if strings.Contains(got, "not initialized") { + t.Errorf("reading %s reported it uninitialised: %q", name, got) + } + }) + } +} + +// The control for the test above: it must be able to SEE the thing it looks for. +// A predicate that answered "no warning" for every input would pass that test +// with the fix removed, so this plants a positive in the same harness, with the +// same assertion, and fails if the warning cannot be observed at all. +func TestUninitializedGlobalWarningIsObservable(t *testing.T) { + got := runWarn(t, "$VERBOSE = true\n$a_name_in_no_table\n") + if !strings.Contains(got, "not initialized") { + t.Fatalf("the planted positive produced no warning: %q — the other test's "+ + "silence proves nothing", got) + } +} diff --git a/internal/vm/regexp.go b/internal/vm/regexp.go index ae52d9c8..f3f4f05a 100644 --- a/internal/vm/regexp.go +++ b/internal/vm/regexp.go @@ -1532,9 +1532,27 @@ func (vm *VM) strRindexRegexp(re *Regexp, subject, enc string, limit int) object // gvar reads a global variable. The match-data specials derive from $~ (the // last match); any other name reads as nil (uninitialised global). +// +// This is the INTERNAL accessor, the one that never warns. The Ruby-level read +// is gvarGet, and the difference is explained there. func (vm *VM) gvar(name string) object.Value { + v, _ := vm.gvarResolve(name) + return v +} + +// gvarResolve is gvar plus the one thing a caller cannot otherwise recover: a +// flag saying the name fell through to the ordinary-user-global branch and +// nothing had ever been assigned to it. Only that branch is "uninitialised"; +// `$&` without a match and `$1` outside a match resolve to nil through getters +// MRI defines, so they are not. +// +// It exists because the warning needs to know WHICH branch answered, and +// re-deriving that from outside would mean a second pass over specialGvar — a +// second name table to keep in step, and a second firing of the `$=` +// deprecation warning, which specialGvar emits as a side effect. +func (vm *VM) gvarResolve(name string) (object.Value, bool) { if v, handled := vm.specialGvar(name); handled { - return v + return v, false } // English match-data aliases ($MATCH -> $&, …) rewrite to the cryptic form so // the match-data resolution below applies (specialGvar reported them @@ -1547,21 +1565,21 @@ func (vm *VM) gvar(name string) object.Value { last = object.NilV } if name == "$~" { - return last + return last, false } md, ok := last.(*MatchData) switch name { case "$&": if ok { - return md.mdString(md.md.Str(0)) + return md.mdString(md.md.Str(0)), false } case "$`": if ok { - return md.mdString(md.md.Pre()) + return md.mdString(md.md.Pre()), false } case "$'": if ok { - return md.mdString(md.md.Post()) + return md.mdString(md.md.Post()), false } case "$+": // The last capture group that participated (highest-numbered match), or @@ -1569,23 +1587,24 @@ func (vm *VM) gvar(name string) object.Value { if ok { for i := md.md.NGroups(); i >= 1; i-- { if md.md.Begin(i) >= 0 { - return md.mdString(md.md.Str(i)) + return md.mdString(md.md.Str(i)), false } } } default: if n, isGroup := gvarGroup(name); isGroup { if ok && n <= md.md.NGroups() { - return groupValue(md, n) + return groupValue(md, n), false } - return object.NilV + return object.NilV, false } // Any other name is an ordinary user global: nil until assigned. if v, set := vm.globals[name]; set { - return v + return v, false } + return object.NilV, true } - return object.NilV + return object.NilV, false } // gvarGroup parses "$N" (N a positive integer) into its group number. diff --git a/internal/vm/vm.go b/internal/vm/vm.go index cf69be33..02060b7a 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -2469,7 +2469,7 @@ func (vm *VM) exec(iseq *bytecode.ISeq, self object.Value, args []object.Value, push(vm.curExc) } } else { - push(vm.gvar(name)) + push(vm.gvarGet(name)) } case bytecode.OpSetGVar: // Assignment is an expression: set the global, keep its value.