Skip to content

Commit

Permalink
[Prism] Fix InterpolatedMatchLastLine Instructions
Browse files Browse the repository at this point in the history 
I looked at this at RubyConf with Kevin, and we noticed that there was a
`putobject` empty string missing from the instructions. I just got back
around to implementing this and pushing a PR and while doing that
noticed that we also have a `getspecial` when we want a `getglobal`.

This change adds the `putobject` instruction and replaces the
`getspecial` with a `getglobal`. If we look at the parsetree for the
following code:

```ruby
$pit = '.oo'; if /"#{$pit}"/mix; end
```

We can see it has a `NODE_GVAR` and the [Ruby
compiler](https://github.com/ruby/ruby/blob/a4b43e92645e46ee5a8c9af42d3de57cd052e87c/compile.c#L10024-L10030) shows that should
use `getglobal.

```
 @ NODE_SCOPE (id: 14, line: 1, location: (1,0)-(22,36))
 +- nd_tbl: (empty)
 +- nd_args:
 |   (null node)
 +- nd_body:
     @ NODE_BLOCK (id: 12, line: 22, location: (22,0)-(22,36))
     +- nd_head (1):
     |   @ NODE_GASGN (id: 0, line: 22, location: (22,0)-(22,12))*
     |   +- nd_vid: :$pit
     |   +- nd_value:
     |       @ NODE_STR (id: 1, line: 22, location: (22,7)-(22,12))
     |       +- nd_lit: ".oo"
     +- nd_head (2):
         @ NODE_IF (id: 11, line: 22, location: (22,14)-(22,36))*
         +- nd_cond:
         |   @ NODE_MATCH2 (id: 10, line: 22, location: (22,14)-(22,36))
         |   +- nd_recv:
         |   |   @ NODE_DREGX (id: 2, line: 22, location: (22,17)-(22,31))
         |   |   +- nd_lit: "\""
         |   |   +- nd_next->nd_head:
         |   |   |   @ NODE_EVSTR (id: 4, line: 22, location: (22,19)-(22,26))
         |   |   |   +- nd_body:
         |   |   |       @ NODE_GVAR (id: 3, line: 22, location: (22,21)-(22,25))
         |   |   |       +- nd_vid: :$pit
         |   |   +- nd_next->nd_next:
         |   |       @ NODE_LIST (id: 7, line: 22, location: (22,26)-(22,27))
         |   |       +- as.nd_alen: 1
         |   |       +- nd_head:
         |   |       |   @ NODE_STR (id: 6, line: 22, location: (22,26)-(22,27))
         |   |       |   +- nd_lit: "\""
         |   |       +- nd_next:
         |   |           (null node)
         |   +- nd_value:
         |       @ NODE_GVAR (id: 9, line: 22, location: (22,14)-(22,36))
         |       +- nd_vid: :$_
         +- nd_body:
         |   @ NODE_BEGIN (id: 8, line: 22, location: (22,32)-(22,32))
         |   +- nd_body:
         |       (null node)
         +- nd_else:
             (null node)
```

I'm struggling with writing a failing test, but the before/after
instructions show that `getglobal` is correct here. I compared the
instructions for the  other `InterpolatedMatchLastLine` node tests
and they also used `getglobal`. I've edited the existing
`InterpolatedLastMatchLineNode` test so that it will use that instruction when
copied out of the test. Without the quotes it thinks it's just a
`MatchLastLineNode`.

Incorrect instructions:

```
"********* Ruby *************"
== disasm: #<ISeq:<compiled>@<compiled>:1 (1,0)-(22,36)>
0000 putstring                              ".oo"                     (  22)[Li]
0002 setglobal                              :$pit
0004 putobject                              "\""
0006 getglobal                              :$pit
0008 dup
0009 objtostring                            <calldata!mid:to_s, argc:0, FCALL|ARGS_SIMPLE>
0011 anytostring
0012 putobject                              "\""
0014 toregexp                               7, 3
0017 getglobal                              :$_
0019 send                                   <calldata!mid:=~, argc:1, ARGS_SIMPLE>, nil
0022 branchunless                           30
0024 jump                                   26
0026 putnil
0027 jump                                   31
0029 pop
0030 putnil
0031 leave

"********* PRISM *************"
== disasm: #<ISeq:<compiled>@<compiled>:21 (21,0)-(21,36)>
0000 putstring                              ".oo"                     (  21)[Li]
0002 setglobal                              :$pit
0004 putobject                              "\""
0006 getglobal                              :$pit
0008 dup
0009 objtostring                            <calldata!mid:to_s, argc:0, FCALL|ARGS_SIMPLE>
0011 anytostring
0012 putobject                              "\""
0014 toregexp                               7, 3
0017 getspecial                             0, 0
0020 send                                   <calldata!mid:=~, argc:1, ARGS_SIMPLE>, nil
0023 branchunless                           31
0025 jump                                   27
0027 putnil
0028 jump                                   32
0030 pop
0031 putnil
0032 leave
```

Fixed instructions:

```
== disasm: #<ISeq:<compiled>@<compiled>:1 (1,0)-(22,36)>
0000 putstring                              ".oo"                     (  22)[Li]
0002 setglobal                              :$pit
0004 putobject                              "\""
0006 getglobal                              :$pit
0008 dup
0009 objtostring                            <calldata!mid:to_s, argc:0, FCALL|ARGS_SIMPLE>
0011 anytostring
0012 putobject                              "\""
0014 toregexp                               7, 3
0017 getglobal                              :$_
0019 send                                   <calldata!mid:=~, argc:1, ARGS_SIMPLE>, nil
0022 branchunless                           30
0024 jump                                   26
0026 putnil
0027 jump                                   31
0029 pop
0030 putnil
0031 leave

"********* PRISM *************"
== disasm: #<ISeq:<compiled>@<compiled>:21 (21,0)-(21,36)>
0000 putstring                              ".oo"                     (  21)[Li]
0002 setglobal                              :$pit
0004 putobject                              "\""
0006 getglobal                              :$pit
0008 dup
0009 objtostring                            <calldata!mid:to_s, argc:0, FCALL|ARGS_SIMPLE>
0011 anytostring
0012 putobject                              "\""
0014 toregexp                               7, 3
0017 getglobal                              :$_
0019 send                                   <calldata!mid:=~, argc:1, ARGS_SIMPLE>, nil
0022 branchunless                           30
0024 jump                                   26
0026 putnil
0027 jump                                   31
0029 pop
0030 putnil
0031 leave
```
  • Loading branch information
@eileencodes @jemmaissroff
eileencodes authored and jemmaissroff committed Dec 13, 2023
1 parent ea4a4c3 commit 110dbf6
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 3 deletions.
10 changes: 8 additions & 2 deletions prism_compile.c
View file
Expand Up @@ -3480,11 +3480,17 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
case PM_INTERPOLATED_MATCH_LAST_LINE_NODE: {
pm_interpolated_match_last_line_node_t *cast = (pm_interpolated_match_last_line_node_t *) node;

int parts_size = (int)cast->parts.size;
if (parts_size > 0 && !PM_NODE_TYPE_P(cast->parts.nodes[0], PM_STRING_NODE)) {
ADD_INSN1(ret, &dummy_line_node, putobject, rb_str_new(0, 0));
parts_size++;
}

pm_interpolated_node_compile(&cast->parts, iseq, dummy_line_node, ret, src, popped, scope_node, parser);

ADD_INSN2(ret, &dummy_line_node, toregexp, INT2FIX(pm_reg_flags(node)), INT2FIX((int) (cast->parts.size)));
ADD_INSN2(ret, &dummy_line_node, toregexp, INT2FIX(pm_reg_flags(node)), INT2FIX(parts_size));

ADD_INSN2(ret, &dummy_line_node, getspecial, INT2FIX(0), INT2FIX(0));
ADD_INSN1(ret, &dummy_line_node, getglobal, rb_id2sym(idLASTLINE));
ADD_SEND(ret, &dummy_line_node, idEqTilde, INT2NUM(1));
PM_POP_IF_POPPED;

Expand Down
2 changes: 1 addition & 1 deletion test/ruby/test_compile_prism.rb
View file
Expand Up @@ -546,7 +546,7 @@ def test_EmbeddedVariableNode
end

def test_InterpolatedMatchLastLineNode
assert_prism_eval("$pit = '.oo'; if /\#$pit/mix; end")
assert_prism_eval('$pit = ".oo"; if /"#{$pit}"/mix; end')
end

def test_InterpolatedRegularExpressionNode
Expand Down

0 comments on commit 110dbf6

Please sign in to comment.