compiler: _1 beside a written parameter list is a SyntaxError

internal/compiler/compiler.go

@@ -48,6 +48,11 @@ type builder struct {
 	children    []*bytecode.ISeq
 	parent      *builder
 	isBlock     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
+	// the block a SyntaxError instead of a method call; see compileCall.
+	writtenParams 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.
@@ -1337,6 +1342,27 @@ func (c *Compiler) compileCall(v *ast.Call) {
 		b.emit(bytecode.OpPushConst, b.addConst(object.IntValue(int64(b.curLine))), 0)
 		return
 	}
+	// ⛔ `_1`..`_9` inside a block that WROTE its parameter list is a SyntaxError,
+	// not a method call. MRI's lexer reserves the nine names for the
+	// numbered-parameter mechanism, so a block cannot have both: `proc { |x| _1 }`
+	// is rejected at parse time with "ordinary parameter is defined", and so are
+	// the empty-list forms `proc { || _1 }` and `-> () { _1 }` — writing `||` at
+	// all is what declares the list, not what it contains.
+	// language/numbered_parameters_spec.rb:49 pins all eight shapes.
+	//
+	// Reaching here means the name resolved to no local, so it was about to be
+	// compiled as a receiverless send and fail at RUNTIME with NameError — which
+	// is what rbgo did, and only for the shapes that were actually called:
+	// `proc { |x| _1 }` on its own raised nothing at all.
+	//
+	// A `for` body is excluded: forScope is the each-block `for` lowers into, and
+	// `for` opens no Ruby scope, so the numbered-parameter rules do not reach it.
+	// Confined to the bare, argumentless, receiverless, blockless name, matching
+	// the shape of the __LINE__ and block_given? intrinsics above.
+	if v.Recv == nil && v.Block == nil && len(callArgs) == 0 &&
+		isNumberedParamName(v.Name) && b.isBlock && b.writtenParams && !b.forScope {
+		c.fail("ordinary parameter is defined")
+	}
 	// block_given? is a frame intrinsic, not a real dispatch.
 	if v.Recv == nil && v.Block == nil && v.Name == "block_given?" && len(callArgs) == 0 {
 		b.emit(bytecode.OpBlockGiven, 0, 0)
@@ -2116,6 +2142,8 @@ func (c *Compiler) compileBlock(blk *ast.Block) int {
 		b.emit(bytecode.OpPop, 0, 0)
 		b.patch(skip, b.here())
 	}
+	// Recorded BEFORE the body is compiled, because the body is where `_1` is met.
+	b.writtenParams = !blockParamsAreImplicit(blk)
 	c.ctxs = append(c.ctxs, &loopCtx{kind: ctxBlock})
 	c.compileBody(blk.Body)
 	c.ctxs = c.ctxs[:len(c.ctxs)-1]

internal/compiler/numbered_param_ordinary_test.go

@@ -0,0 +1,105 @@
+// Copyright (c) the go-embedded-ruby/ruby authors
+//
+// SPDX-License-Identifier: BSD-3-Clause
+
+package compiler
+
+import (
+	"strings"
+	"testing"
+)
+
+// A block cannot have both a WRITTEN parameter list and a numbered parameter:
+// MRI's lexer reserves `_1`..`_9` for the numbered-parameter mechanism, so
+// `proc { |x| _1 }` is rejected at parse time with "ordinary parameter is
+// defined". Writing `||` at all is what declares the list, which is why the
+// empty forms are rejected too.
+//
+// ⛔ Unlike its neighbour TestNumberedParameterNameIsReserved, NOT ONE row here
+// was run against a reference interpreter: the Ruby on this machine is 2.6.10
+// and numbered parameters arrived in 2.7, so the local oracle cannot answer any
+// of these. The authority is language/numbered_parameters_spec.rb:49, which pins
+// all eight shapes below verbatim, and MRI's own rule that the nine names are
+// reserved by the grammar. Said plainly so nobody reads a measurement here that
+// was never taken.
+//
+// Before this check the names resolved to no local and were compiled as
+// receiverless sends, so the shapes that were CALLED failed at runtime with
+// NameError and the ones that were only defined — `proc { |x| _1 }` on its own —
+// raised nothing at all.
+func TestNumberedParameterWithOrdinaryParameters(t *testing.T) {
+	const want = "ordinary parameter is defined"
+	refused := []string{
+		// The eight shapes the spec lists, in its order: a written list that is
+		// empty and one that is not, across ->, proc, lambda and a method block.
+		"-> () { _1 }",
+		"-> (x) { _1 }",
+		"proc { || _1 }",
+		"proc { |x| _1 }",
+		"lambda { || _1 }",
+		"lambda { |x| _1 }",
+		"['a'].map { || _1 }",
+		"['a'].map { |x| _1 }",
+		// The same rule reaches the other nine names and the other kinds of
+		// written parameter — a block parameter and a keyword are still a written
+		// list, even though neither is positional.
+		"proc { |x| _9 }",
+		"proc { |&blk| _1 }",
+		"proc { |k: 1| _1 }",
+	}
+	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. Each one would be broken by a check that was too eager, and
+	// the first two are the ones that matter most: the numbered parameter's whole
+	// reason for existing is the block that writes NO list.
+	allowed := []string{
+		"proc { _1 }",              // synthesised by the parser — the normal case
+		"proc { _1 + _2 }",         // and more than one of them
+		"proc { |x| x }",           // an ordinary written parameter, no _N in sight
+		"proc { |x| _0 }",          // _0 is an ordinary identifier
+		"proc { |x| _10 }",         // so is _10
+		"proc { |x| _1a }",         // and so is anything longer
+		"proc { |x| @_1 }",         // an ivar is not a numbered parameter
+		"proc { |x| self._1 }",     // an explicit receiver is an ordinary call
+		"proc { |x| _1(2) }",       // so is a call with arguments
+		"proc { |x| proc { _1 } }", // the INNER block wrote no list; _1 is its own
+		"def m; _1; end",           // not a block at all: left as a runtime NameError
+		"_1",                       // nor at the top level
+		"for x in [1]; _1; end",    // `for` opens no Ruby scope, so the rule misses it
+	}
+	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: the two refusals must stay DISTINGUISHABLE.
+// Binding `_1` is "reserved for numbered parameters" (storeLocal) and reading it
+// beside an ordinary parameter is "ordinary parameter is defined" (compileCall).
+// They are different sentences in MRI and the specs match on them separately —
+// `/_1 is reserved for numbered parameter/` and `/ordinary parameter is defined/`
+// — so a fix that collapsed them into one message would satisfy neither spec.
+func TestNumberedParameterRefusalsStayDistinct(t *testing.T) {
+	for _, tc := range []struct{ src, want, notWant string }{
+		{"proc { _1 = 0 }", "reserved for numbered parameters", "ordinary parameter is defined"},
+		{"proc { |x| _1 }", "ordinary parameter is defined", "reserved for numbered parameters"},
+	} {
+		_, err := compileString(tc.src)
+		if err == nil {
+			t.Errorf("%q: compiled, want a refusal", tc.src)
+			continue
+		}
+		if !strings.Contains(err.Error(), tc.want) {
+			t.Errorf("%q: err = %v, want one containing %q", tc.src, err, tc.want)
+		}
+		if strings.Contains(err.Error(), tc.notWant) {
+			t.Errorf("%q: err = %v, must NOT contain %q", tc.src, err, tc.notWant)
+		}
+	}
+}