whitequark / parser

A Ruby parser.
Other
1.57k stars 197 forks source link

Unary `+` precedence #1016

Open kddnewton opened 1 month ago

kddnewton commented 1 month ago

There are two issues with unary + precedence that I can find.

The first is that the parser is creating a send node, when there isn't really going to be a method call. This is, for example:

class Integer; def +@; self * 10; end; end

+5 ** 2 # => 25
+ 5 ** 2 # => 2500

As you can see, the space affects the result. If there is no space, then no method call is generated. However, in both cases you get the same AST:

(send
  (send
    (int 5) :**
    (int 2)) :+@)

The other issue is that the precedence in this AST is incorrect in both cases. This is implying that the expression should be parsed as:

+(5 ** 2)

but in reality, in both cases it should be parsed as:

(+5) ** 2 # => 25
(+ 5) ** 2 # => 2500
marcandre commented 1 month ago

You must note that this is particular to integer constants. +5 is interpreted by Ruby as a literal corresponding to5, but foo = 5; +foo will behave how you expected it to.

OTOH, there does indeed seem to be a bug in the result from parser, in the case of +5 I think there shouldn't be a send :@+ at all.

Note that this is only the case with integer literals:

$ ruby-parse -e "foo = 5; +foo ** 2"
(begin
  (lvasgn :foo
    (int 5))
  (send
    (send
      (lvar :foo) :+@) :**
    (int 2)))
kddnewton commented 1 month ago

I believe this will be a problem with any number constants, not just integers.

marcandre commented 1 month ago

Sorry, I did mean number, you are correct.

iliabylich commented 1 month ago

Thanks for reporting. Does this bug (or #1017) impact Prism's test suite? If not I'll fix it "later", nobody else seems to need it.

kddnewton commented 1 month ago

Thanks @iliabylich no rush at all, I'm just excluding those tests from the test suite that compares the AST translation. So it's no rush.