From 8171164a52e968a5acffaef4aceff6f6a6172bb7 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 1 Nov 2026 09:55:08 +0100 Subject: [PATCH 1/3] compiler: assigning a constant inside a method body is a SyntaxError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ```console $ rbgo -e 'def d; FOO = 1; end' # on main $ # nothing ``` MRI refuses that at parse time with `dynamic constant assignment`, and has for many versions. rbgo refused no form of it. ## It has its own spec, in a file nobody had connected to this `language/constants_spec.rb` fails on `Assignment dynamic assignment | raises SyntaxError` — measured `pass=88 fail=8` → **`pass=89 fail=7`** with this change, a gain this branch was not aiming at. I reached the defect sideways, from #799's fifth item, which I had written down as *"a non-ASCII upcased character must be rejected as a constant identifier"*. That title points at the lexer. The spec behind it is ```ruby -> { eval("def test\n ἍBB = 1\nend") }.should.raise(SyntaxError, /dynamic constant assignment/) ``` so what it needs is this check, plus a lexer that classifies `ἍBB` as a constant at all (go-ruby-parser#57 — rbgo's `defined?(ἍDD)` is nil where MRI says "constant"). Running the simplest version of the example — `def t; FOO_X = 1; end`, three words shorter — is what showed the real defect was ASCII-wide and in the compiler. Corrected on #799. ## The boundary is syntactic, and two measured rows prove it | | MRI | rbgo before | |---|---|---| | `def d; Class.new { FOO = 1 }; end` | refused | — | | `class C; define_method(:m) { FOO = 1 }; end` | **allowed** | — | Both blocks end up running as a method body, so reasoning from "will this run as a method?" gets both backwards. The rule is about an enclosing `def` **keyword**, reached through any number of blocks and stopped by a class or module body — which is exactly what `inDefBody` walks. The other row I expected to go the other way: a **scoped** assignment, `C::FOO = 1`, is refused inside a `def` too. Naming the namespace explicitly settles where the constant would go, and upstream refuses it anyway. ## Measured: eighteen shapes, eighteen agreeing Against `/usr/bin/ruby`. The rule predates that interpreter by many versions, so unlike the numbered-parameter check in #861 this oracle is a valid one here. Refused by both, after the change — plain, through one/two/three blocks, a lambda, `Class.new`, `instance_eval`, scoped, `::FOO`, `||=`, `+=`, masgn in either position, nested masgn, `def self.m`, `class << self` + `def`, `def o.m`, a nested `def`, a `rescue` clause, an `ensure` clause, and a default argument. Allowed by both — the top level, a class body, a module body, a nested class body, a block at the top level, a block in a class body, a `class << self` body, `define_method`, `module_eval`, and, inside a `def`, a local, an ivar, a class variable, a global, and READING a constant. One row deliberately still diverges: `def d; class I; end; end` is `class definition in method body` upstream and still compiles here. Different message, different construct, different check — filed as #860, and the `inDefBody` predicate this adds is what it will use. ## Blast radius No valid Ruby does this, so our own Ruby should be clean — and it is: the prelude, stdlib and gem tests pass, which includes regenerating the frozen prelude. `language/def_spec.rb` is unchanged at `pass=63 fail=8 error=1`. ⛔ The full `internal/vm` suite was NOT run to completion for this branch. The machine is at load 37–76 on 16 cores (around forty peer sessions are building across this fleet), and the run on the previous branch hit `panic: test timed out after 10m0s` at 604s where the same suite took 297s earlier this morning. Above roughly 8 this machine stops measuring, so I am claiming nothing from it either way; the `test` lanes and the ruby/spec ratchet here are the verdict. Tests: `internal/compiler/dynamic_constant_assignment_test.go` — the refusals, the fifteen controls, a planted positive (every control row passes with the check deleted), a same-name/two-scopes pair proving the predicate reads the SCOPE rather than the name, and `inDefBody` pinned directly through sources whose outcome depends only on the walk. Refs #799 Co-authored-by: Claude Opus 5 --- internal/compiler/compiler.go | 55 +++++++ .../dynamic_constant_assignment_test.go | 143 ++++++++++++++++++ 2 files changed, 198 insertions(+) create mode 100644 internal/compiler/dynamic_constant_assignment_test.go diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index 34241cb1..fe97f609 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -48,6 +48,10 @@ type builder struct { children []*bytecode.ISeq parent *builder isBlock bool + // isMethod marks a `def` body. A method body and a class body are both + // non-block scopes, so isBlock alone cannot tell them apart — and that + // difference is exactly what the dynamic-constant refusal turns on. + isMethod bool // forScope marks the each-block a `for` loop is compiled into. It is a block // iseq, but `for` opens no Ruby scope: a variable first assigned inside the // body belongs to the scope AROUND the loop, so declarations hop out of it. @@ -927,9 +931,11 @@ func (c *Compiler) compileNode1(n ast.Node) { c.compileNode(v.Recv) b.emit(bytecode.OpGetScopedConst, b.addName(v.Name), 0) case *ast.ConstAssign: + c.refuseDynamicConstAssign() c.compileNode(v.Value) b.emit(bytecode.OpSetConst, b.addName(v.Name), 0) case *ast.ScopedConstAssign: + c.refuseDynamicConstAssign() if sc, be, ok := scopedConstOpAssign(v); ok { c.compileScopedConstOpAssign(sc, be) break @@ -3224,8 +3230,10 @@ func (c *Compiler) storeMultiTarget(target ast.Node) { case *ast.VarRef: c.storeLocal(t.Name) case *ast.ConstRef: + c.refuseDynamicConstAssign() b.emit(bytecode.OpSetConst, b.addName(t.Name), 0) case *ast.ScopedConst: + c.refuseDynamicConstAssign() // Foo::BAR = value. SetScopedConst pops [recv, value]; the receiver must // sit under the value already on top, so stash and reload around it. tmp := b.localSlot("") @@ -3334,6 +3342,52 @@ func isNumberedParamName(name string) bool { return len(name) == 2 && name[0] == '_' && name[1] >= '1' && name[1] <= '9' } +// inDefBody reports whether the scope being compiled sits inside a `def` body, +// and it is MRI's own boundary for the dynamic-constant refusal rather than a +// convenient approximation of it. +// +// The walk passes THROUGH block scopes, so a constant assigned in a block, a +// lambda, or a `Class.new { … }` inside a method is still inside that method — +// `def d; Class.new { X = 1 }; end` is refused upstream. It STOPS at a class or +// module body and at the top level, which is why `class C; define_method(:m) { +// X = 1 }; end` is NOT refused even though the block will run as a method body. +// +// ⛔ Those two were measured, not reasoned about, and they are the pair that +// shows the rule is purely syntactic: it is about an enclosing `def` KEYWORD, +// not about whether the code will execute as a method. Reasoning from "will +// this run as a method?" gets the Class.new row and the define_method row both +// backwards. +// +// `for` bodies are followed like any other block (forScope is a block scope), +// which matches upstream: `for` opens no Ruby scope, so an assignment in a +// `for` inside a method is in the method. +func (c *Compiler) inDefBody() bool { + for b := c.cur(); b != nil; b = b.parent { + if b.isMethod { + return true + } + if !b.isBlock { + return false // a class body, a module body, or the top level + } + } + return false +} + +// refuseDynamicConstAssign is MRI's "dynamic constant assignment": a constant +// cannot be assigned where the assignment would run more than once with a +// different meaning, which upstream decides lexically — anywhere inside a `def`. +// It is a SyntaxError, raised before the value is compiled, because upstream +// raises it before the program runs at all. +// +// It covers the plain form, the scoped form (`C::X = 1` is refused inside a +// `def` TOO — being explicit about the namespace does not buy it), the +// op-assign forms, and a constant used as a multiple-assignment target. +func (c *Compiler) refuseDynamicConstAssign() { + if c.inDefBody() { + c.fail("dynamic constant assignment") + } +} + func (c *Compiler) storeLocal(name string) { // Binding `_1`..`_9` as a local is a SyntaxError in MRI wherever the binding // is written — a plain assignment, an op-assign, a multiple-assignment target, @@ -3731,6 +3785,7 @@ func (c *Compiler) compileMethodDef(v *ast.MethodDef) { } c.push(newBuilder(v.Name, params)) b := c.cur() + b.isMethod = true b.splatIndex = splatIndex // Keyword params get local slots right after the positionals, so the body // resolves them by name; record their names/required flags for the VM. diff --git a/internal/compiler/dynamic_constant_assignment_test.go b/internal/compiler/dynamic_constant_assignment_test.go new file mode 100644 index 00000000..28760f7d --- /dev/null +++ b/internal/compiler/dynamic_constant_assignment_test.go @@ -0,0 +1,143 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package compiler + +import ( + "strings" + "testing" +) + +// Assigning a constant inside a method body is MRI's "dynamic constant +// assignment", a SyntaxError raised before the program runs. rbgo refused none +// of it: `def d; FOO = 1; end` compiled and ran. +// +// Every row below was run through /usr/bin/ruby on the same source. The rule is +// ancient — it predates the 2.6 on this machine by many versions — so unlike the +// numbered-parameter check next door, that interpreter IS a valid oracle here, +// and all eighteen shapes agree with it. +// +// The boundary is PURELY SYNTACTIC: an enclosing `def` keyword, reached through +// any number of blocks. Two measured rows make that concrete and they point in +// opposite directions: +// +// def d; Class.new { FOO = 1 }; end refused +// class C; define_method(:m) { FOO = 1 }; end allowed +// +// Both blocks end up running as a method body, so reasoning from "will this run +// as a method?" gets both of them backwards. Only the lexical question answers. +func TestDynamicConstantAssignment(t *testing.T) { + const want = "dynamic constant assignment" + + refused := []string{ + // The plain shape, and the three scopes that are still the method. + "def d; AA = 1; end", + "def d; [1].each { AA = 1 }; end", + "def d; [1].each { [2].each { AA = 1 } }; end", + "def d; -> { AA = 1 }; end", + // ⛔ A block is not an escape even when it is building a class: the + // `Class.new` receiver is an expression, so nothing here opens a + // class BODY, and the walk still reaches the `def`. + "def d; Class.new { AA = 1 }; end", + "def d; Object.new.instance_eval { AA = 1 }; end", + // ⛔ A scoped assignment is refused inside a `def` too. Naming the + // namespace explicitly looks like it should settle where the constant + // goes, and it does — upstream refuses it anyway. + "class C; def m; C::AA = 1; end; end", + "def d; ::AA = 1; end", + // Every way of writing the assignment, not just the plain one. + "def d; AA ||= 1; end", + "def d; AA += 1; end", + "def d; AA, x = 1, 2; end", + "def d; x, AA = 1, 2; end", + "def d; AA, (x, BB) = 1, [2, 3]; end", + // Singleton and nested definitions are method bodies like any other. + "class S; def self.m; AA = 1; end; end", + "class S; class << self; def m; AA = 1; end; end; end", + "o = Object.new; def o.m; AA = 1; end", + "def d; def inner; AA = 1; end; end", + // Including the parts of a method that are not its statement list. + "def d; raise; rescue; AA = 1; end", + "def d; ensure; AA = 1; end", + "def d(x = (AA = 1)); end", + } + for _, src := range refused { + _, err := compileString(src) + if err == nil || !strings.Contains(err.Error(), want) { + t.Errorf("%q: err = %v, want one containing %q", src, err, want) + } + } + + // The controls. Every one of these is a place a constant assignment is + // ORDINARY Ruby, and a check that reached any of them would break real code. + allowed := []string{ + "AA = 1", // the top level + "class C; AA = 1; end", // a class body + "module M; AA = 1; end", // a module body + "class C; class D; AA = 1; end; end", // and a nested one + "[1].each { AA = 1 }", // a block at the top level + "class C; [1].each { AA = 1 }; end", // a block in a class body + "class C; class << self; AA = 1; end; end", // a singleton class body + "class C; define_method(:m) { AA = 1 }; end", // ⛔ the define_method row + "class C; end; C.module_eval { AA = 1 }", // a module_eval block + "def d; x = 1; end", // a local in a def is fine + "def d; @ivar = 1; end", // so is an ivar + "def d; @@cvar = 1; end", // and a class variable + "def d; $gvar = 1; end", // and a global + "def d; AA; end", // and READING a constant + "def d; C::AA; end", // or reading a scoped one + } + for _, src := range allowed { + if _, err := compileString(src); err != nil && strings.Contains(err.Error(), want) { + t.Errorf("%q: refused with %q, must be allowed", src, err) + } + } +} + +// The control for the control. Every row of the `allowed` list above passes if +// the refusal is deleted, so the suite needs one planted positive it cannot pass +// without the check — and one assertion that the check is looking at the SCOPE +// and not at the name, which a lookup table of constant names would also +// satisfy. +func TestDynamicConstantAssignmentIsObservable(t *testing.T) { + if _, err := compileString("def d; AA = 1; end"); err == nil { + t.Fatal("the planted positive compiled: the silence of the allowed rows proves nothing") + } + // Same name, same assignment, two scopes: one refused, one not. This is what + // says the predicate reads the scope chain. + if _, err := compileString("class C; SAME = 1; end"); err != nil { + t.Errorf("SAME = 1 in a class body: %v, want no error", err) + } + if _, err := compileString("class C; def m; SAME = 1; end; end"); err == nil { + t.Error("SAME = 1 in a method body compiled, want a refusal") + } +} + +// inDefBody is the predicate both this refusal and (later) the +// class-definition-in-method-body one need, so it is worth pinning directly +// rather than only through the sources above: the walk must pass through blocks +// and stop at a class or module body. +func TestInDefBodyWalksThroughBlocksAndStopsAtAClassBody(t *testing.T) { + // Expressed through sources whose outcome depends only on the walk, since + // inDefBody has no state a test can set up by hand. + for _, tc := range []struct { + src string + refused bool + }{ + {"def d; AA = 1; end", true}, // one hop, no blocks + {"def d; [1].each { AA = 1 }; end", true}, // through one block + {"def d; [1].each { [2].each { [3].each { AA = 1 } } }; end", true}, // three + {"def d; for x in [1]; AA = 1; end; end", true}, // a `for` is a block scope + {"class C; [1].each { AA = 1 }; end", false}, // stops at the class body + {"module M; [1].each { AA = 1 }; end", false}, // and at a module body + {"[1].each { AA = 1 }", false}, // and at the top level + {"class C; def m; end; AA = 1; end", false}, // a sibling def does not count + } { + _, err := compileString(tc.src) + got := err != nil && strings.Contains(err.Error(), "dynamic constant assignment") + if got != tc.refused { + t.Errorf("%q: refused = %v (err %v), want %v", tc.src, got, err, tc.refused) + } + } +} From b4b4e0f53f81b947c4a140f41d0d8d3313bb0aa6 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 1 Nov 2026 10:46:42 +0100 Subject: [PATCH 2/3] aot: the setconst codegen case was testing invalid Ruby MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three `arch` lanes failed on this branch, and the log names one case: ``` --- FAIL: TestCompileSupported/setconst codegen_test.go:84: compile "def m\n X = 1\n X\nend\nm": compile error: dynamic constant assignment ``` That fixture is **not valid Ruby**. MRI refuses a constant assignment inside a method body, and this branch makes rbgo refuse it too — so the fixture was exercising the `OpSetConst` arm through a source no Ruby program can contain. ## What it was hiding A method's ISeq can never legally hold `OpSetConst`, because upstream forbids the assignment in every form inside a `def` — including the scoped one, `C::X = 1`. Where the opcode does reach a child ISeq is a **block** whose lexical chain does not cross a `def`, or a class/module body. `methodISeq` takes `Children[0]` whatever kind it is, so a top-level block serves directly: ```go {"setconst", "[1].each { X = 1 }\nX", "block in
", "vm.consts["}, ``` `[1].each { X = 1 }` at the top level is accepted by MRI — measured, alongside the eight refusals in the commit below. So the fixture change is not a workaround for the new refusal. The old source could only ever have described a program the language does not permit, and the question of which ISeq kinds legally carry `OpSetConst` was invisible while it stood. ## ⛔ Why CI found this and I did not I ran `./internal/compiler/` and `./internal/vm/` for this branch, plus the prelude/stdlib/gem tests — and never `./internal/aot/`. A compiler change reaches every package that compiles Ruby, so the suite to run was `./...`. `go test ./... -count=1` now passes on this branch: exit 0, no FAIL lines, with the full `internal/vm` suite included (which also clears the only local measurement I could not complete when this PR was opened — the machine had been at load 37–76 and the suite had timed out at 604s). `main` is merged in as well, since it has moved (#856 landed). Refs #799 Co-authored-by: Claude Opus 5 --- internal/aot/codegen_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/aot/codegen_test.go b/internal/aot/codegen_test.go index 7f0bccad..77c3c248 100644 --- a/internal/aot/codegen_test.go +++ b/internal/aot/codegen_test.go @@ -71,7 +71,16 @@ func TestCompileSupported(t *testing.T) { {"getivar", "def m = @x\nm", "m", "getIvar(self,"}, {"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["}, + // ⛔ This case used to read `def m\n X = 1\n X\nend\nm`, which is NOT + // valid Ruby: MRI refuses a constant assignment inside a method body with + // "dynamic constant assignment", and so does this compiler now. The arm + // was being exercised through a source no Ruby program can contain. + // + // Where OpSetConst legally reaches a child ISeq is a BLOCK whose lexical + // chain does not cross a `def` — `[1].each { X = 1 }` at the top level is + // accepted by MRI — or a class/module body. methodISeq takes Children[0] + // whatever kind it is, so the block serves directly. + {"setconst", "[1].each { X = 1 }\nX", "block in
", "vm.consts["}, {"getgvar", "def m = $g\nm", "m", "vm.gvar("}, {"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("}, From 898a25dcf31263489ab7aca1e2870415787bcd96 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 1 Nov 2026 11:38:50 +0100 Subject: [PATCH 3/3] compiler: inDefBody's trailing return was unreachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macOS lane failed, and not on a percentage: ``` -- NEWLY BELOW 100% (1) -- internal/compiler/compiler.go inDefBody 83.3% (was fully covered) ##[error]1 function(s) dropped below 100% -- named above ``` Five statements of six. The missing one was the `return false` **after** the loop, and it is not missing a test — it cannot have one. Every builder chain is rooted at `
`, ``, ``, `` or ``, and none of those is a block, so the walk always leaves through the `!b.isBlock` arm and never by exhausting `b != nil`. So the arm returns through a `break` and the function ends with the single reachable `return false`. `inDefBody` and `refuseDynamicConstAssign` both measure 100.0% under `go tool cover -func`, the same instrument the lane uses. ⛔ Worth noting about this gate, because my own notes had it wrong: the percentage is **reported and not gated** any more — `coverage: 99.9% (reported, NOT gated -- see below)` — and `scripts/coverage/cmd/covratchet` gates per-function against `scripts/coverage/BELOW100.` instead. So a POSIX lane failing here is no longer the rounding knife-edge it used to be; it names a function, and the two "further below than recorded" rows it also printed (`bootstrap` 99.6% and `registerNetHTTP` 92.3%) are pre-existing and ungated. That is a strictly better gate than the one that had to be argued with. The comment now says why the return cannot be covered, which is cheaper than the next person rediscovering it. Refs #799 Co-authored-by: Claude Opus 5 --- internal/compiler/compiler.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index fe97f609..fdf05d68 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -3361,13 +3361,19 @@ func isNumberedParamName(name string) bool { // `for` bodies are followed like any other block (forScope is a block scope), // which matches upstream: `for` opens no Ruby scope, so an assignment in a // `for` inside a method is in the method. +// ⛔ The walk ALWAYS terminates on a non-block scope, so the exit it takes is +// the `break`: every builder chain is rooted at `
`, ``, +// ``, `` or ``, none of which is a block. +// Written with a bare `return false` after the loop instead, that return was +// unreachable — the coverage ratchet caught it at 83.3% and naming it here is +// cheaper than the next person rediscovering why it cannot be tested. func (c *Compiler) inDefBody() bool { for b := c.cur(); b != nil; b = b.parent { if b.isMethod { return true } if !b.isBlock { - return false // a class body, a module body, or the top level + break // a class body, a module body, or the top level } } return false