Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix colorize_code, fix irb crash. #391

Merged
merged 3 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 32 additions & 27 deletions lib/irb/color.rb
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,14 @@ def colorize_code(code, complete: true, ignore_error: false, colorable: colorabl

symbol_state = SymbolState.new
colored = +''
length = 0
end_seen = false

scan(code, allow_last_error: !complete) do |token, str, expr|
# handle uncolorable code
if token.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I commented out this entire condition, the tests still passed.
I think it's because the str will still be passed to Reline::Unicode.escape_for_print at line 145 and then be pushed to colored at line 150. So the end result is still the same.

Therefore, this condition doesn't seem to be needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually works without this change, (I did not notice that) but I think this code is necessary.

I think symbol_state.scan_token(token) and dispatch_seq is originally designed to receive Ripper::Lexer::Elem#event that is not nil.
The current implementation of scan_token and dispatch_seq works just by chance with nil, but I don't think we should pass nil to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the clear explanation 👍

colored << Reline::Unicode.escape_for_print(str)
next
end

# IRB::ColorPrinter skips colorizing fragments with any invalid token
if ignore_error && ERROR_TOKENS.include?(token)
return Reline::Unicode.escape_for_print(code)
Expand All @@ -147,15 +151,7 @@ def colorize_code(code, complete: true, ignore_error: false, colorable: colorabl
colored << line
end
end
length += str.bytesize
end_seen = true if token == :on___end__
end

# give up colorizing incomplete Ripper tokens
unless end_seen or length == code.bytesize
return Reline::Unicode.escape_for_print(code)
end

colored
end

Expand All @@ -170,33 +166,42 @@ def without_circular_ref(obj, seen:, &block)
end

def scan(code, allow_last_error:)
pos = [1, 0]

verbose, $VERBOSE = $VERBOSE, nil
RubyLex.compile_with_errors_suppressed(code) do |inner_code, line_no|
lexer = Ripper::Lexer.new(inner_code, '(ripper)', line_no)
if lexer.respond_to?(:scan) # Ruby 2.7+
lexer.scan.each do |elem|
str = elem.tok
next if allow_last_error and /meets end of file|unexpected end-of-input/ =~ elem.message
next if ([elem.pos[0], elem.pos[1] + str.bytesize] <=> pos) <= 0
byte_pos = 0
line_positions = [0]
inner_code.lines.each do |line|
line_positions << line_positions.last + line.bytesize
end

on_scan = proc do |elem|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's more understandable if we rewrite the block like this?

  on_scan = proc do |elem|
    start_pos = line_positions[elem.pos[0] - 1] + elem.pos[1]

    # yield uncolorable code
    if byte_pos < start_pos
      yield(nil, inner_code.byteslice(byte_pos...start_pos), nil)
    end

    if byte_pos <= start_pos
      str = elem.tok
      yield(elem.event, str, elem.state)
      byte_pos = start_pos + str.bytesize
    end
  end

Reasons for the changes:

  1. It's easier to understand if we always put byte_pos and start_pos at the same position in comparisons.
  2. We can move local variable assignment closer to where it's going to be used.
  3. In complicated logic like this, I feel traditional if block is easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code you suggested looks more readable. thanks

start_pos = line_positions[elem.pos[0] - 1] + elem.pos[1]

str.each_line do |line|
if line.end_with?("\n")
pos[0] += 1
pos[1] = 0
else
pos[1] += line.bytesize
end
end
# yield uncolorable code
if byte_pos < start_pos
yield(nil, inner_code.byteslice(byte_pos...start_pos), nil)
end

if byte_pos <= start_pos
str = elem.tok
yield(elem.event, str, elem.state)
byte_pos = start_pos + str.bytesize
end
end

if lexer.respond_to?(:scan) # Ruby 2.7+
lexer.scan.each do |elem|
next if allow_last_error and /meets end of file|unexpected end-of-input/ =~ elem.message
on_scan.call(elem)
end
else
lexer.parse.each do |elem|
yield(elem.event, elem.tok, elem.state)
lexer.parse.sort_by(&:pos).each do |elem|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests still pass without this change. Why is it needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added test case <<A+1\nA for this.

Another code lexer.parse.reject in ruby-lex.rb should also be fixed to lexer.parse.sort_by(&:pos).reject. This change is in another pull request #390

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this sort_by again and the test still passed 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else branch is only executed when ruby < 2.7 and test failed in my environment ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin19] without sort_by.

[tomoya:irb]% ruby -v
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin19]
[tomoya:irb]% git log -n1
commit da54e7f0813d88546091af5bf220d83a6bd10198 (HEAD -> colorize_every_characters, tompng/colorize_every_characters)
Author: tompng <tomoyapenguin@gmail.com>
Date:   Mon Sep 19 14:14:10 2022 +0900

    Rewrite on_scan proc to be more readable.
