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

Improve indentation: bugfix, heredoc, embdoc, strings #515

Merged
merged 4 commits into from
Jun 20, 2023

Conversation

tompng
Copy link
Member

@tompng tompng commented Feb 5, 2023

Description

  • Implement heredoc and embdoc indent
  • Implement string indent
  • Fixes all indentation bug that I know now

left: irb 1.6.2 right: this branch

perfect_indent.mp4

Heredoc and embdoc indent

I implemented these heredoc and embdoc indent

if 1
  x = <<EOS
heredoc # initial indent should be 0
EOS
  puts x
=begin
embdoc # initial indent should be 0
=end
  y = <<~EOS1 + <<~EOS2
    heredoc1 # initial indent should be 4
    # indent of heredoc_end should be 2
  EOS1
    heredoc2
  EOS2
end
<<~JS
  function f() {
    const x = 1 // any number of indent(>=2) should be accepted
    console.log(x) // next-line's initial indent should be same as prev line
  }
JS

String indent

string, backtick, regexp and symbol

def send_response
  # Inside string literal, any number of indent(>=0) should be accepted.
  # In master branch, two spaces will automatically inserted and make http header invalid.
  http_header = "HTTP/1.1 200 OK
Content-Type: text/html
Content-Length: #{body.bytesize}"
  socket.write "#{http_header}\n\n#{body}"
end

Indentation bugfix

These bugs will be fixed

> if 1
>   def f() = (█1 * 2)
>   1
# ↓ insert line-break by pressing option+enter
> if 1
>     def f() = (
>     █1 * 2)
>   1
> if 1
>   a = 1
>   1
# ↓ insert line-break by pressing option+enter
> if 1
> a = 1
> 
>   1
if 1
  beginning
  en█ing
# ↓ type d
if 1
  beginning
end█ing

Internal

auto_indent_proc ->(lines, line_index, byte_pointer, is_newline){} was implemented as:

if is_newline
  # next line's indent number
else
  # current line's indent if indent number is decreasing
  # nil (means any number of indent is accepted) if indent number is not decreasing (this will cause a bug)
  # note that it ignores right side of the cursor in the line which also cause a bug
end

But it is wrong. In most of the cases, it should return a number to avoid bug when inserting newline.

if is_newline
  # next line's indent number
else
  # nil (means any number of indent is accepted) when deleting or adding indent spaces
  # checked by left side of the cursor is all space or not
  # otherwise, returns current line's indent
end

Also, special treatment is done for heredoc, embdoc and free-indent-acceptable literal like string.

@tompng tompng force-pushed the rewrite_rubylex_plus_indent_fix branch from 8012d9a to 32a68b4 Compare March 24, 2023 14:46
@tompng tompng force-pushed the rewrite_rubylex_plus_indent_fix branch 3 times, most recently from c6ca53b to 177395f Compare May 26, 2023 15:43
@tompng tompng force-pushed the rewrite_rubylex_plus_indent_fix branch from 177395f to 7c4719b Compare June 9, 2023 16:58
@@ -80,7 +80,7 @@ def b; true; end
irb(main):008:0>
irb(main):009:0> a
irb(main):010:0> .a
irb(main):011:0> .b
Copy link
Member Author

@tompng tompng Jun 9, 2023

Choose a reason for hiding this comment

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

This indent retain feature dropped in this pull request had a bug.

Paste this code and press return key

a
  .b
  .c
if d

then you will get wrong indent

irb(main):001:0> a
irb(main):002:0>   .b
irb(main):003:0>   .c
irb(main):004:1*   if d
irb(main):005:1*     

@tompng tompng force-pushed the rewrite_rubylex_plus_indent_fix branch 2 times, most recently from 733070d to 540b64c Compare June 15, 2023 15:43
@tompng tompng marked this pull request as ready for review June 15, 2023 15:45
@tompng tompng mentioned this pull request Jun 15, 2023
@tompng tompng force-pushed the rewrite_rubylex_plus_indent_fix branch from 540b64c to c382a02 Compare June 19, 2023 10:56
next nil if !is_newline && lines[line_index]&.byteslice(0, byte_pointer)&.match?(/\A\s*\z/)

code = lines[0..line_index].map { |l| "#{l}\n" }.join
next nil if code == "\n"
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean lines is either [nil] or [""] when the condition is true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, it looks like almost-unreachable code.

I think I added this to returns nil if lines are empty, but empty lines are not passed to this proc

Copy link
Member Author

Choose a reason for hiding this comment

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

When I type CTRL+d, [nil] is passed. It might be a bug of reline.
I added again with more simple condition and comment.

next nil if lines == [nil] # Workaround for exit IRB with CTRL+d

lib/irb/ruby-lex.rb Outdated Show resolved Hide resolved
lib/irb/ruby-lex.rb Show resolved Hide resolved
@tompng tompng force-pushed the rewrite_rubylex_plus_indent_fix branch 2 times, most recently from 2a4519a to 1e493c9 Compare June 19, 2023 19:19
lib/irb/ruby-lex.rb Outdated Show resolved Hide resolved
lib/irb/ruby-lex.rb Outdated Show resolved Hide resolved
lib/irb/ruby-lex.rb Show resolved Hide resolved
test/irb/test_ruby_lex.rb Outdated Show resolved Hide resolved
end
elsif prev_opens.last&.event == :on_heredoc_beg
tok = prev_opens.last.tok
if prev_opens.size < next_opens.size || prev_opens.last == next_opens.last
Copy link
Member Author

@tompng tompng Jun 20, 2023

Choose a reason for hiding this comment

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

I changed this condition simpler to if prev_opens.size <= next_opens.size

When heredoc is closed, next_opens.size is always smaller than prev_opens.size because only heredoc_end can be on the line.

<<A # heredoc_beg
A + ( # heredoc_end + lparen ← this is not possible
A
 heredoc ends here

@tompng tompng force-pushed the rewrite_rubylex_plus_indent_fix branch from 1e493c9 to fd8feba Compare June 20, 2023 13:46
@st0012 st0012 added the bug Something isn't working label Jun 20, 2023
@st0012 st0012 merged commit 1159c13 into ruby:master Jun 20, 2023
24 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 20, 2023
(ruby/irb#515)

* Implement heredoc embdoc and string indentation with bugfix

* Fix test_ruby_lex's indentation value

* Add embdoc indent test

* Add workaround for lines==[nil] passed to auto_indent when exit IRB with CTRL+d
@tompng tompng deleted the rewrite_rubylex_plus_indent_fix branch June 20, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

None yet

2 participants