Always use local variables in current context to parse code #397

tompng · 2022-08-31T15:09:11Z

Ruby will parse differently depending on defined local_variables.

a = 1
a +1 # a + 1
b +1 # b(+1)
a *a # a * a
b *b # b(*b)
a **a # a ** a
b **b # b(**b)
a %(1) # a % 1
b %(1) # b("1")
a /1/i # a / 1 / i
b /1/i # b(regexp)

Therefore coloring, indent, code_block_open and assignment_expression? needs local_variables in current context.

assignment expression check

The code below is an assignment expression and the evaluated result (large array) should be truncated.

a /"/i if false; a=1; x=1000.times.to_a#".size

But once it is evaluated, it turns into a division expression(a / "string".size).
Assignment expression check should be done before evaluation.

semicolon and newline character

There are ;\n in the code to be parsed. "a=b=nil;\n#{original_code}"
Without \n, syntax valid code =begin\n=end will be syntax error.
Without ;, .to_s will wrongly be syntax ok.

…l and assignment_expression check

…ight define new localvars and change result of assignment_expression?

st0012 · 2022-10-08T10:36:28Z

lib/irb/ruby-lex.rb

@@ -269,18 +275,15 @@ def each_top_level_statement
    end
  end

-  def lex
+  def lex(context: nil)


Is this and the below context passing necessary? It looks like lex is never called with context argument?

This context is mainly used to test RubyLex's nesting_level and code_block_open calculation in test_ruby_lex.rb

I fixed to call lex with context in L248.

I also changed the optional keyword parameter context: to normal required parameter.

Not to forget calling method without context

Method in this library that receives only context is using normal parameter, not keyword parameter. (ex: set_auto_indent(context), suspend_context(context))

Is it necessary?

Yes.
Since RubyLex#lex just returns @input.call in mulitline mode, this code is needed only when USE_MULTILINE is false.
Example when nil is passed to context here:

% irb --nomultiline irb(main):001:0> a = 8 => 8 irb(main):002:0> a /2 #=> should be 4 irb(main):003:0/ irb(main):004:0/

st0012 · 2022-10-08T10:38:30Z

lib/irb/ruby-lex.rb

-    @indent = process_nesting_level
-    @ltype = process_literal_type
+    @tokens = self.class.ripper_lex_without_warning(code, context: context)
+    @ltype, @indent, @continue, @code_block_open = check_state(code, @tokens, context: context)


Good catch on using check_state instead 👍

st0012 · 2022-10-08T10:40:50Z

lib/irb/ruby-lex.rb

@@ -136,16 +136,20 @@ def set_prompt(p = nil, &block)
    :on_param_error
  ]

+  def self.local_variables_assign_code(context: nil, local_variables: [])


Can we call it generate_local_variables_assign_code?

st0012 · 2022-10-08T10:43:43Z

test/irb/test_color.rb

+      result_without_lvars = "a #{RED}#{BOLD}/#{CLEAR}#{RED}(b +1)#{CLEAR}#{RED}#{BOLD}/i#{CLEAR}"
+      result_with_lvar = "a /(b #{BLUE}#{BOLD}+1#{CLEAR})/i"
+      result_with_lvars = "a /(b +#{BLUE}#{BOLD}1#{CLEAR})/i"
+      if colorize_code_supported?


This is not necessary because irb requires Ruby 2.5+, which always satisfy this condition. I added #413 to remove this obsolete check.

code_block_open is not working on truffleruby

tompng · 2022-10-10T17:05:03Z

About the failing test build / rake test head (latest reline) and build / rake test head

Result of Ripper::Lexer.new('def req(true) end').scan changed, master branch is also failing

# ruby -v
ruby 3.2.0dev (2022-10-06T10:00:49Z :detached: e696ec67ac) [x86_64-linux]
# ruby -rripper -e "p Ripper::Lexer.new('def req(true) end').scan.map(&:event)"
[:on_kw, :on_sp, :on_ident, :on_lparen, :on_parse_error, :on_kw, :on_rparen, :on_sp, :on_parse_error, :on_kw]

% ruby -v
ruby 3.2.0dev (2022-10-10T09:35:16Z master 1a7e7bb2d1) [x86_64-darwin20]
% ruby -rripper -e "p Ripper::Lexer.new('def req(true) end').scan.map(&:event)"
[:on_kw, :on_sp, :on_ident, :on_lparen, :on_parse_error, :on_kw, :on_parse_error, :on_rparen, :on_sp, :on_parse_error, :on_kw]

st0012 · 2022-10-10T20:46:37Z

lib/irb.rb

        signal_status(:IN_EVAL) do
          begin
            line.untaint if RUBY_VERSION < '2.7'
            if IRB.conf[:MEASURE] && IRB.conf[:MEASURE_CALLBACKS].empty?
              IRB.set_measure_callback
            end
+            is_assignment = assignment_expression?(line)


Why moving this line here though? It doesn't look like to change the behavior?

