Skip to content

Ruby 2.7 backport about IRB #2990

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

Merged
merged 94 commits into from
Mar 30, 2020
Merged

Conversation

aycabta
Copy link
Member

@aycabta aycabta commented Mar 28, 2020

No description provided.

aycabta and others added 30 commits March 29, 2020 04:25
TestRelineAsReadline#test_input_metachar passes on MinGW
The set_auto_indent method calculates the correct number of spaces for
indenting a line. We think there might be a few bugs in this method so
we are testing the current functionality to make sure nothing breaks
when we address those bugs.

Example test failure:

```
  1) Failure:
TestIRB::TestRubyLex#test_auto_indent [/Users/Ben/Projects/irb/test/irb/test_ruby_lex.rb:75]:
Calculated the wrong number of spaces for:
 def each_top_level_statement
  initialize_input
  catch(:TERM_INPUT) do
    loop do
      begin
        prompt
        unless l = lex
          throw :TERM_INPUT if @line == ''
        else
.
<10> expected but was
<12>.
```

ruby/irb@752d5597ab
#### Legacy mode:

  ```console
  $ irb --legacy
  irb(main):001:0> l[TAB][TAB]
  lambda           load             local_variables  loop
  ```

#### Before this patch:

  ```console
  $ irb
  irb(main):001:0> l[TAB][TAB]
  local_variables
  loop
  lambda
  load
  ```

#### After this patch:

  ```console
  $ irb
  irb(main):001:0> l[TAB][TAB]
  lambda
  load
  local_variables
  loop
  ```

ruby/reline@6074069c7d
If C-d is pressed before IRB is ready, IRB crashes because EOFError occurs.
  def foo
    ''.upca[TAB]

This will be completed to be:

  def foo
  ''.upcase

The indent was gone. This commit fixes the bug.
This commit fixes the check_newline_depth_difference method to multiple
open braces on one line into account. Before this change we were
subtracting from the depth in check_newline_depth_difference on
every open brace. This is the right thing to do if the opening and
closing brace are on the same line. For example in a method definition we
have an opening and closing parentheses we want to add 1 to our depth,
and then remove it.

```
def foo()
end
```

However this isn't the correct behavior when the brace spans multiple
lines. If a brace spans multiple lines we don't want to subtract from
check_newline_depth_difference and we want to treat the braces the same
way as we do `end` and allow check_corresponding_token_depth to pop the
correct depth.

Example of bad behavior:

```
def foo()
  [
  ]
puts 'bar'
end
```

Example of desired behavior:

```
def foo()
  [
  ]
  puts 'bar'
end
```

ruby/irb@7dc8af01e0
ruby/irb#55

If we had put multiple open braces on a line the with no closing brace
spaces_of_nest array keeps getting '0' added to it. This means that when
we pop off of this array we are saying that we should be in position zero
for the next line. This is an issue because we don't always want to be
in position 0 after a closing brace.

Example:
```
[[[
]
]
]
```
In the above example the 'spaces_of_nest' array looks like this after
the first line is entered: [0,0,0]. We really want to be indented 4
spaces for the 1st closing brace 2 for the 2nd and 0 for the 3rd. i.e.
we want it to be: [0,2,4].

We also saw this issue with a heredoc inside of an array.

```
[<<FOO]
hello
FOO
```

ruby/irb@80c69c8272
The command prompt on Windows always uses Unicode to take input and print
output but most Reline implementation depends on Encoding.default_external.
This commit introduces an abstracted structure about the encoding of Reline.
Rails before 5.2 added Array#append as an alias to Array#<< ,
so that it expects only one argument.
However ruby-2.5 added Array#append as an alias to Array#push
which takes any number of arguments.

