From cdd6e228769a0c7221b4cb5637406efe884ebe6b Mon Sep 17 00:00:00 2001 From: tannevaled Date: Mon, 28 Sep 2026 20:18:55 +0200 Subject: [PATCH] ast: record the brace on a hash literal (HashLit.Braced) `f({k: 1})` and `f(k: 1)` built structurally identical `ast.HashLit`s, so nothing downstream could tell a POSITIONAL Hash argument from KEYWORDS. `ast.Call` carries no position either, so a consumer could not re-read the source to recover the brace. Add one bit of written form, following MRI, which records exactly this bit in exactly this place: * `primary: tLBRACE assoc_list '}'` sets `RNODE_HASH($$)->nd_brace = TRUE` (parse.y-ruby_4_0:4415-4419); * `rb_node_hash_new` leaves it 0 for every hash the grammar assembles itself (parse.y-ruby_4_0:11784-11791); * `aryset_check` reads `!nd_brace` back to mean "keywords" (parse.y-ruby_4_0:13669). So `parseHashLiteral` sets Braced and `addKwPair` does not. The bit says only "written with braces" and is deliberately not conditional on being an argument, again as in MRI: `x = {a: 1}` is Braced too. A consumer asking "positional Hash argument?" asks Braced of a node it already found in Call.Args. `obj.m {}`, where the braces are a BLOCK, cannot be mislabelled: a `{` after a call never reaches parseHashLiteral, structurally the same defence as MRI's lexer, which picks tLBRACE (hash) or '{' / tLBRACE_ARG (block) before the grammar sees it (parse.y-ruby_4_0:11141-11153). There is no HashLit there to mislabel, and a test asserts it. The zero value is false, so no existing consumer changes. No existing test was modified; the full suite passes unchanged. Closes #40 Co-Authored-By: Claude Opus 5 --- ast/ast.go | 19 +++++ hash_braced_test.go | 171 ++++++++++++++++++++++++++++++++++++++++++++ parser.go | 11 ++- 3 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 hash_braced_test.go diff --git a/ast/ast.go b/ast/ast.go index 2a87117..6b85c85 100644 --- a/ast/ast.go +++ b/ast/ast.go @@ -72,9 +72,28 @@ type XStr struct{ Command string } type ArrayLit struct{ Elems []Node } // HashLit is a hash literal {k => v, …}; Keys[i] maps to Values[i]. +// +// Braced records one bit of WRITTEN FORM, exactly as MRI's own parser does: +// `primary: tLBRACE assoc_list '}'` sets `RNODE_HASH($$)->nd_brace = TRUE` +// (parse.y-ruby_4_0:4415-4419), while `rb_node_hash_new` leaves it 0 for every +// hash the grammar assembles itself (parse.y-ruby_4_0:11784-11791). The two +// spellings mean different things at a call site: `f({k: 1})` passes a +// POSITIONAL Hash and `f(k: 1)` passes KEYWORDS. MRI reads the bit back for +// exactly that question (aryset_check, parse.y-ruby_4_0:13669). +// +// The bit says only "written with braces". It is NOT conditional on being an +// argument, again as in MRI: `x = {a: 1}` is Braced too. A consumer that wants +// "positional Hash argument" asks Braced of a node it already found in +// Call.Args. A `{` that opens a BLOCK never reaches the hash-literal +// production at all — that disambiguation is the lexer's +// (parse.y-ruby_4_0:11141-11153) and ours (see parseHashLiteral) — so +// `obj.m {}` builds no HashLit to ask. type HashLit struct { Keys []Node Values []Node + // Braced is true when the hash was written as `{…}`, and false when the + // parser collected it from bare `k: v` pairs in an argument list. + Braced bool } // RangeLit is a range literal: Lo..Hi (inclusive) or Lo...Hi (Exclusive). diff --git a/hash_braced_test.go b/hash_braced_test.go new file mode 100644 index 0000000..f59229a --- /dev/null +++ b/hash_braced_test.go @@ -0,0 +1,171 @@ +package parser_test + +import ( + "testing" + + "github.com/go-ruby-parser/parser" + "github.com/go-ruby-parser/parser/ast" +) + +// bracedTopCall parses src and returns its single top-level Call. +func bracedTopCall(t *testing.T, src string) *ast.Call { + t.Helper() + prog, err := parser.Parse(src) + if err != nil { + t.Fatalf("Parse(%q): %v", src, err) + } + if len(prog.Body) != 1 { + t.Fatalf("Parse(%q): want 1 top-level node, got %d", src, len(prog.Body)) + } + call, ok := prog.Body[0].(*ast.Call) + if !ok { + t.Fatalf("Parse(%q): top node is %T, want *ast.Call", src, prog.Body[0]) + } + return call +} + +// TestHashLitBracedAtCallSite is the bit itself: at a call site a braced hash +// literal is a POSITIONAL Hash and bare `k: v` pairs are KEYWORDS, and the two +// build structurally identical HashLits that only Braced tells apart. +// +// MRI draws the same line in the same place: `primary: tLBRACE assoc_list '}'` +// sets nd_brace (parse.y-ruby_4_0:4415-4419) and nothing else does +// (rb_node_hash_new, parse.y-ruby_4_0:11784-11791). +func TestHashLitBracedAtCallSite(t *testing.T) { + cases := []struct { + src string + argIdx int + wantBraced bool + wantPairs int + }{ + // The pair from issue #40. Same Keys/Values, different written form. + {"f({k: 1})", 0, true, 1}, + {"f(k: 1)", 0, false, 1}, + // Empty braces are still a positional Hash — and `f()` below shows the + // contrast is not "is there a hash", since there it is absent entirely. + {"f({})", 0, true, 0}, + // A double-splat INSIDE braces builds a braced hash; bare at the call + // site it is the keyword form. `assoc: tDSTAR arg_value` + // (parse.y-ruby_4_0:6682) is reached from both, so only Braced differs. + {"f({**h})", 0, true, 1}, + {"f(**h)", 0, false, 1}, + {"f(k: 1, **h)", 0, false, 2}, + // Hashrocket form is braced just the same. + {"f({1 => 2})", 0, true, 1}, + // A braced literal after a splat is still positional. + {"f(*a, {k: 1})", 1, true, 1}, + // Keywords collected after an ordinary positional argument. + {"f(a, k: 1)", 1, false, 1}, + // An array literal shares the argument grammar, so the same two + // spellings must stay distinguishable there too. + {"[1, k: 2]", -1, false, 1}, + {"[1, {k: 2}]", -1, true, 1}, + } + for _, tc := range cases { + t.Run(tc.src, func(t *testing.T) { + var node ast.Node + if tc.argIdx < 0 { + prog, err := parser.Parse(tc.src) + if err != nil { + t.Fatalf("Parse(%q): %v", tc.src, err) + } + arr, ok := prog.Body[0].(*ast.ArrayLit) + if !ok { + t.Fatalf("Parse(%q): top node is %T, want *ast.ArrayLit", tc.src, prog.Body[0]) + } + node = arr.Elems[len(arr.Elems)-1] + } else { + call := bracedTopCall(t, tc.src) + if tc.argIdx >= len(call.Args) { + t.Fatalf("Parse(%q): want an arg at %d, got %d args", tc.src, tc.argIdx, len(call.Args)) + } + node = call.Args[tc.argIdx] + } + h, ok := node.(*ast.HashLit) + if !ok { + t.Fatalf("Parse(%q): node is %T, want *ast.HashLit", tc.src, node) + } + if h.Braced != tc.wantBraced { + t.Errorf("Parse(%q): Braced = %v, want %v", tc.src, h.Braced, tc.wantBraced) + } + if len(h.Keys) != tc.wantPairs || len(h.Values) != tc.wantPairs { + t.Errorf("Parse(%q): %d keys / %d values, want %d each", + tc.src, len(h.Keys), len(h.Values), tc.wantPairs) + } + }) + } +} + +// TestHashLitBracedOutsideArguments pins the bit down as WRITTEN FORM and not +// "is a keyword argument": a braced literal in a non-argument position is +// Braced as well, which is what MRI records too — nd_brace is set by the +// literal production with no call-site condition on it. +func TestHashLitBracedOutsideArguments(t *testing.T) { + for _, src := range []string{"{a: 1}", "x = {a: 1}", "{1 => 2}", "{}"} { + prog, err := parser.Parse(src) + if err != nil { + t.Fatalf("Parse(%q): %v", src, err) + } + node := prog.Body[0] + if a, ok := node.(*ast.Assign); ok { + node = a.Value + } + h, ok := node.(*ast.HashLit) + if !ok { + t.Fatalf("Parse(%q): node is %T, want *ast.HashLit", src, node) + } + if !h.Braced { + t.Errorf("Parse(%q): Braced = false, want true", src) + } + } +} + +// TestHashLitBracedDoesNotClaimBlocks is the case a careless rule breaks the +// language on: in `obj.m {}` the braces are a BLOCK, not a hash. Ours cannot +// confuse them because a `{` after a call never reaches parseHashLiteral — +// structurally the same defence as MRI's lexer, which decides between tLBRACE +// (hash) and '{' / tLBRACE_ARG (block) before the grammar ever sees it +// (parse.y-ruby_4_0:11141-11153). So there is no HashLit here to mislabel. +func TestHashLitBracedDoesNotClaimBlocks(t *testing.T) { + for _, src := range []string{"obj.m {}", "obj.m { |x| x }", "f {}", "f { |x| x }"} { + call := bracedTopCall(t, src) + if call.Block == nil { + t.Fatalf("Parse(%q): want a Block, got none", src) + } + for i, arg := range call.Args { + if h, ok := arg.(*ast.HashLit); ok { + t.Errorf("Parse(%q): arg %d is a HashLit{Braced:%v}; the braces are a block", + src, i, h.Braced) + } + } + } + // And a real braced argument alongside a real block: both readings at once. + call := bracedTopCall(t, "obj.m({k: 1}) { |y| y }") + if call.Block == nil { + t.Fatal(`Parse("obj.m({k: 1}) { |y| y }"): want a Block, got none`) + } + if len(call.Args) != 1 { + t.Fatalf(`Parse("obj.m({k: 1}) { |y| y }"): want 1 arg, got %d`, len(call.Args)) + } + h, ok := call.Args[0].(*ast.HashLit) + if !ok { + t.Fatalf(`Parse("obj.m({k: 1}) { |y| y }"): arg is %T, want *ast.HashLit`, call.Args[0]) + } + if !h.Braced { + t.Error(`Parse("obj.m({k: 1}) { |y| y }"): Braced = false, want true`) + } +} + +// TestHashLitBracedAbsentWhenNoHash guards the probe against measuring nothing: +// `f()` and `f(h)` must build NO HashLit at all, so the `f({})`/`f({k: 1})` +// results above are about the brace and not about the argument count. +func TestHashLitBracedAbsentWhenNoHash(t *testing.T) { + for _, src := range []string{"f()", "f(h)", "f"} { + call := bracedTopCall(t, src) + for i, arg := range call.Args { + if _, ok := arg.(*ast.HashLit); ok { + t.Errorf("Parse(%q): arg %d is a HashLit, want none", src, i) + } + } + } +} diff --git a/parser.go b/parser.go index 3bcad0f..f6b4ecc 100644 --- a/parser.go +++ b/parser.go @@ -3318,7 +3318,11 @@ func (p *Parser) parseHashLiteral() ast.Node { // `{k: x rescue nil}` and `{1 => x rescue nil}` are SyntaxErrors where // `{k: (x rescue nil)}` and `{k: a = x rescue nil}` (an `arg_rhs`) are legal. defer p.permitRescueMod(false)() - h := &ast.HashLit{} + // The only place a `{…}` hash literal is built, which is why Braced can be + // set unconditionally here: MRI sets nd_brace on exactly this production + // (`primary: tLBRACE assoc_list '}'`, parse.y-ruby_4_0:4415-4419). A `{` + // that opens a block is routed away before this point (see above). + h := &ast.HashLit{Braced: true} p.skipNewlines() for !p.is(token.RBRACE) { var k, v ast.Node @@ -4414,7 +4418,10 @@ func (p *Parser) atAnonForwardEnd() bool { } // addKwPair appends a key/value pair to the implicit trailing-hash argument, -// allocating it on first use. +// allocating it on first use. This hash is assembled by the parser from bare +// `k: v` pairs, so it is deliberately NOT Braced: it is the KEYWORD form, the +// `f(k: 1)` of #40. MRI leaves nd_brace at its 0 default here for the same +// reason (rb_node_hash_new, parse.y-ruby_4_0:11784-11791). func (p *Parser) addKwPair(kw **ast.HashLit, k, v ast.Node) { if *kw == nil { *kw = &ast.HashLit{}