From a08b9a8cf793ed87fae2600b290d67c835184275 Mon Sep 17 00:00:00 2001 From: Ilya Bylich Date: Fri, 24 Sep 2021 17:56:42 +0300 Subject: [PATCH] - ruby31.y: fix parsing of hash pairs with lvar/const keys --- doc/AST_FORMAT.md | 36 ++++++++++++++ lib/parser/builders/default.rb | 53 ++++++++++++++++---- lib/parser/messages.rb | 1 + lib/parser/ruby27.y | 86 ++++++++++++++++---------------- lib/parser/ruby30.y | 86 ++++++++++++++++---------------- lib/parser/ruby31.y | 89 ++++++++++++++++++---------------- test/test_parser.rb | 58 +++++++++++++++++----- 7 files changed, 264 insertions(+), 145 deletions(-) diff --git a/doc/AST_FORMAT.md b/doc/AST_FORMAT.md index c8c793a22..4c72f9157 100644 --- a/doc/AST_FORMAT.md +++ b/doc/AST_FORMAT.md @@ -266,6 +266,42 @@ Format: ~~~~~~~~~~ expression (pair) ~~~ +#### With local variable + +Format: + +~~~ +(pair (sym :foo) (lvar :foo)) +"{foo:}" + ^ operator (pair) + ~~~ expression (sym) + ~~~ expression (lvar) +~~~ + +#### With constant + +Format: + +~~~ +(pair (sym :foo) (const nil :foo)) +"{FOO:}" + ^ operator (pair) + ~~~ expression (const) + ~~~ expression (lvar) +~~~ + +#### With method call + +Format: + +~~~ +(pair (sym :puts) (send nil :puts)) +"{puts:}" + ^ operator (pair) + ~~~~ expression (sym) + ~~~~ expression (send) +~~~ + #### Plain Format: diff --git a/lib/parser/builders/default.rb b/lib/parser/builders/default.rb index 4ed33b9c2..e9e903031 100644 --- a/lib/parser/builders/default.rb +++ b/lib/parser/builders/default.rb @@ -518,6 +518,20 @@ def pair_quoted(begin_t, parts, end_t, value) n(:pair, [ key, value ], pair_map) end + def pair_label(key_t) + key_l = loc(key_t) + value_l = key_l.adjust(end_pos: -1) + + label = value(key_t) + value = + if label =~ /\A[[:lower:]]/ + n(:ident, [ label.to_sym ], Source::Map::Variable.new(value_l)) + else + n(:const, [ nil, label.to_sym ], Source::Map::Constant.new(nil, value_l, value_l)) + end + pair_keyword(key_t, accessible(value)) + end + def kwsplat(dstar_t, arg) n(:kwsplat, [ arg ], unary_op_map(dstar_t, arg)) @@ -608,19 +622,29 @@ def accessible(node) when :ident name, = *node - if @parser.static_env.declared?(name) - if name.to_s == parser.current_arg_stack.top - diagnostic :error, :circular_argument_reference, - { :var_name => name.to_s }, node.loc.expression - end + if %w[? !].any? { |c| name.to_s.end_with?(c) } + diagnostic :error, :invalid_id_to_get, + { :identifier => name.to_s }, node.loc.expression + end - node.updated(:lvar) - else - name, = *node - n(:send, [ nil, name ], + # Numbered parameters are not declared anywhere, + # so they take precedence over method calls in numblock contexts + if @parser.version >= 27 && @parser.try_declare_numparam(node) + return node.updated(:lvar) + end + + unless @parser.static_env.declared?(name) + return n(:send, [ nil, name ], var_send_map(node)) end + if name.to_s == parser.current_arg_stack.top + diagnostic :error, :circular_argument_reference, + { :var_name => name.to_s }, node.loc.expression + end + + node.updated(:lvar) + else node end @@ -683,6 +707,17 @@ def assignable(node) node.updated(:lvasgn) + when :match_var + name, = *node + + var_name = node.children[0].to_s + name_loc = node.loc.expression + + check_assignment_to_numparam(var_name, name_loc) + check_reserved_for_numparam(var_name, name_loc) + + node + when :nil, :self, :true, :false, :__FILE__, :__LINE__, :__ENCODING__ diagnostic :error, :invalid_assignment, nil, node.loc.expression diff --git a/lib/parser/messages.rb b/lib/parser/messages.rb index dbf6233d9..cbef0d70e 100644 --- a/lib/parser/messages.rb +++ b/lib/parser/messages.rb @@ -76,6 +76,7 @@ module Parser :duplicate_variable_name => 'duplicate variable name %{name}', :duplicate_pattern_key => 'duplicate hash pattern key %{name}', :endless_setter => 'setter method cannot be defined in an endless method definition', + :invalid_id_to_get => 'identifier %{identifier} is not valid to get', # Parser warnings :useless_else => 'else without rescue is useless', diff --git a/lib/parser/ruby27.y b/lib/parser/ruby27.y index 00710279a..5da29d1c6 100644 --- a/lib/parser/ruby27.y +++ b/lib/parser/ruby27.y @@ -2133,7 +2133,7 @@ opt_block_args_tail: p_variable: tIDENTIFIER { - result = @builder.match_var(val[0]) + result = @builder.assignable(@builder.match_var(val[0])) } p_var_ref: tCARET tIDENTIFIER @@ -2470,46 +2470,6 @@ keyword_variable: kNIL var_ref: user_variable { - if (node = val[0]) && node.type == :ident - name = node.children[0] - - if name =~ /\A_[1-9]\z/ && !static_env.declared?(name) && context.in_dynamic_block? - # definitely an implicit param - location = node.loc.expression - - if max_numparam_stack.has_ordinary_params? - diagnostic :error, :ordinary_param_defined, nil, [nil, location] - end - - raw_context = context.stack.dup - raw_max_numparam_stack = max_numparam_stack.stack.dup - - # ignore current block scope - raw_context.pop - raw_max_numparam_stack.pop - - raw_context.reverse_each do |outer_scope| - if outer_scope == :block || outer_scope == :lambda - outer_scope_has_numparams = raw_max_numparam_stack.pop > 0 - - if outer_scope_has_numparams - diagnostic :error, :numparam_used_in_outer_scope, nil, [nil, location] - else - # for now it's ok, but an outer scope can also be a block - # with numparams, so we need to continue - end - else - # found an outer scope that can't have numparams - # like def/class/etc - break - end - end - - static_env.declare(name) - max_numparam_stack.register(name[1].to_i) - end - end - result = @builder.accessible(val[0]) } | keyword_variable @@ -2978,3 +2938,47 @@ require 'parser' def default_encoding Encoding::UTF_8 end + + def try_declare_numparam(node) + name = node.children[0] + + if name =~ /\A_[1-9]\z/ && !static_env.declared?(name) && context.in_dynamic_block? + # definitely an implicit param + location = node.loc.expression + + if max_numparam_stack.has_ordinary_params? + diagnostic :error, :ordinary_param_defined, nil, [nil, location] + end + + raw_context = context.stack.dup + raw_max_numparam_stack = max_numparam_stack.stack.dup + + # ignore current block scope + raw_context.pop + raw_max_numparam_stack.pop + + raw_context.reverse_each do |outer_scope| + if outer_scope == :block || outer_scope == :lambda + outer_scope_has_numparams = raw_max_numparam_stack.pop > 0 + + if outer_scope_has_numparams + diagnostic :error, :numparam_used_in_outer_scope, nil, [nil, location] + else + # for now it's ok, but an outer scope can also be a block + # with numparams, so we need to continue + end + else + # found an outer scope that can't have numparams + # like def/class/etc + break + end + end + + static_env.declare(name) + max_numparam_stack.register(name[1].to_i) + + true + else + false + end + end diff --git a/lib/parser/ruby30.y b/lib/parser/ruby30.y index df4cecad4..c96a2bbc7 100644 --- a/lib/parser/ruby30.y +++ b/lib/parser/ruby30.y @@ -2219,7 +2219,7 @@ opt_block_args_tail: p_variable: tIDENTIFIER { - result = @builder.match_var(val[0]) + result = @builder.assignable(@builder.match_var(val[0])) } p_var_ref: tCARET tIDENTIFIER @@ -2556,46 +2556,6 @@ keyword_variable: kNIL var_ref: user_variable { - if (node = val[0]) && node.type == :ident - name = node.children[0] - - if name =~ /\A_[1-9]\z/ && !static_env.declared?(name) && context.in_dynamic_block? - # definitely an implicit param - location = node.loc.expression - - if max_numparam_stack.has_ordinary_params? - diagnostic :error, :ordinary_param_defined, nil, [nil, location] - end - - raw_context = context.stack.dup - raw_max_numparam_stack = max_numparam_stack.stack.dup - - # ignore current block scope - raw_context.pop - raw_max_numparam_stack.pop - - raw_context.reverse_each do |outer_scope| - if outer_scope == :block || outer_scope == :lambda - outer_scope_has_numparams = raw_max_numparam_stack.pop > 0 - - if outer_scope_has_numparams - diagnostic :error, :numparam_used_in_outer_scope, nil, [nil, location] - else - # for now it's ok, but an outer scope can also be a block - # with numparams, so we need to continue - end - else - # found an outer scope that can't have numparams - # like def/class/etc - break - end - end - - static_env.declare(name) - max_numparam_stack.register(name[1].to_i) - end - end - result = @builder.accessible(val[0]) } | keyword_variable @@ -3075,3 +3035,47 @@ require 'parser' diagnostic :error, :endless_setter, nil, name_t end end + + def try_declare_numparam(node) + name = node.children[0] + + if name =~ /\A_[1-9]\z/ && !static_env.declared?(name) && context.in_dynamic_block? + # definitely an implicit param + location = node.loc.expression + + if max_numparam_stack.has_ordinary_params? + diagnostic :error, :ordinary_param_defined, nil, [nil, location] + end + + raw_context = context.stack.dup + raw_max_numparam_stack = max_numparam_stack.stack.dup + + # ignore current block scope + raw_context.pop + raw_max_numparam_stack.pop + + raw_context.reverse_each do |outer_scope| + if outer_scope == :block || outer_scope == :lambda + outer_scope_has_numparams = raw_max_numparam_stack.pop > 0 + + if outer_scope_has_numparams + diagnostic :error, :numparam_used_in_outer_scope, nil, [nil, location] + else + # for now it's ok, but an outer scope can also be a block + # with numparams, so we need to continue + end + else + # found an outer scope that can't have numparams + # like def/class/etc + break + end + end + + static_env.declare(name) + max_numparam_stack.register(name[1].to_i) + + true + else + false + end + end diff --git a/lib/parser/ruby31.y b/lib/parser/ruby31.y index cc0b51e03..dda280be9 100644 --- a/lib/parser/ruby31.y +++ b/lib/parser/ruby31.y @@ -2288,7 +2288,7 @@ opt_block_args_tail: p_variable: tIDENTIFIER { - result = @builder.match_var(val[0]) + result = @builder.assignable(@builder.match_var(val[0])) } p_var_ref: tCARET tIDENTIFIER @@ -2650,46 +2650,6 @@ keyword_variable: kNIL var_ref: user_variable { - if (node = val[0]) && node.type == :ident - name = node.children[0] - - if name =~ /\A_[1-9]\z/ && !static_env.declared?(name) && context.in_dynamic_block? - # definitely an implicit param - location = node.loc.expression - - if max_numparam_stack.has_ordinary_params? - diagnostic :error, :ordinary_param_defined, nil, [nil, location] - end - - raw_context = context.stack.dup - raw_max_numparam_stack = max_numparam_stack.stack.dup - - # ignore current block scope - raw_context.pop - raw_max_numparam_stack.pop - - raw_context.reverse_each do |outer_scope| - if outer_scope == :block || outer_scope == :lambda - outer_scope_has_numparams = raw_max_numparam_stack.pop > 0 - - if outer_scope_has_numparams - diagnostic :error, :numparam_used_in_outer_scope, nil, [nil, location] - else - # for now it's ok, but an outer scope can also be a block - # with numparams, so we need to continue - end - else - # found an outer scope that can't have numparams - # like def/class/etc - break - end - end - - static_env.declare(name) - max_numparam_stack.register(name[1].to_i) - end - end - result = @builder.accessible(val[0]) } | keyword_variable @@ -3100,8 +3060,7 @@ f_opt_paren_args: f_paren_args } | tLABEL { - value = @builder.call_method(nil, nil, val[0]) - result = @builder.pair_keyword(val[0], value) + result = @builder.pair_label(val[0]) } | tSTRING_BEG string_contents tLABEL_END arg_value { @@ -3174,3 +3133,47 @@ require 'parser' diagnostic :error, :endless_setter, nil, name_t end end + + def try_declare_numparam(node) + name = node.children[0] + + if name =~ /\A_[1-9]\z/ && !static_env.declared?(name) && context.in_dynamic_block? + # definitely an implicit param + location = node.loc.expression + + if max_numparam_stack.has_ordinary_params? + diagnostic :error, :ordinary_param_defined, nil, [nil, location] + end + + raw_context = context.stack.dup + raw_max_numparam_stack = max_numparam_stack.stack.dup + + # ignore current block scope + raw_context.pop + raw_max_numparam_stack.pop + + raw_context.reverse_each do |outer_scope| + if outer_scope == :block || outer_scope == :lambda + outer_scope_has_numparams = raw_max_numparam_stack.pop > 0 + + if outer_scope_has_numparams + diagnostic :error, :numparam_used_in_outer_scope, nil, [nil, location] + else + # for now it's ok, but an outer scope can also be a block + # with numparams, so we need to continue + end + else + # found an outer scope that can't have numparams + # like def/class/etc + break + end + end + + static_env.declare(name) + max_numparam_stack.register(name[1].to_i) + + true + else + false + end + end diff --git a/test/test_parser.rb b/test/test_parser.rb index 4a71e1e7a..496324478 100644 --- a/test/test_parser.rb +++ b/test/test_parser.rb @@ -10070,19 +10070,41 @@ def test_private_endless_method_command_syntax SINCE_3_1) end - def test_value_omission + def test_hash_pair_value_omission assert_parses( s(:hash, - s(:pair, s(:sym, :a), s(:send, nil, :a)), - s(:pair, s(:sym, :b), s(:send, nil, :b))), - %q{{a:, b:}}, - %q{^ begin - | ^ end - | ^ operator (pair) - | ~ expression (pair.sym) - | ~~ expression (pair.send) - | ~~ expression (pair) - |~~~~~~~~ expression}, + s(:pair, s(:sym, :puts), s(:send, nil, :puts))), + %q{{puts:}}, + %q{ ^ operator (pair) + | ~~~~ expression (pair.sym) + | ~~~~ expression (pair.send) + | ~~~~ selector (pair.send) + | ~~~~~ expression (pair)}, + SINCE_3_1) + + assert_parses( + s(:hash, + s(:pair, s(:sym, :BAR), s(:const, nil, :BAR))), + %q{{BAR:}}, + %q{ ^ operator (pair) + | ~~~ expression (pair.sym) + | ~~~ expression (pair.const) + | ~~~ name (pair.const) + | ~~~~ expression (pair)}, + SINCE_3_1) + end + + def test_hash_pair_value_omission_invalid_label + assert_diagnoses( + [:error, :invalid_id_to_get, { :identifier => 'foo?' }], + %q{{ foo?: }}, + %q{ ^^^^ location}, + SINCE_3_1) + + assert_diagnoses( + [:error, :invalid_id_to_get, { :identifier => 'bar!' }], + %q{{ bar!: }}, + %q{ ^^^^ location}, SINCE_3_1) end @@ -10501,4 +10523,18 @@ def test_pin_expr | ~~~~~~~~~~~~~~~~~~~~~ expression (in_pattern.pin)}, SINCE_3_1) end + + def test_assignment_to_numparam_via_pattern_matching + assert_diagnoses( + [:error, :reserved_for_numparam, { :name => '_1' }], + %q{proc { 1 in _1 }}, + %q{ ~~ location}, + SINCE_3_0) + + assert_diagnoses( + [:error, :cant_assign_to_numparam, { :name => '_1' }], + %q{proc { _1; 1 in _1 }}, + %q{ ~~ location}, + SINCE_2_7) + end end