From 04868614047dd226669ff752e8045b79a58440bd Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 22 Sep 2026 13:51:46 +0200 Subject: [PATCH 1/7] deps: go-ruby-parser v0.2.0 The v0.2.0 parser makes 13 more language/*_spec.rb files of the pinned corpus parseable, and adds For.Target and Block.Locals to the AST. None of the compiler work in this branch is visible on v0.1.9. Co-Authored-By: Claude Opus 5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 7921bfb1..67927d0d 100644 --- a/go.mod +++ b/go.mod @@ -129,7 +129,7 @@ require ( github.com/go-ruby-pagy/pagy v0.0.0-20260707193354-a84fe1d16a86 github.com/go-ruby-paper-trail/paper-trail v0.0.0-20260707193452-4e10f2b4d0b3 github.com/go-ruby-parquet/parquet v0.0.0-20260704170648-c7f0507946f7 - github.com/go-ruby-parser/parser v0.1.9 + github.com/go-ruby-parser/parser v0.2.0 github.com/go-ruby-pathname/pathname v0.0.0-20260629151955-d8d2c4e5f81b github.com/go-ruby-pg/pg v0.0.0-20260702135906-e5650264cc5d github.com/go-ruby-prawn/prawn v0.0.0-20260728194101-a5f2d786bf88 diff --git a/go.sum b/go.sum index 17102e55..a36d4704 100644 --- a/go.sum +++ b/go.sum @@ -519,8 +519,8 @@ github.com/go-ruby-paper-trail/paper-trail v0.0.0-20260707193452-4e10f2b4d0b3 h1 github.com/go-ruby-paper-trail/paper-trail v0.0.0-20260707193452-4e10f2b4d0b3/go.mod h1:z370KNJcUH8n39yLdlT0Y1QvHukxjsq1XjGpIzvfqWE= github.com/go-ruby-parquet/parquet v0.0.0-20260704170648-c7f0507946f7 h1:kuL9coQlysvsIZXLzCfKZguDKvDwTibgJrLCx4jh5Ak= github.com/go-ruby-parquet/parquet v0.0.0-20260704170648-c7f0507946f7/go.mod h1:reJ/Dgyv5OWqVEERevwU1yX6N+CB5RfTQeGWGjmnOFo= -github.com/go-ruby-parser/parser v0.1.9 h1:q6fy2ReOop5rKej0HJHLQSFVC+ejerzpZ5ySvAXyhQU= -github.com/go-ruby-parser/parser v0.1.9/go.mod h1:NRbX4dYcQ6CgcaSaMmu1dh95QuKGq+RdenUoWU73214= +github.com/go-ruby-parser/parser v0.2.0 h1:YeTtK8BgYMbFXW4pRcfC4p6vjtnENWzg9oUFMsiMHkk= +github.com/go-ruby-parser/parser v0.2.0/go.mod h1:NRbX4dYcQ6CgcaSaMmu1dh95QuKGq+RdenUoWU73214= github.com/go-ruby-pathname/pathname v0.0.0-20260629151955-d8d2c4e5f81b h1:/ZN1eLOg0oH4wF8qmd6NOyB4ENYthx6wFwOpZiPhRIE= github.com/go-ruby-pathname/pathname v0.0.0-20260629151955-d8d2c4e5f81b/go.mod h1:IvW5q5Kyl5l3hrYwiHylOEmk5hpGSo7XqcmUXBHF5H0= github.com/go-ruby-pg/pg v0.0.0-20260702135906-e5650264cc5d h1:JKIZD+pJQ64/nla4DHdzMLnc4/v45MyYwnjACwS6dyM= From ae5a94b9e4073cd35ceb2c38fd46bf624772dd0e Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 22 Sep 2026 13:55:10 +0200 Subject: [PATCH 2/7] compiler: give block-local variables their own slots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{ |a; b, c| … }` declares b and c as fresh, nil-valued locals of the block that shadow any enclosing binding of the same name. The v0.2.0 parser carries them on Block.Locals; the compiler ignored the field, so every mention of one resolved to the outer binding — or to nothing, and language/block_spec.rb stopped at "undefined local variable" before a single example ran. MRI read: prism_compile.c v3_4_0:6325-6334, which walks the block parameters node's `locals` list and calls pm_insert_local_index for each right after the requireds/optionals/rest/posts and before the "fill in any locals we missed" pass — so they are table entries, not parameters, and start nil. parse.y v3_4_0:5128-5148 (opt_bv_decl/bv_decls/bvar) and new_bv at 13621 show the parser already rejects a block-local that shadows a parameter, so no dedup is needed at this end. language/block_spec.rb: 0 -> 139 passing. Co-Authored-By: Claude Opus 5 --- internal/compiler/compiler.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index f1288210..2be501fb 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -1535,6 +1535,18 @@ func (c *Compiler) compileBlock(blk *ast.Block) int { if blk.BlockParam != "" { b.blockSlot = b.localSlot(blk.BlockParam) } + // Block-local variables (`{ |a; b, c| … }`) take slots of their own right + // after the parameters, and are NOT parameters: they start nil and shadow any + // enclosing binding of the same name, so an assignment inside the block cannot + // reach the outer one. MRI builds exactly this table — prism_compile.c + // v3_4_0:6325-6334 walks the block-parameters node's `locals` list and calls + // pm_insert_local_index for each, after the requireds/optionals/rest/posts and + // before "fill in any locals we missed" — and the parser rejects a name that + // collides with a parameter (parse.y v3_4_0:13621 new_bv → shadowing_lvar_0), + // so no dedup is needed here. + for _, name := range blk.Locals { + b.localSlot(name) + } // Optional keyword params: the VM binds the supplied ones natively, so the // prologue only fills in defaults for the absent ones. for i, kp := range kwParams { From 8bc759b2593865b8f396c03c57206e11a64d9694 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 22 Sep 2026 14:06:52 +0200 Subject: [PATCH 3/7] compiler: read For.Target, and give a `for` body the scope it has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four things, all in the lowering of `for`: 1. For.Target. The v0.2.0 parser puts every loop variable Vars cannot spell into For.Target and leaves Vars empty — a lone non-local (`for @v in …`, `for a[0] in …`), or any list with a `*rest`, a nested group, a non-local, or a trailing comma. The compiler read only Vars, so `for i, in [[1,2]]` compiled a loop over nothing and language/for_spec.rb stopped at "undefined local variable". 2. The each-block's parameter shape. MRI picks it by the kind of loop variable (prism_compile.c v3_4_0:6293-6301): a lone local target bumps lead_num, so the block takes one required argument; anything else sets has_rest, so the block collects every yielded value into an Array. rbgo always took one required argument, so a `for x, y in o` over an `each` that does `yield 1, 2` bound y to nil where MRI binds 2. The single-non-local arm is MRI's `expandarray 1, 0` on that rest Array (prism_compile.c v3_4_0:4862-4880), which is why the loop variable takes the FIRST yielded value and drops the others. For the multi-target arm MRI emits `(args.length == 1 && Array.try_convert(args[0])) || args` then expandarray (compile.c compile_for_masgn v3_4_0:8302-8329, prism_compile.c v3_4_0:4881-4919). The try_convert half is redundant in front of an expandarray that already converts: when args holds one element, expanding args[0] yields its Array conversion if it has one and the one-element list [args[0]] otherwise — which is what expanding args gives. So the same lowering is `args.length == 1 ? args[0] : args`, emitted with the same idLength and idAREF sends MRI uses. 3. `for o&.target in m`. storeMultiTarget ignored Call.Safe, so a nil receiver raised NoMethodError instead of skipping the write. A multiple assignment can never reach that branch (`a&.x, b = …` is a SyntaxError), but a for-var can: prism_compile.c v3_4_0:4862 routes PM_CALL_TARGET_NODE through pm_compile_target_node, which honours PM_CALL_NODE_FLAGS_SAFE_NAVIGATION. 4. `for` opens no scope. The each-block is a block ISeq but not a Ruby scope, so a variable FIRST ASSIGNED in the body belongs to the scope around the loop: after `for a in [6]; for b in [7]; c = a * b; end; end`, c is a local of the enclosing method. It hops out of enclosing `for` bodies but not out of an enclosing block, which is a scope — the for_spec "can be nested with blocks in between" example pins both halves. builder.declOwner names that scope; every site that declares a named local now goes through it (assignment, op-assignment, named regexp captures, `rescue => e`), which also removes four copies of the resolve-or-allocate dance. Also fixed on the way, because for_spec is the file that exposes it: mustResolve dropped the block depth, so `g(...)` argument forwarding inside ANY block — `def f(...); [1].each { g(...) }; end` as much as a `for` body — indexed the block's own frame and crashed with a Go index-out-of-range surfaced as ArgumentError. language/for_spec.rb: 0 -> 29 passing, 0 failing. The 5 remaining errors are Kernel#local_variables and #redo, neither implemented, neither in this branch's files. language/rescue_spec.rb: 49 -> 50. Co-Authored-By: Claude Opus 5 --- internal/compiler/compiler.go | 243 ++++++++++++++++++++++++++-------- 1 file changed, 185 insertions(+), 58 deletions(-) diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index 2be501fb..f9db4c69 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -47,6 +47,10 @@ type builder struct { children []*bytecode.ISeq parent *builder isBlock 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. + forScope bool // borrowed marks a scope whose locals belong to somebody else's already-built // frame — the synthetic parent CompileWithLocals puts a Binding's locals in. // Its slots may be read and written but no new one may be added: the frame @@ -83,6 +87,19 @@ func (b *builder) resolve(name string) (depth, index int, ok bool) { return 0, 0, false } +// declOwner returns the scope a variable first assigned in b belongs to, and +// how many block hops away it is. `for` opens no scope of its own, so a name +// introduced in a `for` body is declared in the nearest enclosing scope that is +// not itself a `for` body — the surrounding block when there is one, else the +// method or top level. For any other builder the answer is b itself, at depth 0. +func (b *builder) declOwner() (owner *builder, depth int) { + owner, depth = b, 0 + for owner.forScope && owner.parent != nil { + owner, depth = owner.parent, depth+1 + } + return owner, depth +} + func (b *builder) emit(op bytecode.Op, a, bb int) int { b.insns = append(b.insns, bytecode.Instr{Op: op, A: a, B: bb}) return len(b.insns) - 1 @@ -460,12 +477,9 @@ func (c *Compiler) compileNode(n ast.Node) { case *ast.Assign: c.compileNode(v.Value) // Assign to an enclosing local if one is visible; otherwise create a - // new local in the current scope. - if depth, slot, ok := b.resolve(v.Name); ok { - b.emit(bytecode.OpSetLocal, slot, depth) - } else { - b.emit(bytecode.OpSetLocal, b.localSlot(v.Name), 0) - } + // new local in the current scope (or, inside a `for` body, in the scope + // around the loop — see declOwner). + c.storeLocal(v.Name) case *ast.MultiAssign: c.compileMultiAssign(v) case *ast.Begin: @@ -498,10 +512,7 @@ func (c *Compiler) compileNode(n ast.Node) { case *ast.OpAssign: // Allocate the slot before the read so a fresh `x ||= v` sees nil // rather than failing to resolve. - depth, slot, ok := b.resolve(v.Name) - if !ok { - slot, depth = b.localSlot(v.Name), 0 - } + slot, depth := c.bindLocal(v.Name) c.compileNode(&ast.BinaryExpr{Op: v.Op, Left: &ast.VarRef{Name: v.Name}, Right: v.Value}) b.emit(bytecode.OpSetLocal, slot, depth) case *ast.UnaryExpr: @@ -799,7 +810,8 @@ func (c *Compiler) compileForwardCall(v *ast.Call, fwdAt int) { } c.emitForwardArgsArray(v.Args[:fwdAt], v.Args[fwdAt+1:]) // Forward the block (&...blk; nil ⇒ no block) and send. - b.emit(bytecode.OpGetLocal, c.mustResolve(fwdBlockName), 0) + blkSlot, blkDepth := c.mustResolve(fwdBlockName) + b.emit(bytecode.OpGetLocal, blkSlot, blkDepth) b.emit(bytecode.OpSendArrayBlockArg, b.addName(v.Name), 0) } @@ -819,14 +831,16 @@ func (c *Compiler) emitForwardArgsArray(leading, trailing []ast.Node) { for _, a := range leading { appendOne(a) } - b.emit(bytecode.OpGetLocal, c.mustResolve(fwdRestName), 0) + restSlot, restDepth := c.mustResolve(fwdRestName) + b.emit(bytecode.OpGetLocal, restSlot, restDepth) b.emit(bytecode.OpSplatToArray, 0, 0) b.emit(bytecode.OpConcatArray, 0, 0) for _, a := range trailing { appendOne(a) } // Append **...kw as a trailing hash, but only when it has entries. - b.emit(bytecode.OpGetLocal, c.mustResolve(fwdKwName), 0) + kwSlot, kwDepth := c.mustResolve(fwdKwName) + b.emit(bytecode.OpGetLocal, kwSlot, kwDepth) b.emit(bytecode.OpDup, 0, 0) b.emit(bytecode.OpSend, b.addName("empty?"), 0) skip := b.emit(bytecode.OpBranchIf, 0, 0) @@ -839,14 +853,18 @@ func (c *Compiler) emitForwardArgsArray(leading, trailing []ast.Node) { } // mustResolve resolves a (compiler-synthesised) local that is guaranteed to -// exist in scope, returning its slot. It panics via fail otherwise, which only -// happens on a compiler bug. -func (c *Compiler) mustResolve(name string) int { - _, slot, ok := c.cur().resolve(name) +// exist in scope, returning its slot AND its block depth. The depth matters: +// `def f(...)` parks the forwarded arguments in the method's own locals, so a +// `g(...)` written inside a block — including the each-block a `for` compiles +// into — reads them from an enclosing frame, and a depth of 0 would index the +// block's own frame instead. It panics via fail when the name is out of scope, +// which only happens on a compiler bug. +func (c *Compiler) mustResolve(name string) (slot, depth int) { + depth, slot, ok := c.cur().resolve(name) if !ok { c.fail("argument forwarding outside a def(...) method") } - return slot + return slot, depth } // anonLocal returns a VarRef reading the enclosing method's anonymous-forward @@ -1292,10 +1310,7 @@ func (c *Compiler) compileMatchWithCaptures(v *ast.BinaryExpr, names []string) { b.emit(bytecode.OpSend, b.addName("=~"), 1) // leaves the match position (or nil) for _, name := range names { // Reuse an existing local of this name, else declare it here. - depth, slot, ok := b.resolve(name) - if !ok { - slot, depth = b.localSlot(name), 0 - } + slot, depth := c.bindLocal(name) b.emit(bytecode.OpGetConst, b.addName("Regexp"), 0) b.emit(bytecode.OpPushConst, b.addConst(object.SymVal(name)), 0) b.emit(bytecode.OpSend, b.addName("last_match"), 1) // $~[:name], nil if unmatched @@ -1672,7 +1687,8 @@ func (c *Compiler) compileSuper(v *ast.Super) { // built args array (with the captured &...blk overriding the frame block). if fwdAt := forwardIndex(v.Args); fwdAt >= 0 { c.emitForwardArgsArray(v.Args[:fwdAt], v.Args[fwdAt+1:]) - b.emit(bytecode.OpGetLocal, c.mustResolve(fwdBlockName), 0) + blkSlot, blkDepth := c.mustResolve(fwdBlockName) + b.emit(bytecode.OpGetLocal, blkSlot, blkDepth) at := b.emit(bytecode.OpInvokeSuperArray, 0, 0) b.insns[at].C = 1 return @@ -2480,6 +2496,19 @@ func (c *Compiler) storeMultiTarget(target ast.Node) { b.emit(bytecode.OpPop, 0, 0) pre := c.masgnPre[target] c.pushMasgnRecv(target, t.Recv) + // `for o&.x in …`: a nil receiver skips the setter and leaves nil as this + // arm's value. MRI reaches the same store arm for a safe-navigation for-var + // (prism_compile.c v3_4_0:4862 routes PM_CALL_TARGET_NODE through + // pm_compile_target_node, which honours PM_CALL_NODE_FLAGS_SAFE_NAVIGATION); + // a multiple assignment cannot get here, since `a&.x, b = …` is a + // SyntaxError. OpBranchNil pops its tested copy, so the receiver left + // underneath is the nil result, matching the ordinary `o&.x = v` lowering. + safeSkip := -1 + if t.Safe { + b.emit(bytecode.OpDup, 0, 0) + safeSkip = b.emit(bytecode.OpBranchNil, 0, 0) + defer func() { b.patch(safeSkip, b.here()) }() + } if pre != nil && pre.argsArr >= 0 { // Dynamic arity, argument array already built before the RHS: append // the assigned value and send. OpConcatArray always allocates a fresh @@ -2528,15 +2557,23 @@ func (c *Compiler) storeMultiTarget(target ast.Node) { } } -// storeLocal emits a SetLocal for name, resolving an existing local or -// allocating a fresh slot, mirroring ast.Assign. -func (c *Compiler) storeLocal(name string) { +// bindLocal returns the slot and block depth to write name at: an existing +// visible local if there is one, else a freshly declared slot in the scope +// declOwner names (the current builder, unless it is a `for` body). +func (c *Compiler) bindLocal(name string) (slot, depth int) { b := c.cur() if depth, slot, ok := b.resolve(name); ok { - b.emit(bytecode.OpSetLocal, slot, depth) - } else { - b.emit(bytecode.OpSetLocal, b.localSlot(name), 0) + return slot, depth } + owner, depth := b.declOwner() + return owner.localSlot(name), depth +} + +// storeLocal emits a SetLocal for name, resolving an existing local or +// allocating a fresh slot, mirroring ast.Assign. +func (c *Compiler) storeLocal(name string) { + slot, depth := c.bindLocal(name) + c.cur().emit(bytecode.OpSetLocal, slot, depth) } // compileBegin compiles begin/rescue/else/ensure. When an ensure clause is @@ -2604,11 +2641,7 @@ func (c *Compiler) compileBeginRescue(v *ast.Begin) { } switch { case clause.Var != "": // bind the exception to a local (reusing an existing slot) - if depth, slot, ok := b.resolve(clause.Var); ok { - b.emit(bytecode.OpSetLocal, slot, depth) - } else { - b.emit(bytecode.OpSetLocal, b.localSlot(clause.Var), 0) - } + c.storeLocal(clause.Var) case clause.VarTarget != nil: // `rescue => @e` / `=> $g` / `=> Foo::E`: a non-local target c.storeMultiTarget(clause.VarTarget) } @@ -2694,27 +2727,42 @@ func (c *Compiler) compileFor(v *ast.For) { // The loop variables live in the enclosing scope; ensure each has a slot // there now (before compiling the block) so the block resolves them by depth. // A `for` does not open a scope, so an absent variable is created in the - // nearest method/top-level scope (the first non-block ancestor), making it - // leak past the whole construct — including out of any enclosing blocks, e.g. - // an outer `for` — exactly as MRI does. + // nearest scope that is not itself a `for` body, making it leak past the whole + // construct — including out of an enclosing `for`, but NOT out of an enclosing + // block, which is a scope. That is what declOwner walks. parent := c.cur() - owner := parent - for owner.isBlock && owner.parent != nil { - owner = owner.parent - } - for _, name := range v.Vars { + owner, _ := parent.declOwner() + declare := func(name string) { + if name == "" { + return // the nameless `*` in `for i, * in …` binds nothing + } if _, _, ok := parent.resolve(name); !ok { owner.localSlot(name) } } + for _, name := range v.Vars { + declare(name) + } + declareForTargetLocals(v.Target, declare) // Receiver: the iterable. c.compileNode(v.Iter) - // Synthesize the each-block as a child ISeq with one hidden parameter. + // Synthesize the each-block as a child ISeq with one hidden parameter. MRI + // shapes that parameter by the kind of loop variable (prism_compile.c + // v3_4_0:6293-6301): a lone local target bumps lead_num, so the block takes + // ONE required argument and gets the yielded value as it stands; anything + // else sets has_rest, so the block collects ALL the yielded values into an + // Array and the destructuring below works from that. c.push(newBlockBuilder("", []string{"...for"}, parent)) b := c.cur() - b.numRequired = 1 + b.forScope = true + if forTakesOneValue(v) { + b.numRequired = 1 + } else { + b.numRequired = 0 + b.splatIndex = 0 + } c.ctxs = append(c.ctxs, &loopCtx{kind: ctxBlock}) - c.compileForVars(v.Vars) + c.compileForTarget(v) c.compileBody(v.Body) c.ctxs = c.ctxs[:len(c.ctxs)-1] b.emit(bytecode.OpReturn, 0, 0) @@ -2725,25 +2773,104 @@ func (c *Compiler) compileFor(v *ast.For) { parent.insns[at].C = idx + 1 // C-1 indexes Children } -// compileForVars binds the yielded value (the hidden first block local) to the -// `for` loop variables, which resolve to enclosing-scope locals. A single -// variable takes the value directly; multiple variables destructure it like a -// MultiAssign (the value is splatted into an Array and expanded). -func (c *Compiler) compileForVars(vars []string) { +// forTakesOneValue reports whether the loop variable is a single plain local +// (`for i in …`), the one shape whose each-block takes a required parameter +// rather than collecting the yielded values into a rest Array. +func forTakesOneValue(v *ast.For) bool { + return v.Target == nil && len(v.Vars) == 1 +} + +// declareForTargetLocals calls declare for every plain local name a For.Target +// assigns, so the name exists in the enclosing scope before the loop body is +// compiled — `for a, *b in […]` leaves both a and b visible after the loop, like +// any other `for` variable. Non-local targets (ivars, globals, constants, setter +// calls) declare nothing. +func declareForTargetLocals(target ast.Node, declare func(string)) { + switch t := target.(type) { + case *ast.VarRef: + declare(t.Name) + case *ast.MultiAssign: + if len(t.Targets) != len(t.Names) { + // All-locals form (`for i, in …`): Names carries every binding. + for _, name := range t.Names { + declare(name) + } + return + } + for _, sub := range t.Targets { + declareForTargetLocals(sub, declare) + } + } +} + +// compileForTarget binds the values yielded to the each-block to the `for` loop +// variable, which resolves to enclosing-scope locals (or to whatever else the +// target names). MRI emits this inside the block iseq, before the body and +// before the B_CALL event: prism_compile.c v3_4_0:6489-6494 calls +// pm_compile_for_node_index, whose three arms (v3_4_0:4844-4924) are the three +// below. +func (c *Compiler) compileForTarget(v *ast.For) { b := c.cur() - if len(vars) == 1 { - b.emit(bytecode.OpGetLocal, 0, 0) // the hidden parameter - c.storeLocal(vars[0]) + // A lone local: the block took the value as a required parameter, so it is + // already the value to bind (PM_LOCAL_VARIABLE_TARGET_NODE: getlocal, write). + if forTakesOneValue(v) { + b.emit(bytecode.OpGetLocal, 0, 0) + c.storeLocal(v.Vars[0]) b.emit(bytecode.OpPop, 0, 0) return } - b.emit(bytecode.OpGetLocal, 0, 0) - b.emit(bytecode.OpSplatToArray, 0, 0) - b.emit(bytecode.OpExpandArray, len(vars), 0) - for _, name := range vars { - c.storeLocal(name) + names, targets, splatIndex := v.Vars, []ast.Node(nil), -1 + switch t := v.Target.(type) { + case nil: + // Several plain locals (`for a, b in …`): MRI's multi-target arm. + case *ast.MultiAssign: + names, splatIndex = t.Names, t.SplatIndex + if len(t.Targets) == len(t.Names) { + targets = t.Targets + } + default: + // A single non-local target (`for @v in …`, `for a[0] in …`): MRI pushes + // the rest Array and runs `expandarray 1, 0` on it, taking the first + // yielded value and discarding the rest, then performs the write. + b.emit(bytecode.OpGetLocal, 0, 0) + b.emit(bytecode.OpExpandArray, 1, 0) + c.storeMultiTarget(v.Target) b.emit(bytecode.OpPop, 0, 0) + return } + c.pushForMasgnOperand() + c.expandAndStore(names, targets, splatIndex) +} + +// pushForMasgnOperand pushes the value a multi-target `for` destructures, from +// the rest Array of yielded values held in the block's hidden parameter. +// +// MRI's sequence (compile.c compile_for_masgn, ruby/ruby v3_4_0:8302-8329, and +// the identical prism arm at prism_compile.c v3_4_0:4881-4919) is +// +// (args.length == 1 && Array.try_convert(args[0])) || args +// +// followed by expandarray. The try_convert half is redundant in front of an +// expandarray that already performs it: when args holds exactly one element, +// expanding args[0] gives the converted Array if args[0] has one, and otherwise +// the one-element list [args[0]] — which is what expanding args itself would +// have given. So the same lowering is `args.length == 1 ? args[0] : args`, with +// the conversion left where it always was, in expandarray. Both halves use the +// same sends MRI uses (idLength and idAREF), so a redefined Array#length or +// Array#[] is honoured here exactly as it is there. +func (c *Compiler) pushForMasgnOperand() { + b := c.cur() + b.emit(bytecode.OpGetLocal, 0, 0) // the hidden *rest parameter + b.emit(bytecode.OpDup, 0, 0) + b.emit(bytecode.OpSend, b.addName("length"), 0) + b.emit(bytecode.OpPushConst, b.addConst(object.IntValue(1)), 0) + b.emit(bytecode.OpEq, 0, 0) + notSingle := b.emit(bytecode.OpBranchUnless, 0, 0) + b.emit(bytecode.OpPushConst, b.addConst(object.IntValue(0)), 0) + b.emit(bytecode.OpSend, b.addName("[]"), 1) + done := b.emit(bytecode.OpJump, 0, 0) + b.patch(notSingle, b.here()) + b.patch(done, b.here()) } // Reserved local names for argument forwarding (`def f(...)`): they capture the From 8123f37f5cd34d43a880cb2dfc4ed9ae5e88e0c3 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 22 Sep 2026 14:14:16 +0200 Subject: [PATCH 4/7] compiler: declare the locals a defined? operand introduces, and tag a desugared compound assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `defined?` inspects its operand's syntactic kind instead of compiling it, so the assignments inside it emit nothing — but they still DECLARE. MRI builds its local table in the PARSER, which walks the operand like any other code, so `defined?(a += 1)` leaves `a` a nil local of the enclosing scope and a later bare `a` is a local-variable read. rbgo declares as it compiles, so `a` got no slot and the next mention of it — in language/defined_spec.rb, `defined?(a.b += 1)` six lines further on, whose receiver the compiler DOES evaluate — failed with "undefined local variable". Verified against MRI: `defined?(a += 1); p a` prints nil, and `p defined?(a = 1), a.inspect` prints "assignment" then "nil". Second, `a.b += 1` and `a[:b] ||= 1` reach the compiler as the parser's textual desugaring into a setter Call, and were tagged "method". MRI tags them "assignment": PM_CALL_OPERATOR_WRITE_NODE, PM_CALL_OR_WRITE_NODE, PM_CALL_AND_WRITE_NODE and the PM_INDEX_* trio are all in the DEFINED_ASGN group (prism_compile.c v3_4_0:4052-4093; compile.c's NODE_OP_ASGN1/NODE_OP_ASGN2 at v3_4_0:6151-6164 say the same). A plain `a[0] = 1` is NOT in that group — it is an ordinary attrasgn call and stays "method", which ruby/spec pins. opassign.go's shared-node recognisers tell the two apart. language/defined_spec.rb: 0 -> 211 passing. Of the 47 that remain, two groups are worth naming: `defined?(super)` answers "expression" instead of "super"/nil (18 examples), and the tag Strings are not frozen (about 12). Half of the latter is a one-line change in internal/vm/defined.go, outside this branch's files. Co-Authored-By: Claude Opus 5 --- internal/compiler/defined.go | 92 ++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/internal/compiler/defined.go b/internal/compiler/defined.go index 306e8c4e..7931877e 100644 --- a/internal/compiler/defined.go +++ b/internal/compiler/defined.go @@ -14,6 +14,7 @@ import ( // sub-expression maps to nil rather than propagating. func (c *Compiler) compileDefined(operand ast.Node) { b := c.cur() + c.declareDefinedLocals(operand) switch v := operand.(type) { case *ast.NilLit: c.pushDefinedTag("nil") @@ -44,6 +45,26 @@ func (c *Compiler) compileDefined(operand ast.Node) { *ast.ScopedConstAssign, *ast.IvarAssign, *ast.CVarAssign, *ast.GVarAssign: c.pushDefinedTag("assignment") case *ast.Call: + // A compound assignment to an attribute or an index (`a.b += 1`, + // `a[:b] ||= 1`) reaches the compiler as the parser's textual desugaring + // into a setter Call, but MRI tags it "assignment", not "method": + // PM_CALL_OPERATOR_WRITE_NODE / PM_CALL_OR_WRITE_NODE / + // PM_CALL_AND_WRITE_NODE and their PM_INDEX_* counterparts are all in the + // DEFINED_ASGN group (prism_compile.c v3_4_0:4052-4093, matching + // compile.c's NODE_OP_ASGN1/NODE_OP_ASGN2 at v3_4_0:6151-6164). A PLAIN + // `a[0] = 1` is not in that group — it is an ordinary attrasgn call, and + // stays "method". The same shared-node recognisers compileCall uses tell + // the two apart; see opassign.go for why identity is the test. + if isSetterCall(v) { + if _, _, ok := indexOpAssign(v); ok { + c.pushDefinedTag("assignment") + return + } + if _, ok := attrOpAssign(v); ok { + c.pushDefinedTag("assignment") + return + } + } c.compileDefinedCall(v) case *ast.BinaryExpr: c.compileDefinedBinary(v) @@ -151,3 +172,74 @@ func (c *Compiler) guarded(body func(gb *builder)) { parent.children = append(parent.children, child) parent.emit(bytecode.OpDefinedGuard, idx, 0) } + +// declareDefinedLocals declares the locals an operand of `defined?` introduces. +// +// `defined?` inspects its operand's syntactic kind instead of compiling it, so +// the assignments inside it emit no code — but they still DECLARE. MRI builds +// its local table in the parser, which walks the operand like any other code, +// so `defined?(a += 1)` leaves `a` as a (nil) local of the enclosing scope and a +// later bare `a` is a local-variable read rather than a method call. rbgo +// declares locals as it compiles, so without this walk the name has no slot and +// the next mention of it fails to compile — which is where +// language/defined_spec.rb stopped. +// +// Only assignment targets are declared; nothing is evaluated. The walk descends +// through the value and receiver/argument positions an assignment can hide in, +// which is what the parser's own walk amounts to. +func (c *Compiler) declareDefinedLocals(n ast.Node) { + switch v := n.(type) { + case *ast.Assign: + c.declareLocal(v.Name) + c.declareDefinedLocals(v.Value) + case *ast.OpAssign: + c.declareLocal(v.Name) + c.declareDefinedLocals(v.Value) + case *ast.MultiAssign: + for _, name := range v.Names { + c.declareLocal(name) + } + for _, t := range v.Targets { + c.declareDefinedLocals(t) // a nested group declares its own names + } + for _, val := range v.Values { + c.declareDefinedLocals(val) + } + case *ast.IvarAssign: + c.declareDefinedLocals(v.Value) + case *ast.CVarAssign: + c.declareDefinedLocals(v.Value) + case *ast.GVarAssign: + c.declareDefinedLocals(v.Value) + case *ast.ConstAssign: + c.declareDefinedLocals(v.Value) + case *ast.ScopedConstAssign: + c.declareDefinedLocals(v.Value) + case *ast.Call: + if v.Recv != nil { + c.declareDefinedLocals(v.Recv) + } + for _, a := range v.Args { + c.declareDefinedLocals(a) + } + case *ast.BinaryExpr: + c.declareDefinedLocals(v.Left) + c.declareDefinedLocals(v.Right) + case *ast.UnaryExpr: + c.declareDefinedLocals(v.Operand) + } +} + +// declareLocal gives name a slot if it does not already have one, without +// emitting anything. An empty name is the nameless `*` of `a, * = …`. +func (c *Compiler) declareLocal(name string) { + if name == "" { + return + } + b := c.cur() + if _, _, ok := b.resolve(name); ok { + return + } + owner, _ := b.declOwner() + owner.localSlot(name) +} From e4ddf386c08f697626254da700555586e5779217 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 08:13:39 +0200 Subject: [PATCH 5/7] compiler: whitebox tests for the wave-26 for/block-local/defined? branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers every new branch in the package profile: the For.Target shapes and where each declares its locals, the each-block parameter shape and the two target lowerings, the safe-navigation guard, block-local slots, the defined? declaration walk, and the depth mustResolve had been dropping. Also drops an unreachable guard: compileFor's declare() skipped an empty name, but every For.Target shape that carries one (`for i, * in …`) also carries a parallel Targets list with a typed nil in that position, so the all-locals arm — the only caller that passes Names straight through — never sees it. An unreachable branch cannot be covered. Co-Authored-By: Claude Opus 5 --- internal/compiler/compiler.go | 8 +- internal/compiler/wave26_for_defined_test.go | 418 +++++++++++++++++++ 2 files changed, 422 insertions(+), 4 deletions(-) create mode 100644 internal/compiler/wave26_for_defined_test.go diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index f9db4c69..c85378b0 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -2733,9 +2733,6 @@ func (c *Compiler) compileFor(v *ast.For) { parent := c.cur() owner, _ := parent.declOwner() declare := func(name string) { - if name == "" { - return // the nameless `*` in `for i, * in …` binds nothing - } if _, _, ok := parent.resolve(name); !ok { owner.localSlot(name) } @@ -2791,7 +2788,10 @@ func declareForTargetLocals(target ast.Node, declare func(string)) { declare(t.Name) case *ast.MultiAssign: if len(t.Targets) != len(t.Names) { - // All-locals form (`for i, in …`): Names carries every binding. + // All-locals form: Names carries every binding. The parser only builds + // it for a list of plain locals — `for i, in …` — so no entry is the + // empty name a nameless `*` would carry; every shape that HAS one (`for + // i, * in …`) comes with a parallel Targets list holding a nil there. for _, name := range t.Names { declare(name) } diff --git a/internal/compiler/wave26_for_defined_test.go b/internal/compiler/wave26_for_defined_test.go new file mode 100644 index 00000000..1760ee8f --- /dev/null +++ b/internal/compiler/wave26_for_defined_test.go @@ -0,0 +1,418 @@ +package compiler + +import ( + "strings" + "testing" + + "github.com/go-embedded-ruby/ruby/internal/bytecode" + "github.com/go-embedded-ruby/ruby/internal/object" + "github.com/go-ruby-parser/parser" + "github.com/go-ruby-parser/parser/ast" +) + +// compileSrc parses and compiles src, failing the test on either error. +func compileSrc(t *testing.T, src string) *bytecode.ISeq { + t.Helper() + prog, err := parser.Parse(src) + if err != nil { + t.Fatalf("parse %q: %v", src, err) + } + iseq, err := Compile(prog) + if err != nil { + t.Fatalf("compile %q: %v", src, err) + } + return iseq +} + +// hasLocal reports whether iseq's local table holds name. +func hasLocal(iseq *bytecode.ISeq, name string) bool { + for _, l := range iseq.Locals { + if l == name { + return true + } + } + return false +} + +// forBlock returns the child ISeq a `for` compiles its body into. A `for` emits +// exactly one child at the top level of these sources. +func forBlock(t *testing.T, iseq *bytecode.ISeq) *bytecode.ISeq { + t.Helper() + if len(iseq.Children) != 1 { + t.Fatalf("expected exactly one child ISeq, got %d", len(iseq.Children)) + } + return iseq.Children[0] +} + +// Every shape of loop variable declares its plain locals in the scope AROUND +// the loop, so they outlive it — `for` opens no scope. The shapes come in +// through two different parser fields: the simple lists through For.Vars, and +// everything Vars cannot spell through For.Target. +func TestForDeclaresLoopVariablesInTheEnclosingScope(t *testing.T) { + cases := []struct { + src string + want []string + unset []string + }{ + {src: "for i in [1]; end", want: []string{"i"}}, + {src: "for a, b in [[1,2]]; end", want: []string{"a", "b"}}, + {src: "for i, in [[1,2]]; end", want: []string{"i"}}, + {src: "for i, * in [[1,2]]; end", want: []string{"i"}}, + {src: "for i, *j in [[1,2]]; end", want: []string{"i", "j"}}, + {src: "for i, *j, k in [[1,2,3,4]]; end", want: []string{"i", "j", "k"}}, + {src: "for (i, j, k) in [[1,2,3]]; end", want: []string{"i", "j", "k"}}, + {src: "for i, (j, k) in [[1,[2,3]]]; end", want: []string{"i", "j", "k"}}, + // Non-local targets bind nothing in the local table. + {src: "for @v in [1]; end", unset: []string{"@v", "v"}}, + {src: "for $v in [1]; end", unset: []string{"$v", "v"}}, + {src: "for @@v in [1]; end", unset: []string{"@@v", "v"}}, + {src: "for V in [1]; end", unset: []string{"V"}}, + // A variable FIRST ASSIGNED in the body belongs to the scope around the + // loop too, and hops out of an enclosing `for` as well. + {src: "for a in [6]; c = a; end", want: []string{"a", "c"}}, + {src: "for a in [6]; for b in [7]; c = a * b; end; end", want: []string{"a", "b", "c"}}, + {src: "for a in [6]; d += 1; end", want: []string{"a", "d"}}, + {src: "for a in [6]; begin; raise; rescue => e; end; end", want: []string{"a", "e"}}, + } + for _, tc := range cases { + iseq := compileSrc(t, tc.src) + for _, name := range tc.want { + if !hasLocal(iseq, name) { + t.Errorf("%s: %q is not a local of the enclosing scope (locals %v)", tc.src, name, iseq.Locals) + } + } + for _, name := range tc.unset { + if hasLocal(iseq, name) { + t.Errorf("%s: %q should not be a local (locals %v)", tc.src, name, iseq.Locals) + } + } + } +} + +// An enclosing BLOCK is a scope, so a `for` inside one declares into the block, +// not into the method around it. The for_spec example "can be nested with blocks +// in between" pins exactly this. +func TestForInsideABlockDeclaresIntoTheBlock(t *testing.T) { + iseq := compileSrc(t, "[1].each { for c in [3]; c1 = c; end }") + if hasLocal(iseq, "c") || hasLocal(iseq, "c1") { + t.Fatalf("c/c1 leaked past the block into the top level: %v", iseq.Locals) + } + blk := iseq.Children[0] + if !hasLocal(blk, "c") || !hasLocal(blk, "c1") { + t.Fatalf("c/c1 are not locals of the enclosing block: %v", blk.Locals) + } +} + +// The each-block's hidden parameter is shaped by the kind of loop variable: a +// lone local takes one REQUIRED argument, everything else collects the yielded +// values into a REST Array. That is what lets `for x, y in o` see both values of +// an `each` that yields two. +func TestForBlockParameterShape(t *testing.T) { + cases := []struct { + src string + required int + splat int + }{ + {"for i in [1]; end", 1, -1}, + {"for a, b in [[1,2]]; end", 0, 0}, + {"for i, in [[1,2]]; end", 0, 0}, + {"for @v in [1]; end", 0, 0}, + } + for _, tc := range cases { + blk := forBlock(t, compileSrc(t, tc.src)) + if blk.NumRequired != tc.required || blk.SplatIndex != tc.splat { + t.Errorf("%s: NumRequired=%d SplatIndex=%d, want %d/%d", + tc.src, blk.NumRequired, blk.SplatIndex, tc.required, tc.splat) + } + } +} + +// A single non-local target takes the FIRST yielded value and drops the rest, +// which MRI spells `expandarray 1, 0` on the rest Array. A multi-target instead +// runs the length test that picks between args[0] and args. +func TestForTargetLowering(t *testing.T) { + blk := forBlock(t, compileSrc(t, "for @v in [1]; end")) + var sawExpand, sawSetIvar bool + for _, in := range blk.Insns { + if in.Op == bytecode.OpExpandArray && in.A == 1 && in.B == 0 && in.C == 0 { + sawExpand = true + } + if in.Op == bytecode.OpSetIvar { + sawSetIvar = true + } + } + if !sawExpand || !sawSetIvar { + t.Fatalf("for @v: expandarray(1,0)=%v setivar=%v", sawExpand, sawSetIvar) + } + + blk = forBlock(t, compileSrc(t, "for a, b in [[1,2]]; end")) + var sawLength, sawAref bool + for _, in := range blk.Insns { + if in.Op == bytecode.OpSend { + switch blk.Names[in.A] { + case "length": + sawLength = true + case "[]": + sawAref = true + } + } + } + if !sawLength || !sawAref { + t.Fatalf("multi-target for: length=%v aref=%v", sawLength, sawAref) + } +} + +// `for o&.x in m` must skip the setter on a nil receiver instead of raising, so +// the store arm emits the same nil guard an ordinary `o&.x = v` does. A +// multiple assignment never reaches that branch — `a&.x, b = …` is a SyntaxError. +func TestForSafeNavigationTargetGuardsOnNil(t *testing.T) { + for _, tc := range []struct { + src string + want bool + }{ + {"o = nil; for o&.x in [1]; end", true}, + {"o = nil; for o.x in [1]; end", false}, + } { + blk := forBlock(t, compileSrc(t, tc.src)) + var got bool + for _, in := range blk.Insns { + if in.Op == bytecode.OpBranchNil { + got = true + } + } + if got != tc.want { + t.Errorf("%s: BranchNil present = %v, want %v", tc.src, got, tc.want) + } + } +} + +// An index or attribute target is written through its setter, with the receiver +// and the index evaluated inside the block — once per iteration, as MRI does. +func TestForIndexAndAttributeTargets(t *testing.T) { + for _, tc := range []struct{ src, setter string }{ + {"arr = []; for arr[1] in [1]; end", "[]="}, + {"o = nil; for o.x in [1]; end", "x="}, + } { + blk := forBlock(t, compileSrc(t, tc.src)) + var got bool + for _, in := range blk.Insns { + if in.Op == bytecode.OpSend && blk.Names[in.A] == tc.setter { + got = true + } + } + if !got { + t.Errorf("%s: no %q send in the each-block", tc.src, tc.setter) + } + } +} + +// declareForTargetLocals ignores a target kind that names no local. The parser +// never builds this shape (a For.Target is always an assignable node), so it is +// synthesized to reach the walk's default arm. +func TestDeclareForTargetLocalsIgnoresANonTarget(t *testing.T) { + var got []string + declareForTargetLocals(&ast.IntLit{Value: 1}, func(n string) { got = append(got, n) }) + declareForTargetLocals(nil, func(n string) { got = append(got, n) }) + if len(got) != 0 { + t.Fatalf("expected no declarations, got %v", got) + } +} + +// `def f(...)` parks the forwarded arguments in the METHOD's locals, so a +// forwarding call written inside a block — a `for` body as much as any other — +// must read them from the enclosing frame. A depth of 0 indexes the block's own +// frame, which held one local and crashed. +func TestForwardingCallInsideABlockReadsTheMethodFrame(t *testing.T) { + for _, src := range []string{ + "def f(...); [1].each { g(...) }; end", + "def f(...); for x in [1]; g(...); end; end", + "class C; def f(...); [1].each { super(...) }; end; end", + } { + blocks := blockISeqs(compileSrc(t, src)) + if len(blocks) == 0 { + t.Fatalf("%s: no block ISeq compiled", src) + } + var sawDepth bool + for _, blk := range blocks { + for _, in := range blk.Insns { + if in.Op == bytecode.OpGetLocal && in.B > 0 { + sawDepth = true + } + } + } + if !sawDepth { + t.Errorf("%s: the block reads no enclosing-frame local; the forward locals were resolved at depth 0", src) + } + } +} + +// blockISeqs collects every block/for child ISeq in the tree. +func blockISeqs(iseq *bytecode.ISeq) []*bytecode.ISeq { + var out []*bytecode.ISeq + for _, ch := range iseq.Children { + if ch.Name == "" || ch.Name == "" { + out = append(out, ch) + } + out = append(out, blockISeqs(ch)...) + } + return out +} + +// mustResolve reports a compiler bug rather than returning a bogus slot when the +// forwarding locals are not in scope. +func TestMustResolveOutsideAForwardingMethod(t *testing.T) { + c := &Compiler{} + c.push(newBuilder("t", nil)) + defer func() { + r := recover() + if r == nil { + t.Fatal("expected mustResolve to panic outside a def(...) method") + } + if e, ok := r.(compileError); !ok || !strings.Contains(e.msg, "argument forwarding") { + t.Fatalf("expected an argument-forwarding compileError, got %#v", r) + } + }() + c.mustResolve(fwdRestName) +} + +// Block-local variables (`{ |a; b| … }`) are slots of the BLOCK, not parameters, +// and shadow any enclosing binding of the same name. +func TestBlockLocalsTakeTheirOwnSlots(t *testing.T) { + iseq := compileSrc(t, "x = 1; [1].each { |q; x, y| x = 9; y = 8 }") + blk := iseq.Children[0] + for _, name := range []string{"q", "x", "y"} { + if !hasLocal(blk, name) { + t.Fatalf("%q is not a local of the block: %v", name, blk.Locals) + } + } + if blk.NumRequired != 1 { + t.Fatalf("block locals must not be parameters: NumRequired=%d", blk.NumRequired) + } + // The write to x must reach the block's own slot (depth 0), not the outer one. + for _, in := range blk.Insns { + if in.Op == bytecode.OpSetLocal && in.B != 0 { + t.Fatalf("a block-local write escaped to depth %d", in.B) + } + } +} + +// `defined?` inspects its operand instead of compiling it, but the assignments +// inside it still DECLARE — MRI builds its local table in the parser. Without +// that, the next mention of the name has no slot and fails to compile. +func TestDefinedDeclaresTheLocalsItsOperandIntroduces(t *testing.T) { + cases := []struct { + src string + want []string + }{ + {"defined?(a += 1)", []string{"a"}}, + {"defined?(a = 1)", []string{"a"}}, + {"defined?(a, b = 1, 2)", []string{"a", "b"}}, + {"defined?(a, (b, c) = 1, [2, 3])", []string{"a", "b", "c"}}, + {"defined?(a, * = 1, 2)", []string{"a"}}, + {"defined?(a = (b = 1))", []string{"a", "b"}}, + {"defined?(a += (b = 1))", []string{"a", "b"}}, + {"defined?(foo(a = 1))", []string{"a"}}, + {"defined?(foo.bar(a = 1))", []string{"a"}}, + {"defined?(1 + (a = 1))", []string{"a"}}, + {"defined?(-(a = 1))", []string{"a"}}, + {"defined?(@v = (a = 1))", []string{"a"}}, + {"defined?(@@v = (a = 1))", []string{"a"}}, + {"defined?($v = (a = 1))", []string{"a"}}, + {"defined?(V = (a = 1))", []string{"a"}}, + {"defined?(Object::V = (a = 1))", []string{"a"}}, + {"defined?(a = 1); defined?(a = 2)", []string{"a"}}, + } + for _, tc := range cases { + iseq := compileSrc(t, tc.src) + for _, name := range tc.want { + if !hasLocal(iseq, name) { + t.Errorf("%s: %q got no slot (locals %v)", tc.src, name, iseq.Locals) + } + } + } + // And the point of it: the name is then readable as a local. + compileSrc(t, "defined?(a += 1); a") + compileSrc(t, "defined?(a += 1); defined?(a.b += 1)") +} + +// A defined? operand that assigns nothing declares nothing. +func TestDefinedDeclaresNothingForAPlainOperand(t *testing.T) { + iseq := compileSrc(t, "defined?(1 + 2)") + if len(iseq.Locals) != 0 { + t.Fatalf("expected no locals, got %v", iseq.Locals) + } +} + +// A compound assignment to an attribute or an index arrives as the parser's +// desugaring into a setter Call, but MRI tags it "assignment", not "method". A +// PLAIN `a[0] = 1` is an ordinary attrasgn call and stays "method". +func TestDefinedTagsDesugaredCompoundAssignment(t *testing.T) { + for _, src := range []string{ + "a = nil; defined?(a.b += 1)", + "a = nil; defined?(a.b ||= 1)", + "a = nil; defined?(a.b &&= 1)", + "a = nil; defined?(a[:b] += 1)", + "a = nil; defined?(a[:b] ||= 1)", + "a = nil; defined?(a[:b] &&= 1)", + } { + iseq := compileSrc(t, src) + if !hasDefinedTag(iseq, "assignment") { + t.Errorf("%s: no \"assignment\" tag (tags %v)", src, definedTags(iseq)) + } + if hasOp(iseq, bytecode.OpDefinedMethod) { + t.Errorf("%s: probed for a method; MRI answers from the syntax alone", src) + } + } + // A plain index or attribute assignment is NOT in MRI's DEFINED_ASGN group. + for _, src := range []string{"a = []; defined?(a[0] = 1)", "a = nil; defined?(a.b)"} { + iseq := compileSrc(t, src) + if hasDefinedTag(iseq, "assignment") { + t.Errorf("%s: tagged \"assignment\"; MRI says \"method\"", src) + } + // "method" is not a compile-time constant: OpDefinedMethod produces it at + // run time from the response test, so the probe itself is the witness. + if !hasOp(iseq, bytecode.OpDefinedMethod) { + t.Errorf("%s: expected a method probe (tags %v)", src, definedTags(iseq)) + } + } +} + +// definedTags collects every defined? tag String pushed anywhere in the tree. +func definedTags(iseq *bytecode.ISeq) []string { + var out []string + for _, v := range iseq.Consts { + if s, ok := v.(*object.String); ok { + switch s.Str() { + case "assignment", "method", "expression", "local-variable": + out = append(out, s.Str()) + } + } + } + for _, ch := range iseq.Children { + out = append(out, definedTags(ch)...) + } + return out +} + +func hasDefinedTag(iseq *bytecode.ISeq, tag string) bool { + for _, t := range definedTags(iseq) { + if t == tag { + return true + } + } + return false +} + +func hasOp(iseq *bytecode.ISeq, op bytecode.Op) bool { + for _, in := range iseq.Insns { + if in.Op == op { + return true + } + } + for _, ch := range iseq.Children { + if hasOp(ch, op) { + return true + } + } + return false +} From eca9725e7d5551fb91354d0896a8dc7766f31aed Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 08:21:21 +0200 Subject: [PATCH 6/7] compiler: defined? tags a constant `||=` as an assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `A ||= v` and `A::B ||= v` are not assignment nodes: the parser desugars them to `(defined?(A) && A) || A = v`, so they reached compileDefined as a `||` and were tagged "expression". MRI tags them "assignment" — PM_CONSTANT_OR_WRITE_NODE and PM_CONSTANT_PATH_OR_WRITE_NODE are in the DEFINED_ASGN group (prism_compile.c v3_4_0:4059 and 4063; compile.c's NODE_OP_CDECL at v3_4_0:6162 agrees) — and ruby/spec pins both. The `&&=` and `+=` forms already arrived as ConstAssign/ScopedConstAssign and were right. Recognised by AST pointer identity between the `defined?` probe's argument, the `&&`'s right operand and the scoped assignment's target, the same test opassign.go uses: a HAND-WRITTEN `(defined?(A) && A) || A = 1` has three distinct reads and stays "expression", which MRI confirms. language/defined_spec.rb: 211 -> 213 passing. Co-Authored-By: Claude Opus 5 --- internal/compiler/defined.go | 42 ++++++++++ internal/compiler/wave26_for_defined_test.go | 81 ++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/internal/compiler/defined.go b/internal/compiler/defined.go index 7931877e..c230b5bd 100644 --- a/internal/compiler/defined.go +++ b/internal/compiler/defined.go @@ -128,12 +128,54 @@ func (c *Compiler) compileDefinedCall(v *ast.Call) { // every other binary operator is a method on the left operand. func (c *Compiler) compileDefinedBinary(v *ast.BinaryExpr) { if v.Op == "&&" || v.Op == "||" { + // `A ||= v` and `A::B ||= v` are not written as an assignment node: the + // parser desugars them to `(defined?(A) && A) || A = v`, so they arrive + // here as a `||`. MRI still calls them assignments — PM_CONSTANT_OR_WRITE + // and PM_CONSTANT_PATH_OR_WRITE are in the DEFINED_ASGN group + // (prism_compile.c v3_4_0:4059 and 4063; compile.c's NODE_OP_CDECL at + // v3_4_0:6162 says the same) — and ruby/spec pins it. + if constOrAssign(v) { + c.pushDefinedTag("assignment") + return + } c.pushDefinedTag("expression") return } c.compileDefinedReceiverMethod(v.Left, v.Op) } +// constOrAssign recognises the parser's desugaring of `A ||= v` / `A::B ||= v`: +// `(defined?(A) && A) || A = v`, with the constant READ shared (same AST +// pointer) between the `defined?` probe, the `&&`'s right operand and — for the +// scoped form — the assignment's target. That identity is what tells the +// desugaring apart from a hand-written `(defined?(A) && A) || A = v`, whose +// three reads are distinct nodes; it is the same test opassign.go uses for the +// index and attribute compound assignments. +func constOrAssign(v *ast.BinaryExpr) bool { + if v.Op != "||" { + return false + } + guard, ok := v.Left.(*ast.BinaryExpr) + if !ok || guard.Op != "&&" { + return false + } + probe, ok := guard.Left.(*ast.Call) + if !ok || probe.Name != "defined?" || probe.Recv != nil || len(probe.Args) != 1 { + return false + } + if probe.Args[0] != guard.Right { + return false + } + switch w := v.Right.(type) { + case *ast.ConstAssign: + read, ok := guard.Right.(*ast.ConstRef) + return ok && read.Name == w.Name + case *ast.ScopedConstAssign: + return w.Target == guard.Right + } + return false +} + // compileDefinedReceiverMethod emits, under the guard, the response probe for // `recv.name`. recv == nil means an implicit self receiver. An explicit receiver // is itself subject to defined? first (MRI: `defined?(@x.m)` is nil when `@x` is diff --git a/internal/compiler/wave26_for_defined_test.go b/internal/compiler/wave26_for_defined_test.go index 1760ee8f..3410b46b 100644 --- a/internal/compiler/wave26_for_defined_test.go +++ b/internal/compiler/wave26_for_defined_test.go @@ -416,3 +416,84 @@ func hasOp(iseq *bytecode.ISeq, op bytecode.Op) bool { } return false } + +// `A ||= v` is not an assignment NODE: the parser desugars it to +// `(defined?(A) && A) || A = v`, with the constant read shared between the +// probe, the `&&`'s right operand and (for the scoped form) the assignment's +// target. MRI still calls it an assignment. A hand-written expression of the +// same shape has three distinct reads and stays "expression". +func TestDefinedTagsDesugaredConstantOrAssign(t *testing.T) { + for _, src := range []string{"defined?(A ||= 1)", "defined?(Object::A ||= 1)"} { + if iseq := compileSrc(t, src); !hasDefinedTag(iseq, "assignment") { + t.Errorf("%s: tags %v, want assignment", src, definedTags(iseq)) + } + } + for _, src := range []string{ + "X = 5; defined?((defined?(X) && X) || X = 1)", + "defined?(1 || 2)", + "defined?(1 && 2)", + "y = 3; defined?(y || 4)", + } { + if iseq := compileSrc(t, src); hasDefinedTag(iseq, "assignment") { + t.Errorf("%s: tagged assignment; MRI says expression", src) + } + } +} + +// Every way the recogniser must say no. The parser builds none of these shapes +// for a `||=`, so they are synthesized; each one leaves the expression an +// ordinary `||`, which defined? tags "expression". +func TestConstOrAssignRejectsOtherShapes(t *testing.T) { + read := &ast.ConstRef{Name: "A"} + probe := func(arg ast.Node) *ast.Call { return &ast.Call{Name: "defined?", Args: []ast.Node{arg}} } + guard := func(l, r ast.Node) *ast.BinaryExpr { return &ast.BinaryExpr{Op: "&&", Left: l, Right: r} } + one := &ast.IntLit{Value: 1} + cases := []struct { + name string + expr *ast.BinaryExpr + }{ + {"not ||", &ast.BinaryExpr{Op: "&&", Left: guard(probe(read), read), Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"left is not a binary expression", &ast.BinaryExpr{Op: "||", Left: read, Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"left is not &&", &ast.BinaryExpr{Op: "||", Left: &ast.BinaryExpr{Op: "+", Left: probe(read), Right: read}, + Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"guard's left is not a call", &ast.BinaryExpr{Op: "||", Left: guard(read, read), Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"probe is not defined?", &ast.BinaryExpr{Op: "||", Left: guard(&ast.Call{Name: "frozen?", Args: []ast.Node{read}}, read), + Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"probe has a receiver", &ast.BinaryExpr{Op: "||", Left: guard(&ast.Call{Recv: read, Name: "defined?", Args: []ast.Node{read}}, read), + Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"probe takes no argument", &ast.BinaryExpr{Op: "||", Left: guard(&ast.Call{Name: "defined?"}, read), + Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"probe reads a different node", &ast.BinaryExpr{Op: "||", Left: guard(probe(&ast.ConstRef{Name: "A"}), read), + Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"right is not an assignment", &ast.BinaryExpr{Op: "||", Left: guard(probe(read), read), Right: one}}, + {"assigns a different constant", &ast.BinaryExpr{Op: "||", Left: guard(probe(read), read), + Right: &ast.ConstAssign{Name: "B", Value: one}}}, + {"read is not a plain constant", func() *ast.BinaryExpr { + sc := &ast.ScopedConst{Recv: &ast.ConstRef{Name: "Object"}, Name: "A"} + return &ast.BinaryExpr{Op: "||", Left: guard(probe(sc), sc), Right: &ast.ConstAssign{Name: "A", Value: one}} + }()}, + {"scoped assignment targets another node", func() *ast.BinaryExpr { + sc := &ast.ScopedConst{Recv: &ast.ConstRef{Name: "Object"}, Name: "A"} + return &ast.BinaryExpr{Op: "||", Left: guard(probe(sc), sc), + Right: &ast.ScopedConstAssign{Target: &ast.ScopedConst{Recv: &ast.ConstRef{Name: "Object"}, Name: "A"}, Value: one}} + }()}, + } + for _, tc := range cases { + if constOrAssign(tc.expr) { + t.Errorf("%s: recognised as a constant ||=", tc.name) + } + } + // …and the two shapes it must say yes to. + sc := &ast.ScopedConst{Recv: &ast.ConstRef{Name: "Object"}, Name: "A"} + for _, tc := range []struct { + name string + expr *ast.BinaryExpr + }{ + {"A ||= 1", &ast.BinaryExpr{Op: "||", Left: guard(probe(read), read), Right: &ast.ConstAssign{Name: "A", Value: one}}}, + {"Object::A ||= 1", &ast.BinaryExpr{Op: "||", Left: guard(probe(sc), sc), Right: &ast.ScopedConstAssign{Target: sc, Value: one}}}, + } { + if !constOrAssign(tc.expr) { + t.Errorf("%s: not recognised", tc.name) + } + } +} From 4ed147bb5eba762cb1ba5535423c1bb015af80d5 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 23 Sep 2026 08:45:19 +0200 Subject: [PATCH 7/7] compiler: say accurately what the block-local comment claims The note said the parser rejects a block-local that repeats a parameter name. MRI's does (parse.y v3_4_0:13621, new_bv -> shadowing_lvar_0); go-ruby-parser does not yet, which is the one ruby/spec example in "Block-local variables" still failing. Nothing is forced here on its behalf: localLookup answers with the first slot of a name, so the repeated declaration is dead and the name keeps meaning the parameter. Comment only; no behaviour change. Co-Authored-By: Claude Opus 5 --- internal/compiler/compiler.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index c85378b0..0ab0e600 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -1556,9 +1556,15 @@ func (c *Compiler) compileBlock(blk *ast.Block) int { // reach the outer one. MRI builds exactly this table — prism_compile.c // v3_4_0:6325-6334 walks the block-parameters node's `locals` list and calls // pm_insert_local_index for each, after the requireds/optionals/rest/posts and - // before "fill in any locals we missed" — and the parser rejects a name that - // collides with a parameter (parse.y v3_4_0:13621 new_bv → shadowing_lvar_0), - // so no dedup is needed here. + // before "fill in any locals we missed". + // + // A block-local that repeats a parameter name (`{ |foo; foo| }`) is a + // SyntaxError in MRI, raised by the PARSER (parse.y v3_4_0:13621 new_bv → + // shadowing_lvar_0), and go-ruby-parser does not raise it yet — which is the + // one ruby/spec example in "Block-local variables" still failing. Nothing is + // forced here on its behalf: localLookup answers with the first slot of a + // name, so the repeated declaration is simply dead and the name keeps meaning + // the parameter. Raising belongs where MRI raises it. for _, name := range blk.Locals { b.localSlot(name) }