diff --git a/internal/aot/codegen_test.go b/internal/aot/codegen_test.go index 00f40e92..cb6de1b1 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.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("}, diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index 918758fb..63f46471 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 // writtenParams records that this block's parameter list was WRITTEN — `|x|`, // `|&b|`, `|k: 1|`, or even an empty `||` or `()` — rather than synthesised by // the parser from `it` / `_1`..`_9` in the body. It is what makes `_1` inside @@ -932,9 +936,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 @@ -3252,8 +3258,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("") @@ -3362,6 +3370,58 @@ 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. +// ⛔ 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 { + break // 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, @@ -3759,6 +3819,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) + } + } +}