It actually changes the behavior in a rare case.

irb = IRB::Irb.new context = irb.instance_variable_get('@context') code = "a /'/i if false; a=1; x=1000.times.to_a#'.size" irb.assignment_expression?(code) #=> true context.evaluate(code, 1) #=> [0,1,2,3,...] assignment to local variable x irb.assignment_expression?(code) #=> false context.evaluate(code, 1) #=> 0 not an assignment expression

assignment_expression? check should be done before @context.evaluate (in L519 and L530)

I added test case for this

Wow thanks for spotting this. I think it also worth a comment then?

Thank you for the example 👍

st0012 · 2022-10-10T20:47:43Z

lib/irb.rb

@@ -827,7 +828,9 @@ def assignment_expression?(line)
      # array of parsed expressions. The first element of each expression is the
      # expression's type.
      verbose, $VERBOSE = $VERBOSE, nil
-      result = ASSIGNMENT_NODE_TYPES.include?(Ripper.sexp(line)&.dig(1,-1,0))
+      code = "#{RubyLex.generate_local_variables_assign_code(context: @context) || 'nil;'}\n#{line}"
+      node_type = Ripper.sexp(code)&.dig(1)&.drop(1)&.dig(-1, 0)


This is not super easy to understand. Would you mind adding a comment for it?

st0012 · 2022-10-10T20:48:02Z

lib/irb.rb

-      result = ASSIGNMENT_NODE_TYPES.include?(Ripper.sexp(line)&.dig(1,-1,0))
+      code = "#{RubyLex.generate_local_variables_assign_code(context: @context) || 'nil;'}\n#{line}"
+      node_type = Ripper.sexp(code)&.dig(1)&.drop(1)&.dig(-1, 0)
+      result = ASSIGNMENT_NODE_TYPES.include?(node_type)
      $VERBOSE = verbose


I feel this should be in an ensure block.

st0012 · 2022-10-10T20:56:13Z

lib/irb/ruby-lex.rb

@@ -136,16 +136,20 @@ def set_prompt(p = nil, &block)
    :on_param_error
  ]

+  def self.generate_local_variables_assign_code(context: nil, local_variables: [])


I feel it weird to let generate_local_variables_assign_code take both context and local_variables while:

They're never used at the same time.

Local variables are all retrieved from the same context, just at different timings.

Can we make it self.generate_local_variables_assign_code(local_variables) instead?

And we can add something like Context#local_variables to encapsulate &.workspace&.binding&.local_variables

So that:

We can have less methods taking context while what they actually need are just the local variables

But it's also easier to generate local variables from a context

generate_local_variables_assign_code can have a simpler implementation

self.generate_local_variables_assign_code(local_variables) Context#local_variables

It looks good, I fixed it

…code more simple

lib/irb/input-method.rb

st0012 · 2022-10-11T18:15:05Z

lib/irb.rb

        signal_status(:IN_EVAL) do
          begin
            line.untaint if RUBY_VERSION < '2.7'
            if IRB.conf[:MEASURE] && IRB.conf[:MEASURE_CALLBACKS].empty?
              IRB.set_measure_callback
            end
+            is_assignment = assignment_expression?(line)


Wow thanks for spotting this. I think it also worth a comment then?

st0012 · 2022-10-11T18:31:31Z

lib/irb.rb

        signal_status(:IN_EVAL) do
          begin
            line.untaint if RUBY_VERSION < '2.7'
            if IRB.conf[:MEASURE] && IRB.conf[:MEASURE_CALLBACKS].empty?
              IRB.set_measure_callback
            end
+            is_assignment = assignment_expression?(line)


Thank you for the example 👍

Co-authored-by: Stan Lo <stan001212@gmail.com>

…valuate

st0012 · 2022-10-17T19:53:08Z

@tompng I think the head tests will pass once you rebased master.

st0012 · 2022-10-17T19:54:00Z

@k0kubun would you mind giving it a look as well? thx

k0kubun

Makes sense. Thank you!

ruby/irb#397) * Use local_variables for colorize, code_block_open check, nesting_level and assignment_expression check * Check if expression is an assignment BEFORE evaluating it. evaluate might define new localvars and change result of assignment_expression? * Add local_variables dependent code test * pend local variable dependent test on truffleruby code_block_open is not working on truffleruby * Always pass context to RubyLex#lex * Rename local_variable_assign_code generator method name * Add assignment expression truncate test * Add Context#local_variables and make generate_local_variables_assign_code more simple * Update lib/irb/input-method.rb Co-authored-by: Stan Lo <stan001212@gmail.com> * Add a comment why assignment expression check should be done before evaluate ruby/irb@c8b3877281 Co-authored-by: Stan Lo <stan001212@gmail.com> Co-authored-by: Takashi Kokubun <takashikkbn@gmail.com>

tompng · 2022-10-21T18:53:48Z

🎉
Thanks for reviewing and merging 🙏

