|(a)| and |a| translate to the same parser tree

[nialbima]

I have no idea if I'm putting this in the right spot! Happy to move this to an issue if that's preferred.

I'm seeing an issue with the tree Parser builds for procargs that are arrays of length 1:

require "prism"

class Builder < Prism::Translation::Parser::Builder
  modernize
end

def parse(source)
  Prism::Translation::Parser34.new(Builder.new).parse(
    Parser::Source::Buffer.new("(example)", source: source)
  )
end

parse("x { |(a)| }") == parse("x { |a| }") # => true
parse("x { |(a)| }") # => s(:block, s(:send, nil, :x), s(:args, s(:procarg0, s(:arg, :a))), nil)

Those are definitely not the same in Ruby:

[[1]].map { |(a)| a } # => [1]
[[1]].map { |a| a }   # => [[1]]

It traces to def procarg0:

  def procarg0(arg)
    if self.class.emit_procarg0
      if arg.type == :arg && self.class.emit_arg_inside_procarg0     # ← path A: a bare |a| arrives as (arg :a)
        n(:procarg0, [ arg ],                                        #   wrapped: (procarg0 (arg :a))
          Source::Map::Collection.new(nil, nil, arg.location.expression))
      else
        arg.updated(:procarg0)                                       # ← path B: |(a)| arrives as (mlhs (arg :a))
      end                                                            #   retyped: (procarg0 (arg :a))
    else
      arg
    end
  end

So Path A produces the tree path Path B already produced. The bug was found while working on a gem that builds ASTs, and Claude dug out the failure case above. This is the generated summary of the issue:

The collapse happens in the parser gem's Builders::Default#procarg0 (lib/parser/builders/default.rb:992), which turns a parenthesized group into procarg0 by changing the mlhs node's type. With one element inside, that gives the same node emit_arg_inside_procarg0 builds for a bare argument. The translator passes the group straight through (compiler.rb:2044), on purpose since #2855, to match the parser gem. This was pointed out on the parser gem side in whitequark/parser#825 in 2021 and is still open. Since the parser gem no longer gets new grammar work, and Prism::Translation::Parser::Builder exists for exactly this kind of change (#3443, then itblock in #3481), I wonder whether the translator would take it.

Claude's suggestion here was to override procarg0 in Prism::Translation::Parser::Builder using an opt-in builder flag:

def procarg0(arg)
  if self.class.emit_mlhs_inside_procarg0 && arg.type == :mlhs
    n(:procarg0, [arg], Parser::Source::Map::Collection.new(nil, nil, arg.loc.expression))
  else
    super
  end
end

This might address the bug, but isn't the right fix because the builder-flag pattern is from Parser, not Prism (Claude has been shamed accordingly). Also, setting a builder flag to make your parser work is deeply, deeply irritating. A full override of procarg0 is likely a better fix:

  def procarg0(arg)
    if single_level_array_in_procarg?(arg)
      n(:procarg0, [arg], Parser::Source::Map::Collection.new(nil, nil, arg.loc.expression))
    else
      super
    end
  end

  def single_level_array_in_procarg?(arg)
    self.class.emit_arg_inside_procarg0 && 
      arg.type == :mlhs &&
      arg.children.length == 1 && 
      %i[arg mlhs].include?(arg.children.first.type)
  end

But this would change the contract with Parser, because Prism would handle the case correctly where Parser fails silently and produces incorrect output. I don't know whether this would be okay here.

This is the scope of the change:

  What changes and what doesn't (the narrow prototype round-trips all 13 shapes):

  ┌────────────────────────────────────────────────┬──────────────────────────────┬────────────────────────────────────┐
  │                     Source                     │            Today             │               After                │
  ├────────────────────────────────────────────────┼──────────────────────────────┼────────────────────────────────────┤
  │ |(a)|                                          │ procarg0(arg a), same as |a| │ procarg0(mlhs(arg a))              │
  ├────────────────────────────────────────────────┼──────────────────────────────┼────────────────────────────────────┤
  │ |((a))|                                        │ procarg0(mlhs(arg a))        │ procarg0(mlhs(mlhs(arg a)))        │
  ├────────────────────────────────────────────────┼──────────────────────────────┼────────────────────────────────────┤
  │ |((a, b))|                                     │ procarg0(mlhs(arg a, arg b)) │ procarg0(mlhs(mlhs(arg a, arg b))) │
  ├────────────────────────────────────────────────┼──────────────────────────────┼────────────────────────────────────┤
  │ |a|, |(a, b)|, |(*a)|, |(*)|, |(a, (b, c))|, … │ —                            │ unchanged                          │
  └────────────────────────────────────────────────┴──────────────────────────────┴────────────────────────────────────┘

So: is this worth pursuing? I know Prism 2 is coming out soon (no idea when), and I also know that Prism promises identical output to Parser.

[Earlopain]

Some bugs we fixed in prism that the parser gem gets wrong but it looks like this would be a rather easy fix in the parser gem instead, not related to actually parsing the code. I would just try and propose the fix to the parser gem.

[nialbima]

Opened on Parser here: whitequark/parser#1095

This is a pretty rare argument pattern, but I could fix it here temporarily if desired. Otherwise, I'm happy to wait for Parser to respond and let Prism + downstream gems just pick it up for free.

[kddnewton]

I agree this is an upstream issue and not on us, the purpose of the translation layer here is not to make a correct AST, but to match what the parse gem emits.