If irb completion is used in `rails c` (for example "IO.<tab>")
it fails with:
  irb/completion.rb:206:in `<<': wrong number of arguments (given 3, expected 1) (ArgumentError)

Using Array#push instead of Array#append fixes compatibility.

ruby/irb@5b7bbf9c34
The previous detection per get_screen_size fails when stdout is passed
to a pipe. That is the case when running ruby tests in parallel ("-j" switch).
In this case Reline believes that it's running on MinTTY and the tests
are running with ANSI IOGate instead of the Windows adapter on MINGW.
So parallel test results were different to that of a single process.
This commit fixes these differencies.

The code is taken from git sources and translated to ruby.
NtQueryObject() is replaced by GetFileInformationByHandleEx(), because
NtQueryObject() is undocumented and is more difficult to use:
  https://github.com/git-for-windows/git/blob/c5a03b1e29c69f3f06c8fabd92493edb73469176/compat/winansi.c#L558
In commit f8ea286 the Reline encoding
for native windows console was changed to hardcoded UTF-8.
This caused failures in reline and readline tests, but they were hidden,
because parallel ruby tests incorrectly used Reline::ANSI as IOGate.
Tests failures were raised in single process mode, but not with -j switch.

This patch corrects encodings on native Windows console.
aycabta and others added 24 commits March 29, 2020 04:25
GNU Readline add a white space when Readline.completion_append_character is
not initialized.
Some other tests can set Reline.completion_proc, so if it is nil,
simply skip this test.
TestRelineAsReadline#test_insert_text expects Readline.point == 0
at the beginning of the test, but a test violate this assumption.
https://github.com/ruby/actions/runs/500526558?check_suite_focus=true#step:16:127
```
Failures:

  1) Bundler.setup when Bundler is bundled doesn't blow up
     Failure/Error: expect(err).to be_empty

       expected `"fatal: not a git repository (or any of the parent directories): .git\nfatal: not a git repository (o...the parent directories): .git\nfatal: not a git repository (or any of the parent directories): .git".empty?` to return true, got false

       Commands:
       $ /home/runner/work/actions/actions/snapshot-master/ruby \
         -I/home/runner/work/actions/actions/snapshot-master/lib:/home/runner/work/actions/actions/snapshot-master/spec/bundler \
         -rsupport/hax -rsupport/artifice/fail \
         /home/runner/work/actions/actions/snapshot-master/libexec/bundle install --retry 0
       Resolving dependencies...
       Using bundler 2.1.4
       Bundle complete! 1 Gemfile dependency, 1 gem now installed.
       Use `bundle info [gemname]` to see where a bundled gem is installed.
       fatal: not a git repository (or any of the parent directories): .git
       fatal: not a git repository (or any of the parent directories): .git
       fatal: not a git repository (or any of the parent directories): .git
       # $? => 0

       $ /home/runner/work/actions/actions/snapshot-master/ruby \
         -I/home/runner/work/actions/actions/snapshot-master/lib:/home/runner/work/actions/actions/snapshot-master/spec/bundler \
         -rsupport/hax -rsupport/artifice/fail \
         /home/runner/work/actions/actions/snapshot-master/libexec/bundle exec ruby -e \
         require\ \'bundler\'\;\ Bundler.setup
       fatal: not a git repository (or any of the parent directories): .git
       fatal: not a git repository (or any of the parent directories): .git
       fatal: not a git repository (or any of the parent directories): .git
       # $? => 0
     # ./spec/bundler/runtime/setup_spec.rb:1056:in `block (3 levels) in <top (required)>'
     # ./spec/bundler/spec_helper.rb:111:in `block (3 levels) in <top (required)>'
     # ./spec/bundler/spec_helper.rb:111:in `block (2 levels) in <top (required)>'
     # ./spec/bundler/spec_helper.rb:78:in `block (2 levels) in <top (required)>'
make: *** [yes-test-bundler] Error 1
```
The old implementation performance test code:

    require 'objspace'
    puts "%.5g MB" % (ObjectSpace.memsize_of_all * 0.001 * 0.001)
    /\A.*\Z/ !~ ('abc' * 20_000_000)
    puts "%.5g MB" % (ObjectSpace.memsize_of_all * 0.001 * 0.001)

and run `time test.rb`:

    2.5868 MB
    62.226 MB

    real    0m1.307s
    user    0m0.452s
    sys     0m0.797s

The new implementation performance test code:

    require 'objspace'
    puts "%.5g MB" % (ObjectSpace.memsize_of_all * 0.001 * 0.001)
    ('abc' * 20_000_000).include?("\n")
    puts "%.5g MB" % (ObjectSpace.memsize_of_all * 0.001 * 0.001)

and run `time test.rb`:

    2.5861 MB
    62.226 MB

    real    0m0.132s
    user    0m0.088s
    sys     0m0.042s

ruby/irb@40d6610baf
This breaks tests using this path on JRuby because the `jruby`
executable turns into `jjruby` after the sub.

ruby/io-console@e5951aa34c
@aycabta aycabta force-pushed the ruby_2_7-irb-backport branch from 658a86c to 76ea375 Compare March 30, 2020 07:58
@aycabta aycabta force-pushed the ruby_2_7-irb-backport branch from 76ea375 to dda8a39 Compare March 30, 2020 08:09
@nurse nurse merged commit 0057fe4 into ruby:ruby_2_7 Mar 30, 2020
@aycabta aycabta deleted the ruby_2_7-irb-backport branch March 30, 2020 11:33
@hsbt hsbt added the Backport label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.