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

CRuby test failures #2935

Closed
kddnewton opened this issue Jul 16, 2024 · 1 comment
Closed

CRuby test failures #2935

kddnewton opened this issue Jul 16, 2024 · 1 comment
Labels
bug Something isn't working compiler

Comments

@kddnewton
Copy link
Collaborator

kddnewton commented Jul 16, 2024

When fully switching everything over to Prism, there were some additional test failures because the Ruby process was manually invoked in the test suite and it wasn't respecting --parser=prism. We need to fix these test failures to get the parser merged. They are categorized below.

  • Missing some method call argument optimizations

We need to implement all of the same recent optimizations to setup_args that were added in 3.4. This largely involves avoiding array and hash allocations. This is done through checking for specific patterns and using a combination of flags, instructions like pushtoarray and concattoarray, and generally avoiding any unnecessary arrays.

TestAllocation::MethodCall#test_anonymous_splat_and_anonymous_keyword_splat_parameters
TestAllocation::MethodCall#test_argument_forwarding
TestAllocation::MethodCall#test_keyword_and_keyword_splat_parameter
TestAllocation::MethodCall#test_keyword_parameter
TestAllocation::MethodCall#test_keyword_splat_parameter
TestAllocation::MethodCall#test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters
TestAllocation::MethodCall#test_nested_argument_forwarding
TestAllocation::MethodCall#test_no_array_allocation_with_splat_and_nonstatic_keywords
TestAllocation::MethodCall#test_no_parameters
TestAllocation::MethodCall#test_optional_parameter
TestAllocation::MethodCall#test_positional_splat_and_keyword_parameter
TestAllocation::MethodCall#test_positional_splat_and_keyword_splat_parameter
TestAllocation::MethodCall#test_required_and_keyword_splat_parameter
TestAllocation::MethodCall#test_required_parameter
TestAllocation::MethodCall#test_required_positional_and_keyword_parameter
TestAllocation::MethodCall#test_ruby2_keywords
  • Syntax error message format does not match

In some cases, the error message generated by Prism does not match the error message generated by the current parser. We need to match this so that the test assertion does not have to change.

TestException#test_syntax_error_detailed_message
TestParse#test_unclosed_unicode_escape_at_eol_bug_19750
TestRegexp#test_extended_comment_invalid_escape_bug_18294
TestRegexp#test_nonextended_section_of_extended_regexp_bug_19379
  • Unused block warning was not implemented for Prism

We need to implement the unused block warning for Prism, since it was only implemented for compile.c. This will involve implementing the same changes to prism_compile.c as in this commit: ruby/ruby@e9d7478dedb.

TestMethod#test_warn_unused_block
  • Not processing switches from shebang

Some files have shebangs with additional switches after them, like #!ruby -s -, which could potentially set additional global variables from the command line. We now use a callback to process those, but are missing something to do with encoding to make this test pass.

TestRubyOptions#test_shebang
  • Not handling FIFO objects

We likely need to match the behavior from this commit: ruby/ruby@bc8687acd62. Essentially since we're going through a different codepath than CRuby for reading source files, we need to make sure we have the same behavior. This has something to do with file systems that is outside my knowledge area, so extensive testing will be required.

TestRequire#test_loading_fifo_fd_leak
TestRequire#test_loading_fifo_threading_raise
TestRequire#test_loading_fifo_threading_success
  • Unknown

I'm not sure why these tests are failing, and they need additional investigation.

TestCoverage#test_coverage_in_main_script
TestIO#test_set_lineno
TestTRICK2015#test_ksk_1

Note that to get these failures, you need to have Prism as the default parser, which involves using the following diff:

