From 69d60cc67ba0d7cf9318ccafa9d4c625103e955d Mon Sep 17 00:00:00 2001 From: Jemma Issroff Date: Mon, 11 Dec 2023 15:17:48 -0500 Subject: [PATCH] [PRISM] Properly compile MultiTargetNodes within parameters If there are MultiTargetNodes within parameters, we need to iterate over them and compile them individually correctly, once the locals are all in the correct spaces. We need to add one getlocal for the hidden variable, and then can recurse into the MultiTargetNodes themselves --- prism_compile.c | 58 ++++++++++++++++++++++++++++---- test/ruby/test_compile_prism.rb | 59 +++++++++++++++++++++++++++++---- 2 files changed, 105 insertions(+), 12 deletions(-) diff --git a/prism_compile.c b/prism_compile.c index 708a2baff60541..b1de3c20e32ca3 100644 --- a/prism_compile.c +++ b/prism_compile.c @@ -3709,6 +3709,13 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret, PM_POP_IF_POPPED; return; } + case PM_REQUIRED_PARAMETER_NODE: { + pm_required_parameter_node_t *required_parameter_node = (pm_required_parameter_node_t *)node; + int index = pm_lookup_local_index(iseq, scope_node, required_parameter_node->name); + + ADD_SETLOCAL(ret, &dummy_line_node, index, 0); + return; + } case PM_MULTI_TARGET_NODE: { pm_multi_target_node_t *cast = (pm_multi_target_node_t *) node; bool has_rest_expression = (cast->rest && @@ -3723,17 +3730,22 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret, } } + if (has_rest_expression) { + if (cast->rights.size) { + ADD_INSN2(ret, &dummy_line_node, expandarray, INT2FIX(cast->rights.size), INT2FIX(3)); + } + pm_node_t *expression = ((pm_splat_node_t *)cast->rest)->expression; + PM_COMPILE_NOT_POPPED(expression); + } + if (cast->rights.size) { - ADD_INSN2(ret, &dummy_line_node, expandarray, INT2FIX(cast->rights.size), INT2FIX(2)); + if (!has_rest_expression) { + ADD_INSN2(ret, &dummy_line_node, expandarray, INT2FIX(cast->rights.size), INT2FIX(2)); + } for (size_t index = 0; index < cast->rights.size; index++) { PM_COMPILE_NOT_POPPED(cast->rights.nodes[index]); } } - - if (has_rest_expression) { - pm_node_t *expression = ((pm_splat_node_t *)cast->rest)->expression; - PM_COMPILE_NOT_POPPED(expression); - } return; } case PM_MULTI_WRITE_NODE: { @@ -4368,6 +4380,11 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret, // those anonymous items temporary names (as below) int local_index = 0; + // We will assign these values now, if applicable, and use them for + // the ISEQs on these multis + int required_multis_hidden_index = 0; + int post_multis_hidden_index = 0; + // Here we figure out local table indices and insert them in to the // index lookup table and local tables. // @@ -4384,6 +4401,7 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret, // def foo(a, (b, *c, d), e = 1, *f, g, (h, *i, j), k:, l: 1, **m, &n) // ^^^^^^^^^^ case PM_MULTI_TARGET_NODE: { + required_multis_hidden_index = local_index; local = rb_make_temporary_id(local_index); local_table_for_iseq->ids[local_index] = local; break; @@ -4453,6 +4471,7 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret, // def foo(a, (b, *c, d), e = 1, *f, g, (h, *i, j), k:, l: 1, **m, &n) // ^^^^^^^^^^ case PM_MULTI_TARGET_NODE: { + post_multis_hidden_index = local_index; local = rb_make_temporary_id(local_index); local_table_for_iseq->ids[local_index] = local; break; @@ -4638,6 +4657,7 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret, // Goal: fill in the names of the parameters in MultiTargetNodes // // Go through requireds again to set the multis + if (requireds_list && requireds_list->size) { for (size_t i = 0; i < requireds_list->size; i++) { // For each MultiTargetNode, we're going to have one @@ -4757,6 +4777,32 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret, body->param.opt_table = (const VALUE *)opt_table; } + if (requireds_list && requireds_list->size) { + for (size_t i = 0; i < requireds_list->size; i++) { + // For each MultiTargetNode, we're going to have one + // additional anonymous local not represented in the locals table + // We want to account for this in our table size + pm_node_t *required = requireds_list->nodes[i]; + if (PM_NODE_TYPE_P(required, PM_MULTI_TARGET_NODE)) { + ADD_GETLOCAL(ret, &dummy_line_node, table_size - required_multis_hidden_index, 0); + PM_COMPILE(required); + } + } + } + + if (posts_list && posts_list->size) { + for (size_t i = 0; i < posts_list->size; i++) { + // For each MultiTargetNode, we're going to have one + // additional anonymous local not represented in the locals table + // We want to account for this in our table size + pm_node_t *post = posts_list->nodes[i]; + if (PM_NODE_TYPE_P(post, PM_MULTI_TARGET_NODE)) { + ADD_GETLOCAL(ret, &dummy_line_node, table_size - post_multis_hidden_index, 0); + PM_COMPILE(post); + } + } + } + switch (body->type) { case ISEQ_TYPE_BLOCK: { LABEL *start = ISEQ_COMPILE_DATA(iseq)->start_label = NEW_LABEL(0); diff --git a/test/ruby/test_compile_prism.rb b/test/ruby/test_compile_prism.rb index dde51b3cb253f5..2bc1821936e879 100644 --- a/test/ruby/test_compile_prism.rb +++ b/test/ruby/test_compile_prism.rb @@ -505,6 +505,7 @@ def test_MultiTargetNode assert_prism_eval("a, (b, *c) = [1, [2, 3]]; c") assert_prism_eval("a, (b, *c) = 1, [2, 3]; c") assert_prism_eval("a, (b, *) = 1, [2, 3]; b") + assert_prism_eval("a, (b, *c, d) = 1, [2, 3, 4]; [a, b, c, d]") assert_prism_eval("(a, (b, c, d, e), f, g), h = [1, [2, 3]], 4, 5, [6, 7]; c") end @@ -1175,12 +1176,58 @@ def self.prism_test_def_node(x = 1, y, a: 8, b: 2, c: 4) assert_prism_eval("class PrismTestDefNode; def prism_test_def_node(*a) a end end; PrismTestDefNode.new.prism_test_def_node(1).inspect") # block argument - assert_prism_eval(<<-CODE - def self.prism_test_def_node(&block) prism_test_def_node2(&block) end - def self.prism_test_def_node2() yield 1 end - prism_test_def_node2 {|a| a } - CODE - ) + assert_prism_eval(<<-CODE) + def self.prism_test_def_node(&block) prism_test_def_node2(&block) end + def self.prism_test_def_node2() yield 1 end + prism_test_def_node2 {|a| a } + CODE + + # multi argument + assert_prism_eval(<<-CODE) + def self.prism_test_def_node(a, (b, *c, d)) + [a, b, c, d] + end + prism_test_def_node("a", ["b", "c", "d"]) + CODE + assert_prism_eval(<<-CODE) + def self.prism_test_def_node(a, (b, c, *)) + [a, b, c] + end + prism_test_def_node("a", ["b", "c"]) + CODE + assert_prism_eval(<<-CODE) + def self.prism_test_def_node(a, (*, b, c)) + [a, b, c] + end + prism_test_def_node("a", ["b", "c"]) + CODE + + # recursive multis + assert_prism_eval(<<-CODE) + def self.prism_test_def_node(a, (b, *c, (d, *e, f))) + [a, b, c, d, d, e, f] + end + prism_test_def_node("a", ["b", "c", ["d", "e", "f"]]) + CODE + + # Many arguments + assert_prism_eval(<<-CODE) + def self.prism_test_def_node(a, (b, *c, d), e = 1, *f, g, (h, *i, j), k:, l: 1, **m) + [a, b, c, d, e, f, g, h, i, j, k, l, m] + end + prism_test_def_node( + "a", + ["b", "c1", "c2", "d"], + "e", + "f1", "f2", + "g", + ["h", "i1", "i2", "j"], + k: "k", + l: "l", + m1: "m1", + m2: "m2" + ) + CODE end def test_method_parameters