Rewrite RubyLex to fix some bugs and make it possible to add new features easily

[tompng]

Description

Fixes #499
In RubyLex, there are several parser-like methods listed below. To fix bugs, almost all of these methods needs change.

• process_nesting_level
• check_corresponding_token_depth
• check_newline_depth_difference
• check_string_literal
• is_the_in_correspond_to_a_for
• take_corresponding_syntax_to_kw_do
• in_keyword_case_scope?
• is_method_calling?
• heredoc_scope?

I combined them all into a single parser that calculates open tokens for each line.
Duplicated codes are reduced.

What I changed

Rewrite nesting parser

Create IRB::NestingParser and use it from several methods.

# Example of open tokens for each line
if true # open tokens: ['if']
  puts( # open tokens: ['if', '(']
    1   # open tokens: ['if', '(']
  )     # open tokens: ['if']
end     # open tokens: []

each_line_result = RubyLex::NestingParser.parse_line(tokens)
line_tokens, open_tokens_before_line, open_tokens_after_line, minimum_token_depth_in_line = each_line_result[line_index]

# Example
puts((
    1 + 2
  ) + [ # HERE
    1
  ].size
)
# line_tokens: ['  ', ')', ' ', '+', ' ', '[', ' ', "# HERE\n"]
# open tokens changes: '((' → '(' (minimum) → '(['
# open_tokens_before_line: ['(', '(']
# open_tokens_after_line: ['(', '[']
# minimum_token_depth_in_line: 1

Prompt(check_corresponding_token_depth, check_string_literal), indent(process_indent_level, check_newline_depth_difference) and termination can be calculated from open tokens.

Update test

• Add test for NestingParser
• Delete some test which will be tested in newly added test
• Fix test which was testing broken feature

Refactor

I've done a minimal refactoring in ruby-lex.rb

• Reduce instance variables, Split readmultiline from each_top_level_statement (moved to Simplify each_top_level_statement #576)
• Refactor free indentation inside heredoc feature because it needs to be implemented in both process_indent_level and check_corresponding_token_depth

Bugs that cannot be fixed in this pull-request

Cannot indent these kind of code correctly. (endless def inside while condition)

if false
  while def f() = p do end and ()[] do end
end
if false
  while def f() = p do end && ()[] do end
  end
end

The only difference of Ripper.lex result is [[2, 27], :on_kw, "and", BEG] and [[2, 27], :on_op, "&&", BEG].

Other good things for the future

Testability

We can now test these functionalities separately.

• calculate nesting for many ruby syntax
• open tokens to indent conversion
• open tokens to prompt conversion
• actual indentation

Indent

We can now implement heredoc indent like the code below.
Previously, we cannot implement it because indent was using indent += 1 and indent -= 1.

if true
  if true
    s = <<HEREDOC
#{ # nesting level gets deeper but indent gets shallower
  (
    1
  )
}
heredoc
HEREDOC
    puts s # restores indent
  end
end

Parsing logic and indent calculation logic is separated, so we can now easily update indent of specific syntax.

# easy to implement this indent
words = %w[
  irb
  reline
]

Prompt

Prompt string is calculated from ltype, indent, continue and line_no.
We can now easily change it.

> string = "hello #{[
>   1, # ltype is currently `"` here because it is restricted to string-like literal
>   2, # changing ltype to `]` might be better
> ].join} world"

Completion

Currently, completion is implemented using regexp. array[index].??? shows Array methods and array.map{}.??? shows Hash | Proc methods.
We can get an S-expression or a syntax tree of incomplete code and use it for accurate completion.

closing_tokens_calculated_from_open_tokens = open_tokens.map { |t| closing_token_from(t) }
Ripper.sexp(incomplete_code + closing_tokens_calculated_from_open_tokens.join)

[tompng]

changed to draft because it got infinite loop in ruby2.7. trying to fixing it now

[st0012]

Thank you so much for the rewrite 🙏 I believe it’ll be a tremendous improvement to IRB.
But given the size of the change, we may need to go through this slowly with multiple reviews. I hope that’ll be ok.

I’ve given it a couple of scans and I have a question: Should we make context an instance variable?

From what I see, RubyLex can be used as an instance (e.g. scanner.set_input) or as a helper class (e.g. RubyLex.generate_local_variables_assign_code). And currently both usages would take context as an argument.

But for the former usages, the invocations could be largely simplified if context could be stored as an instance variable. So I did a quick search on RubyLex.new and I think it should be possible to do:

• In ShowSource it could access the context through irb_context (provided by Nop)
• In Irb.initialize it has context defined just a couple lines prior.
• In RubyLex.set_input it also has access to context through itself. I wonder why we need to initialise another RubyLex instance here though.

If the answer is yes, I will to do that refactor before merging this PR, which is likely to cause some conflicts. So this is why I’m asking here 🙂

[tompng]

@st0012

Should we make context an instance variable?

Thanks for the explanation. I think it's a very good idea too. My answer is yes.

likely to cause some conflicts

I'll resolve it when it's needed 😄

[st0012]

I ended up doing 3 types of refactor:

• Remove redundant argument defaults from some RubyLex methods #502
• Store context in RubyLex #503
• Drop unused arguments in RubyLex #504

I hope once they're all merged, it'll make the rewrite even simpler 🙂

[st0012]

@tompng All 3 refactor PRs have been merged 😄

[tompng]

Thanks, I rebased it

[st0012]

I like that we're getting more detailed coverage with this rewrite and I think some small tweaks can make the new tests even more approachable 👍

[st0012]

This is an amazing work, I think we're pretty close to merging it 👍

[st0012]

Just curious: why do we pick class as the target for checking Ruby syntax? Is the assumption: "if the class is not accidentally closed by any of the complicated syntax inside it, then we assume the nesting parser is working correctly"?

[tompng]

Yes. the intention of the test is to ensure "class is not accidentally closed".
We can also use if true or other nesting syntax instead of class A

[st0012]

I think class A is fine. But perhaps in later PRs we can try to match the inner lines' tokens too to increase the coverage.

[st0012]

Let's merge it 🚀