diff --git a/ruby.c b/ruby.c
index 3c78ada936..d9b7824a98 100644
--- a/ruby.c
+++ b/ruby.c
@@ -1428,10 +1428,10 @@ proc_long_options(ruby_cmdline_options_t *opt, const char *s, long argc, char **
     }
     else if (is_option_with_arg("parser", Qfalse, Qtrue)) {
         if (strcmp("prism", s) == 0) {
-            (*rb_ruby_prism_ptr()) = true;
+            // default behavior
         }
         else if (strcmp("parse.y", s) == 0) {
-            // default behavior
+            (*rb_ruby_prism_ptr()) = false;
         }
         else {
             rb_raise(rb_eRuntimeError, "unknown parser %s", s);
@@ -3077,8 +3077,6 @@ ruby_process_options(int argc, char **argv)
     VALUE iseq;
     const char *script_name = (argc > 0 && argv[0]) ? argv[0] : ruby_engine;
 
-    (*rb_ruby_prism_ptr()) = false;
-
     if (!origarg.argv || origarg.argc <= 0) {
         origarg.argc = argc;
         origarg.argv = argv;
diff --git a/vm.c b/vm.c
index c7ccbc9550..8a66428eeb 100644
--- a/vm.c
+++ b/vm.c
@@ -4461,7 +4461,7 @@ rb_ruby_verbose_ptr(void)
     return &cr->verbose;
 }
 
-static bool prism;
+static bool prism = true;
 
 bool *
 rb_ruby_prism_ptr(void)
@kddnewton kddnewton added bug Something isn't working compiler labels Jul 16, 2024
@kddnewton kddnewton added this to the CRuby unblocked milestone Jul 16, 2024
@kddnewton kddnewton pinned this issue Jul 16, 2024
eileencodes added a commit to Shopify/ruby that referenced this issue Jul 17, 2024
When we have an empty hash the iseq should have a `newhash` but instead
had a `duphash`. To fix, return `Qnil` if the `hash` elements are empty,
use `newhash` in that case, otherwise `duphash`.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 duphash                                {}                        (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 newhash                                0                         (   1)[Li]
0002 leave
```

Fixes the test `TestYJIT#test_compile_newhash`. Related to ruby/prism#2935
eileencodes added a commit to eileencodes/ruby that referenced this issue Jul 17, 2024
If the symbol node is interpolated like this `:"#{foo}"` the instruction
sequence should be `putstring` followed by `intern`. In this case it was
a `putobject` causing the `test_yjit` tests to fail. Note that yjit is
not required to reproduce - the instructions are `putstring` and
`intern` for yjit and non-yjit with the original parser.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putobject                              :foo                      (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putstring                              "foo"                     (   1)[Li]
0002 intern
0003 leave
```

Fixes the test `TestYJIT#test_compile_dynamic_symbol`. Related to ruby/prism#2935
eileencodes added a commit to eileencodes/ruby that referenced this issue Jul 17, 2024
When we have an empty hash the iseq should have a `newhash` but instead
had a `duphash`. To fix, check if the node's elements are equal to `0`.
If so we want a `newhash`, otherwise use the original `duphash`
instructions.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 duphash                                {}                        (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 newhash                                0                         (   1)[Li]
0002 leave
```

Fixes the test `TestYJIT#test_compile_newhash`. Related to ruby/prism#2935
eileencodes added a commit to eileencodes/ruby that referenced this issue Jul 17, 2024
When we have an empty hash the iseq should have a `newhash` but instead
had a `duphash`. To fix, check if the node's elements are equal to `0`.
If so we want a `newhash`, otherwise use the original `duphash`
instructions.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 duphash                                {}                        (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 newhash                                0                         (   1)[Li]
0002 leave
```

Fixes the test `TestYJIT#test_compile_newhash`. Related to ruby/prism#2935
kddnewton pushed a commit to ruby/ruby that referenced this issue Jul 18, 2024
When we have an empty hash the iseq should have a `newhash` but instead
had a `duphash`. To fix, check if the node's elements are equal to `0`.
If so we want a `newhash`, otherwise use the original `duphash`
instructions.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 duphash                                {}                        (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,2)>
0000 newhash                                0                         (   1)[Li]
0002 leave
```

Fixes the test `TestYJIT#test_compile_newhash`. Related to ruby/prism#2935
eileencodes added a commit to eileencodes/ruby that referenced this issue Jul 18, 2024
If the symbol node is interpolated like this `:"#{foo}"` the instruction
sequence should be `putstring` followed by `intern`. In this case it was
a `putobject` causing the `test_yjit` tests to fail. Note that yjit is
not required to reproduce - the instructions are `putstring` and
`intern` for yjit and non-yjit with the original parser.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putobject                              :foo                      (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putstring                              "foo"                     (   1)[Li]
0002 intern
0003 leave
```

Fixes the test `TestYJIT#test_compile_dynamic_symbol`. Related to ruby/prism#2935
eileencodes added a commit to eileencodes/ruby that referenced this issue Jul 18, 2024
If the symbol node is interpolated like this `:"#{foo}"` the instruction
sequence should be `putstring` followed by `intern`. In this case it was
a `putobject` causing the `test_yjit` tests to fail. Note that yjit is
not required to reproduce - the instructions are `putstring` and
`intern` for yjit and non-yjit with the original parser.

To fix I moved `pm_interpolated_node_compile` out of the else, and
entirely removed the conditional. `pm_interpolated_node_compile` knows
how / when to use `putstring` over `putobject` already. The `intern` is
then added by removing the conditional.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putobject                              :foo                      (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putstring                              "foo"                     (   1)[Li]
0002 intern
0003 leave
```

Fixes the test `TestYJIT#test_compile_dynamic_symbol`. Related to ruby/prism#2935
kddnewton pushed a commit to ruby/ruby that referenced this issue Jul 19, 2024
If the symbol node is interpolated like this `:"#{foo}"` the instruction
sequence should be `putstring` followed by `intern`. In this case it was
a `putobject` causing the `test_yjit` tests to fail. Note that yjit is
not required to reproduce - the instructions are `putstring` and
`intern` for yjit and non-yjit with the original parser.

To fix I moved `pm_interpolated_node_compile` out of the else, and
entirely removed the conditional. `pm_interpolated_node_compile` knows
how / when to use `putstring` over `putobject` already. The `intern` is
then added by removing the conditional.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putobject                              :foo                      (   1)[Li]
0002 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,11)>
0000 putstring                              "foo"                     (   1)[Li]
0002 intern
0003 leave
```

Fixes the test `TestYJIT#test_compile_dynamic_symbol`. Related to ruby/prism#2935
eileencodes added a commit to eileencodes/ruby that referenced this issue Jul 23, 2024
In the following code:

```ruby
def foo(&blk)
  blk.call
end
```

Prism was using the `getblockparam` instruction but it should be
`getblockparamproxy`. In this case we have a receiver, if it's a local
variable read node, then it needs to go through the path to use
`getblockparamproxy`.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(3,3)>
0000 definemethod                           :foo, foo                 (   1)[Li]
0003 putobject                              :foo
0005 leave

== disasm: #<ISeq:foo@test2.rb:1 (1,0)-(3,3)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: 0, kw: -1@-1, kwrest: -1])
[ 1] blk@0<Block>
0000 getblockparam                          blk@0, 0                  (   2)[LiCa]
0003 opt_send_without_block                 <calldata!mid:call, argc:0, ARGS_SIMPLE>
0005 leave                                                            (   3)[Re]
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(3,3)>
0000 definemethod                           :foo, foo                 (   1)[Li]
0003 putobject                              :foo
0005 leave

== disasm: #<ISeq:foo@test2.rb:1 (1,0)-(3,3)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: 0, kw: -1@-1, kwrest: -1])
[ 1] blk@0<Block>
0000 getblockparamproxy                     blk@0, 0                  (   2)[LiCa]
0003 opt_send_without_block                 <calldata!mid:call, argc:0, ARGS_SIMPLE>
0005 leave                                                            (   3)[Re]
```

Fixes `test_getblockparamproxy` and `test_ifunc_getblockparamproxy` in
test_yjit.rb. Related to ruby/prism#2935.
eileencodes added a commit to eileencodes/ruby that referenced this issue Jul 23, 2024
This addresses one of the issues in the `test_kw_splat_nil` failure, but
doesn't make the test pass because of other changes that need to be made
to Prism directly.

One issue was when we have the following code Prism was using
`putobject` with an empty hash whereas the parse.y parser used `putnil`.

```ruby
:ok.itself(**nil)
```

Before:

```
0000 putobject                              :ok                       (   1)[Li]
0002 putobject                              {}
0004 opt_send_without_block                 <calldata!mid:itself, argc:1, KW_SPLAT>
0006 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,17)>
0000 putobject                              :ok                       (   1)[Li]
0002 putnil
0003 opt_send_without_block                 <calldata!mid:itself, argc:1, KW_SPLAT>
0005 leave
```

Related to ruby/prism#2935.
kddnewton pushed a commit to ruby/ruby that referenced this issue Jul 24, 2024
This addresses one of the issues in the `test_kw_splat_nil` failure, but
doesn't make the test pass because of other changes that need to be made
to Prism directly.

One issue was when we have the following code Prism was using
`putobject` with an empty hash whereas the parse.y parser used `putnil`.

```ruby
:ok.itself(**nil)
```

Before:

```
0000 putobject                              :ok                       (   1)[Li]
0002 putobject                              {}
0004 opt_send_without_block                 <calldata!mid:itself, argc:1, KW_SPLAT>
0006 leave
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(1,17)>
0000 putobject                              :ok                       (   1)[Li]
0002 putnil
0003 opt_send_without_block                 <calldata!mid:itself, argc:1, KW_SPLAT>
0005 leave
```

Related to ruby/prism#2935.
kddnewton pushed a commit to ruby/ruby that referenced this issue Jul 24, 2024
In the following code:

```ruby
def foo(&blk)
  blk.call
end
```

Prism was using the `getblockparam` instruction but it should be
`getblockparamproxy`. In this case we have a receiver, if it's a local
variable read node, then it needs to go through the path to use
`getblockparamproxy`.

Before:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(3,3)>
0000 definemethod                           :foo, foo                 (   1)[Li]
0003 putobject                              :foo
0005 leave

== disasm: #<ISeq:foo@test2.rb:1 (1,0)-(3,3)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: 0, kw: -1@-1, kwrest: -1])
[ 1] blk@0<Block>
0000 getblockparam                          blk@0, 0                  (   2)[LiCa]
0003 opt_send_without_block                 <calldata!mid:call, argc:0, ARGS_SIMPLE>
0005 leave                                                            (   3)[Re]
```

After:

```
== disasm: #<ISeq:<main>@test2.rb:1 (1,0)-(3,3)>
0000 definemethod                           :foo, foo                 (   1)[Li]
0003 putobject                              :foo
0005 leave

== disasm: #<ISeq:foo@test2.rb:1 (1,0)-(3,3)>
local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: 0, kw: -1@-1, kwrest: -1])
[ 1] blk@0<Block>
0000 getblockparamproxy                     blk@0, 0                  (   2)[LiCa]
0003 opt_send_without_block                 <calldata!mid:call, argc:0, ARGS_SIMPLE>
0005 leave                                                            (   3)[Re]
```

Fixes `test_getblockparamproxy` and `test_ifunc_getblockparamproxy` in
test_yjit.rb. Related to ruby/prism#2935.
eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 20, 2024
eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 20, 2024
eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 20, 2024
@kddnewton
Copy link
Collaborator Author

Closing this in favor of the individual tickets that have been opened.

@kddnewton kddnewton unpinned this issue Aug 21, 2024
eileencodes added a commit to eileencodes/ruby that referenced this issue Aug 21, 2024
kddnewton pushed a commit to ruby/ruby that referenced this issue Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler
Projects
None yet
Development

No branches or pull requests

1 participant