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

Hi,

At Sun, 10 Jul 2011 04:55:07 +0900,
Michael Edgar wrote in [ruby-core:37931]:
> 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.

It seems better but might error messages less user-readable.

Reduced patch based on the second patch with tests.


diff --git i/parse.y w/parse.y
index 9769bb9..af7ff27 100644
--- i/parse.y
+++ w/parse.y
@@ -426,6 +426,8 @@ static VALUE ripper_get_value(VALUE);
 #define get_value(val) ripper_get_value(val)
 static VALUE assignable_gen(struct parser_params*,VALUE);
 #define assignable(lhs,node) assignable_gen(parser, (lhs))
+static int id_is_var_gen(struct parser_params *parser, ID id);
+#define id_is_var(id) id_is_var_gen(parser, (id))
 #endif /* !RIPPER */

 static ID formal_argument_gen(struct parser_params*, ID);
@@ -699,7 +701,7 @@ static void token_info_pop(struct parser_params*,
const char *token);
 %type <node> lambda f_larglist lambda_body
 %type <node> brace_block cmd_brace_block do_block lhs none fitem
 %type <node> mlhs mlhs_head mlhs_basic mlhs_item mlhs_node mlhs_post mlhs_inner
-%type <id>   fsym variable sym symbol operation operation2 operation3
+%type <id>   fsym keyword_variable user_variable sym symbol operation
operation2 operation3
 %type <id>   cname fname op f_rest_arg f_block_arg opt_f_block_arg
f_norm_arg f_bad_arg
 /*%%%*/
 /*%
@@ -1601,10 +1603,14 @@ mlhs_post	: mlhs_item
 		    }
 		;

-mlhs_node	: variable
+mlhs_node	: user_variable
 		    {
 			$$ = assignable($1, 0);
 		    }
+		| keyword_variable
+		    {
+		        $$ = assignable($1, 0);
+		    }
 		| primary_value '[' opt_call_args rbracket
 		    {
 		    /*%%%*/
@@ -1671,7 +1677,7 @@ mlhs_node	: variable
 		    }
 		;

-lhs		: variable
+lhs		: user_variable
 		    {
 			$$ = assignable($1, 0);
 		    /*%%%*/
@@ -1680,6 +1686,15 @@ lhs		: variable
 			$$ = dispatch1(var_field, $$);
 		    %*/
 		    }
+		| keyword_variable
+		    {
+		        $$ = assignable($1, 0);
+		    /*%%%*/
+		        if (!$$) $$ = NEW_BEGIN(0);
+		    /*%
+		        $$ = dispatch1(var_field, $$);
+		    %*/
+		    }
 		| primary_value '[' opt_call_args rbracket
 		    {
 		    /*%%%*/
@@ -4257,12 +4272,14 @@ numeric 	: tINTEGER
 		    }
 		;

-variable	: tIDENTIFIER
+user_variable	: tIDENTIFIER
 		| tIVAR
 		| tGVAR
 		| tCONSTANT
 		| tCVAR
-		| keyword_nil {ifndef_ripper($$ = keyword_nil);}
+		;
+
+keyword_variable: keyword_nil {ifndef_ripper($$ = keyword_nil);}
 		| keyword_self {ifndef_ripper($$ = keyword_self);}
 		| keyword_true {ifndef_ripper($$ = keyword_true);}
 		| keyword_false {ifndef_ripper($$ = keyword_false);}
@@ -4271,7 +4288,20 @@ variable	: tIDENTIFIER
 		| keyword__ENCODING__ {ifndef_ripper($$ = keyword__ENCODING__);}
 		;

-var_ref		: variable
+var_ref		: user_variable
+		    {
+		    /*%%%*/
+			if (!($$ = gettable($1))) $$ = NEW_BEGIN(0);
+		    /*%
+			if (id_is_var(get_id($1))) {
+			    $$ = dispatch1(var_ref, $1);
+			}
+			else {
+			    $$ = dispatch1(vcall, $1);
+			}
+		    %*/
+		    }
+		| keyword_variable
 		    {
 		    /*%%%*/
 			if (!($$ = gettable($1))) $$ = NEW_BEGIN(0);
@@ -4281,7 +4311,7 @@ var_ref		: variable
 		    }
 		;

-var_lhs		: variable
+var_lhs		: user_variable
 		    {
 			$$ = assignable($1, 0);
 		    /*%%%*/
@@ -4289,6 +4319,14 @@ var_lhs		: variable
 			$$ = dispatch1(var_field, $$);
 		    %*/
 		    }
+		| keyword_variable
+		    {
+		        $$ = assignable($1, 0);
+		    /*%%%*/
+		    /*%
+			$$ = dispatch1(var_field, $$);
+		    %*/
+		    }
 		;

 backref		: tNTH_REF
@@ -8242,6 +8280,24 @@ gettable_gen(struct parser_params *parser, ID id)
     compile_error(PARSER_ARG "identifier %s is not valid to get",
rb_id2name(id));
     return 0;
 }
