[ruby-core:37931] [Ruby 1.9 - Bug #5002] Ripper fails to distinguish local vars from vcalls [PATCH]

Issue #5002 has been updated by Michael Edgar.

File vcall.breaking.diff added
File vcall.same_errors.diff added

Ack - I missed how nil/self would be caught as vcalls there.

As you note, splitting the variable node is necessary. I split 'variable' into 'user_variable' and 'keyword_variable', removing 'variable' entirely (since it would give r/r conflicts). However, this turns out to be a good thing overall: every other use of the `variable` production is on the LHS (or LHS-like constructs, like rescue Foo => exc), which bar the use of keywords anyway through manual checking.

So, I have two patches: one which just replaces all other uses of `variable` with `user_variable`. This is "vcall.breaking.diff", as it breaks some previous error-case behavior. The allowed syntax is identical; the error reported on invalid syntax (self = 'foo'), is not. It will, in this patch, simply emit a generic bison "unexpected ..." error, as the grammar will not match such constructs.

The second patch adds back in `keyword_variable` productions, so that error reporting will be the same, even though the new productions will always generate errors. This is "vcall.same_errors.diff". For a patch release, I imagine maintaining the errors may be preferable, so I've included it for completeness.
----------------------------------------
Bug #5002: Ripper fails to distinguish local vars from vcalls [PATCH]
http://redmine.ruby-lang.org/issues/5002

Author: Michael Edgar
Status: Open
Priority: Normal
Assignee: 
Category: 
Target version: 1.9.3
ruby -v: -


Ripper always parses the `variable` grammar production (which includes identifiers, {i,c,g}vars, nil, __FILE__, etc) as a `var_ref` node, whose only child is the token itself.

This is a problem for one huge reason: local variables look exactly like vcalls: no-arg, no-receiver method calls. More importantly, the parse tree defines whether a given bareword identifier is a local variable reference or a method call. Thus, given a ripper parse tree, in order to distinguish local variable references from vcalls, one must reconstruct the parse order, re-implement the local variable introduction rules (local variable assigned in some way, for loops, block arg, rescue exception variable, named regex capture groups, ....), and then relabel those `var_ref` nodes which are method calls as `vcall` nodes.

This is quite a nasty workaround. There are a *lot* of edge cases to mess up. I've implemented it as the `ripper-plus` gem, but it's a huge pain, I'm not sure it's entirely correct, and is something the parser should be doing anyway.

The funny thing is, the parser already *is* doing almost all of the work! It's just not looking at the local variable tables when it comes time to generate the Ripper event. The patch I've attached does do so - it's a small change for a huge benefit for Ripper users.

I'd like to see this land in 1.9.3 - it's a small patch, and given the other bug fixes Ripper's had this cycle, would make Ripper pretty much sufficient for an entire Ruby implementation.


-- 
http://redmine.ruby-lang.org