Backport #1939: Ripper doesn't handle local variables

• Status: Closed
• Assignee: yugui (Yuki Sonoda)

[judofyr (Magnus Holm)]

=begin
Ruby handles some code different when it encounters a local variable:

 whatever /25 # /

If whatever is a local variable it would be interpreted as "whatever / 25",
if not, "whatever(/25 # /)". Since Ripper doesn't handle local variables, it
will always interpreter it as the last. This makes Ripper pretty much useless
because it can't guarantee to parse it equally to Ruby.
=end

[yugui (Yuki Sonoda)]

• Priority changed from Normal to 5
• Target version set to 1.9.2

=begin

=end

[judofyr (Magnus Holm)]

=begin
Since nobody seems to be working on this, I thought I might give it a try. I should warn you that I really don't know parse.y, Bison or C, but it sure was fun trying to figure it all out! Please correct me if I've got something wrong.

The first issue is that Ripper doesn't have a local variable-table at all. This isn't difficult at all to fix; you only need to remove a few #ifndef RIPPER, and move some function calls out of /%%%/. It looks like these functions must be called in both Ruby and Ripper: shadowing_lvar, local_push, local_pop, arg_var, local_var, dyna_push, dyna_pop and new_bv. We must also add something like assignable() which only adds the var in the lvtbl without returning a NODE. Seems correct? If so, I've already done this.

The other issue is a little more complicated: In Ripper yylvar.val is set to the result of the scanner event and we discard the ID. This is because we want some of the parser events to include scanner events, but this also means we for instance can't call local_var($1), because it's a VALUE, not an ID. This is also a VALUE we can't control, because the user will probably redefine it in his SexpBuilder.

So it looks like we want to have two values in Ripper: The VALUE returned from the callback and the ID returned from the lexer (only for scanner event. nil otherwise). As far as I can see, it means we'll have to define YYSTYPE ourself, remove yylvar-declarations from %token and handle it all ourself. This means we'll have to do this:

• Rewrite ripper_dispatch0-5 to call .val on all the values, and alloc a YYSTYPE where .val is set to the result of the funcall.
• Add a macro like this in Ripper: #define GET_ID(x) x.id
• And something like this in Ruby: #define GET_ID(x) x
• Define all the macros I listed above (shadowing_lvar etc.) to call GET_ID(id):
  #define local_var(id) local_var_gen(parser, GET_ID(id));
• Make sure the lexer sets yylvar.id and ripper_dispatch_scan_event sets yylvar.val in Ripper.

Then, in my world, this would work :-)

However, I assume there is a much easier way. Any Bison/parse.y-gurus know a better solution?

If not, I would love to try to write a patch.
=end

[judofyr (Magnus Holm)]

• File patch.diff patch.diff added

=begin
Okay, this turned out to be a lot more easier than I expected.

See the attached diff for a complete patch. http://gist.github.com/198967 is the same patch split up in four different files, so you can more easily review it.

Description of the patches:

0001-test-ripper-test_parser_events.rb-Add-test-case-for.patch:
This patch adds a new test case for the bug. This requires the patch at #2169 in order to run.

0002-parse.y-Ripper-now-uses-a-struct-for-YYSTYPE.patch:
This patch makes Ripper use a struct instead of a union, so we can't get/set both the ID and the VALUE.

0003-parse.y-Expose-lvtbl-to-Ripper.patch:
This patch places makes the required functions visible to Ripper. I've simply placed #endif and #ifndef RIPPER around it (so it's easier to see what's actually changed). In the final patch, these should rather be re-organized. This also defines assignment_var, which is equal to assignable except it doesn't return a NODE.

0004-parse.y-Ripper-now-uses-the-lvtbl.patch:
This patch moves functions like local_push() out of the Ruby-only-macros, and makes sure .id is always called.

Can someone please review these patches? If everything seems fine, I can reorganize the functions, update the ChangeLog and push it to GitHub so Shyuohei can commit it to the SVN repo.
=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

=begin
Applied in changeset r25187.
=end

[zenspider (Ryan Davis)]

=begin

On Oct 2, 2009, at 04:47 , Nobuyoshi Nakada wrote:

Issue #1939 has been updated by Nobuyoshi Nakada.

Status changed from Open to Closed
% Done changed from 0 to 100

Applied in changeset r25187.

Awesome. Thank you.

=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Closed to Assigned
• Assignee changed from aamine (Minero Aoki) to yugui (Yuki Sonoda)
• Priority changed from 5 to Normal

=begin

=end

[nobu (Nobuyoshi Nakada)]

=begin
also needs r25198 and r25201.
=end

[nobu (Nobuyoshi Nakada)]

=begin
and r25209.
=end

[yugui (Yuki Sonoda)]

• Status changed from Assigned to Closed

=begin
This issue was solved with changeset r26001.
Magnus, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end