[tomoya:irb]% git diff
[tomoya:irb]% rake test
...skip...
..........................
Finished in 6.179684 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
114 tests, 1158 assertions, 0 failures, 0 errors, 6 pendings, 0 omissions, 0 notifications
94.7368% passed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
18.45 tests/s, 187.39 assertions/s
[tomoya:irb]% # delete sort_by(&:pos)
[tomoya:irb]% git diff
diff --git a/lib/irb/color.rb b/lib/irb/color.rb
index 7071696..abe0590 100644
--- a/lib/irb/color.rb
+++ b/lib/irb/color.rb
@@ -196,7 +196,7 @@ module IRB # :nodoc:
               on_scan.call(elem)
             end
           else
-            lexer.parse.sort_by(&:pos).each do |elem|
+            lexer.parse.each do |elem|
               on_scan.call(elem)
             end
           end
[tomoya:irb]% rake test
...skip...
..................F
=========================================================================================================================================================================
Failure: test_colorize_code(TestIRB::TestColor)
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:269:in `assert_equal_with_term'
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:139:in `block in test_colorize_code'
     136: 
     137:       tests.each do |code, result|
     138:         if colorize_code_supported?
  => 139:           assert_equal_with_term(result, code, complete: true)
     140:           assert_equal_with_term(result, code, complete: false)
     141: 
     142:           assert_equal_with_term(code, code, complete: true, tty: false)
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:137:in `each'
/Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:137:in `test_colorize_code'
Case: colorize_code("<<A+1\nA", complete: true)
Result: "#{RED}<<A#{CLEAR}+1\n#{RED}A#{CLEAR}"
<"\e[31m<<A\e[0m+\e[34m\e[1m1\e[0m\n" + "\e[31mA\e[0m"> expected but was
<"\e[31m<<A\e[0m+1\n" + "\e[31mA\e[0m">

diff:
? <<A+1m1
  A
=========================================================================================================================================================================
...skip...
..........................
Finished in 4.4035 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
114 tests, 1103 assertions, 1 failures, 0 errors, 6 pendings, 0 omissions, 0 notifications
93.8596% passed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
25.89 tests/s, 250.48 assertions/s
rake aborted!
Command failed with status (1)
/Users/tomoya/.rbenv/versions/2.6.0/bin/bundle:23:in `load'
/Users/tomoya/.rbenv/versions/2.6.0/bin/bundle:23:in `<main>'
Tasks: TOP => test
(See full trace by running task with --trace)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry I missed that 🤦‍♂️

on_scan.call(elem)
end
end
# yield uncolorable DATA section
yield(nil, inner_code.byteslice(byte_pos...inner_code.bytesize), nil) if byte_pos < inner_code.bytesize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment to explain what cases this handles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comment yield uncolorable DATA section

end
ensure
$VERBOSE = verbose
Expand Down
16 changes: 13 additions & 3 deletions test/irb/test_color.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ def test_colorize_code
"foo(*%W(bar))" => "foo(*#{RED}#{BOLD}%W(#{CLEAR}#{RED}bar#{CLEAR}#{RED}#{BOLD})#{CLEAR})",
"$stdout" => "#{GREEN}#{BOLD}$stdout#{CLEAR}",
"__END__" => "#{GREEN}__END__#{CLEAR}",
"foo\n__END__\nbar" => "foo\n#{GREEN}__END__#{CLEAR}\nbar",
"foo\n<<A\0\0bar\nA\nbaz" => "foo\n#{RED}<<A#{CLEAR}^@^@bar\n#{RED}A#{CLEAR}\nbaz",
"<<A+1\nA" => "#{RED}<<A#{CLEAR}+#{BLUE}#{BOLD}1#{CLEAR}\n#{RED}A#{CLEAR}",
}

# specific to Ruby 2.7+
Expand All @@ -112,11 +115,18 @@ def test_colorize_code
"def req(@a) end" => "#{GREEN}def#{CLEAR} #{BLUE}#{BOLD}req#{CLEAR}(#{RED}#{REVERSE}@a#{CLEAR}) #{GREEN}end#{CLEAR}",
})
else
tests.merge!({
"[1]]]\u0013" => "[1]]]^S",
if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0')
tests.merge!({
"[1]]]\u0013" => "[#{BLUE}#{BOLD}1#{CLEAR}]#{RED}#{REVERSE}]#{CLEAR}]^S",
"def req(true) end" => "#{GREEN}def#{CLEAR} #{BLUE}#{BOLD}req#{CLEAR}(#{RED}#{REVERSE}true#{CLEAR}) end",
})
else
tests.merge!({
"[1]]]\u0013" => "[#{BLUE}#{BOLD}1#{CLEAR}]]]^S",
"def req(true) end" => "#{GREEN}def#{CLEAR} #{BLUE}#{BOLD}req#{CLEAR}(#{CYAN}#{BOLD}true#{CLEAR}) end",
})
end
tests.merge!({
"def req(true) end" => "def req(true) end",
"nil = 1" => "#{CYAN}#{BOLD}nil#{CLEAR} = #{BLUE}#{BOLD}1#{CLEAR}",
"alias $x $1" => "#{GREEN}alias#{CLEAR} #{GREEN}#{BOLD}$x#{CLEAR} $1",
"class bad; end" => "#{GREEN}class#{CLEAR} bad; #{GREEN}end#{CLEAR}",
Expand Down