* Use local_variables for colorize, code_block_open check, nesting_level and assignment_expression check * Check if expression is an assignment BEFORE evaluating it. evaluate might define new localvars and change result of assignment_expression? * Add local_variables dependent code test * pend local variable dependent test on truffleruby code_block_open is not working on truffleruby * Always pass context to RubyLex#lex * Rename local_variable_assign_code generator method name * Add assignment expression truncate test * Add Context#local_variables and make generate_local_variables_assign_code more simple * Update lib/irb/input-method.rb Co-authored-by: Stan Lo <stan001212@gmail.com> * Add a comment why assignment expression check should be done before evaluate Co-authored-by: Stan Lo <stan001212@gmail.com> Co-authored-by: Takashi Kokubun <takashikkbn@gmail.com>

ruby/irb#397) * Use local_variables for colorize, code_block_open check, nesting_level and assignment_expression check * Check if expression is an assignment BEFORE evaluating it. evaluate might define new localvars and change result of assignment_expression? * Add local_variables dependent code test * pend local variable dependent test on truffleruby code_block_open is not working on truffleruby * Always pass context to RubyLex#lex * Rename local_variable_assign_code generator method name * Add assignment expression truncate test * Add Context#local_variables and make generate_local_variables_assign_code more simple * Update lib/irb/input-method.rb Co-authored-by: Stan Lo <stan001212@gmail.com> * Add a comment why assignment expression check should be done before evaluate ruby/irb@c8b3877281 Co-authored-by: Stan Lo <stan001212@gmail.com> Co-authored-by: Takashi Kokubun <takashikkbn@gmail.com>

tompng marked this pull request as draft September 1, 2022 11:12

tompng force-pushed the lvar_fix branch from 112cfd4 to e317309 Compare September 1, 2022 11:26

tompng marked this pull request as ready for review September 1, 2022 11:33

tompng added 2 commits October 6, 2022 03:26

Use local_variables for colorize, code_block_open check, nesting_leve…

5a4f06e

…l and assignment_expression check

Check if expression is an assignment BEFORE evaluating it. evaluate m…

b823150

…ight define new localvars and change result of assignment_expression?

tompng force-pushed the lvar_fix branch from e317309 to 084849a Compare October 5, 2022 18:30

st0012 reviewed Oct 8, 2022

View reviewed changes

tompng added 4 commits October 9, 2022 17:14

Add local_variables dependent code test

dcd8b62

pend local variable dependent test on truffleruby

5f582aa

code_block_open is not working on truffleruby

Always pass context to RubyLex#lex

da04c25

Rename local_variable_assign_code generator method name

08c5575

tompng force-pushed the lvar_fix branch from 084849a to 08c5575 Compare October 10, 2022 13:46

st0012 reviewed Oct 10, 2022

View reviewed changes

tompng added 2 commits October 11, 2022 19:02

Add assignment expression truncate test

7a064fa

Add Context#local_variables and make generate_local_variables_assign_…

aa38fa0

…code more simple

st0012 reviewed Oct 11, 2022

View reviewed changes

tompng and others added 2 commits October 12, 2022 16:24

Update lib/irb/input-method.rb

840f7b0

Co-authored-by: Stan Lo <stan001212@gmail.com>

Add a comment why assignment expression check should be done before e…

46589d6

…valuate

Merge remote-tracking branch 'origin/master' into lvar_fix

1bd7434

k0kubun approved these changes Oct 18, 2022

View reviewed changes

k0kubun merged commit c8b3877 into ruby:master Oct 18, 2022

tompng deleted the lvar_fix branch October 20, 2022 09:22

st0012 added the bug Something isn't working label Jan 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always use local variables in current context to parse code #397

Always use local variables in current context to parse code #397

tompng commented Aug 31, 2022

st0012 Oct 8, 2022

tompng Oct 10, 2022

tompng Oct 10, 2022

st0012 Oct 8, 2022

st0012 Oct 8, 2022

st0012 Oct 8, 2022

tompng commented Oct 10, 2022

st0012 Oct 10, 2022

tompng Oct 11, 2022

st0012 Oct 11, 2022

st0012 Oct 11, 2022

st0012 Oct 10, 2022

st0012 Oct 10, 2022

st0012 Oct 10, 2022

tompng Oct 11, 2022

st0012 Oct 11, 2022

st0012 Oct 11, 2022

st0012 commented Oct 17, 2022

st0012 commented Oct 17, 2022 •

edited

k0kubun left a comment

tompng commented Oct 21, 2022

Always use local variables in current context to parse code #397

Always use local variables in current context to parse code #397

Conversation

tompng commented Aug 31, 2022

assignment expression check

semicolon and newline character

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tompng commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st0012 commented Oct 17, 2022

st0012 commented Oct 17, 2022 • edited

k0kubun left a comment

Choose a reason for hiding this comment

tompng commented Oct 21, 2022

st0012 commented Oct 17, 2022 •

edited