+#else  /* !RIPPER */
+static int
+id_is_var_gen(struct parser_params *parser, ID id)
+{
+    if (is_notop_id(id)) {
+	switch (id & ID_SCOPE_MASK) {
+	  case ID_GLOBAL: case ID_INSTANCE: case ID_CONST: case ID_CLASS:
+	    return 1;
+	  case ID_LOCAL:
+	    if (dyna_in_block() && dvar_defined(id)) return 1;
+	    if (local_id(id)) return 1;
+	    /* method call without arguments */
+	    return 0;
+	}
+    }
+    compile_error(PARSER_ARG "identifier %s is not valid to get",
rb_id2name(id));
+    return 0;
+}
 #endif /* !RIPPER */

 #ifdef RIPPER
diff --git i/test/ripper/test_parser_events.rb
w/test/ripper/test_parser_events.rb
index 14799d3..e6968e1 100644
--- i/test/ripper/test_parser_events.rb
+++ w/test/ripper/test_parser_events.rb
@@ -46,11 +46,15 @@ class TestRipper::ParserEvents < Test::Unit::TestCase
   end

   def test_var_ref
-    assert_equal '[ref(a)]', parse('a')
+    assert_equal '[assign(var_field(a),ref(a))]', parse('a=a')
     assert_equal '[ref(nil)]', parse('nil')
     assert_equal '[ref(true)]', parse('true')
   end

+  def test_vcall
+    assert_equal '[vcall(a)]', parse('a')
+  end
+
   def test_BEGIN
     assert_equal '[BEGIN([void()])]', parse('BEGIN{}')
     assert_equal '[BEGIN([ref(nil)])]', parse('BEGIN{nil}')
@@ -77,15 +81,15 @@ class TestRipper::ParserEvents < Test::Unit::TestCase
     assert_equal '[fcall(m,[])]', parse('m()')
     assert_equal '[fcall(m,[1])]', parse('m(1)')
     assert_equal '[fcall(m,[1,2])]', parse('m(1,2)')
-    assert_equal '[fcall(m,[*ref(r)])]', parse('m(*r)')
-    assert_equal '[fcall(m,[1,*ref(r)])]', parse('m(1,*r)')
-    assert_equal '[fcall(m,[1,2,*ref(r)])]', parse('m(1,2,*r)')
-    assert_equal '[fcall(m,[&ref(r)])]', parse('m(&r)')
-    assert_equal '[fcall(m,[1,&ref(r)])]', parse('m(1,&r)')
-    assert_equal '[fcall(m,[1,2,&ref(r)])]', parse('m(1,2,&r)')
-    assert_equal '[fcall(m,[*ref(a),&ref(b)])]', parse('m(*a,&b)')
-    assert_equal '[fcall(m,[1,*ref(a),&ref(b)])]', parse('m(1,*a,&b)')
-    assert_equal '[fcall(m,[1,2,*ref(a),&ref(b)])]', parse('m(1,2,*a,&b)')
+    assert_equal '[fcall(m,[*vcall(r)])]', parse('m(*r)')
+    assert_equal '[fcall(m,[1,*vcall(r)])]', parse('m(1,*r)')
+    assert_equal '[fcall(m,[1,2,*vcall(r)])]', parse('m(1,2,*r)')
+    assert_equal '[fcall(m,[&vcall(r)])]', parse('m(&r)')
+    assert_equal '[fcall(m,[1,&vcall(r)])]', parse('m(1,&r)')
+    assert_equal '[fcall(m,[1,2,&vcall(r)])]', parse('m(1,2,&r)')
+    assert_equal '[fcall(m,[*vcall(a),&vcall(b)])]', parse('m(*a,&b)')
+    assert_equal '[fcall(m,[1,*vcall(a),&vcall(b)])]', parse('m(1,*a,&b)')
+    assert_equal '[fcall(m,[1,2,*vcall(a),&vcall(b)])]', parse('m(1,2,*a,&b)')
   end

   def test_args_add
@@ -120,8 +124,8 @@ class TestRipper::ParserEvents < Test::Unit::TestCase
   end

   def test_aref
-    assert_equal '[aref(ref(v),[1])]', parse('v[1]')
-    assert_equal '[aref(ref(v),[1,2])]', parse('v[1,2]')
+    assert_equal '[aref(vcall(v),[1])]', parse('v[1]')
+    assert_equal '[aref(vcall(v),[1,2])]', parse('v[1,2]')
   end

   def test_assoclist_from_args
@@ -143,7 +147,7 @@ class TestRipper::ParserEvents < Test::Unit::TestCase
   end

   def test_aref_field
-    assert_equal '[assign(aref_field(ref(a),[1]),2)]', parse('a[1]=2')
+    assert_equal '[assign(aref_field(vcall(a),[1]),2)]', parse('a[1]=2')
   end

   def test_arg_ambiguous
@@ -323,7 +327,7 @@ class TestRipper::ParserEvents < Test::Unit::TestCase
       tree = parse("foo.()", :on_call) {thru_call = true}
     }
     assert_equal true, thru_call
-    assert_equal "[call(ref(foo),.,call,[])]", tree
+    assert_equal "[call(vcall(foo),.,call,[])]", tree
   end

   def test_excessed_comma


-- 
Nobu Nakada