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.

Sign up for GitHub

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

Jump to bottom

[PRISM] Handle stack and local table sizes for repeated params #9686

Merged
merged 7 commits into from
Jan 24, 2024
Merged

[PRISM] Handle stack and local table sizes for repeated params #9686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cbf0c23
Fix required positional repeated _ parameters
tenderlove Jan 23, 2024
67e37e5
Fix repeated optional _ parameters
tenderlove Jan 23, 2024
133bae5
Fix local table space for `*_`
tenderlove Jan 23, 2024
8edd456
Fix local table size and index for required post underscore
tenderlove Jan 23, 2024
2fc44fd
Fix local table size and variable offset for repeated keywords
tenderlove Jan 23, 2024
d9cabe0
Fix local table size / index for repeated kwrest
tenderlove Jan 23, 2024
baac595
Fix repeated block param
tenderlove Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
113 changes: 98 additions & 15 deletions prism_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6328,16 +6328,37 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
}
}

// Ensure there is enough room in the local table for any
// parameters that have been repeated
// ex: def underscore_parameters(_, _ = 1, _ = 2); _; end
// ^^^^^^^^^^^^
if (optionals_list && optionals_list->size) {
for (size_t i = 0; i < optionals_list->size; i++) {
pm_node_t * node = optionals_list->nodes[i];
if (PM_NODE_FLAG_P(node, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
table_size++;
}
}
}

// If we have an anonymous "rest" node, we'll need to increase the local
// table size to take it in to account.
// def m(foo, *, bar)
// ^
if (parameters_node && parameters_node->rest) {
if (!(PM_NODE_TYPE_P(parameters_node->rest, PM_IMPLICIT_REST_NODE))) {
if (!((pm_rest_parameter_node_t *)parameters_node->rest)->name) {
table_size++;
if (parameters_node) {
if (parameters_node->rest) {
if (!(PM_NODE_TYPE_P(parameters_node->rest, PM_IMPLICIT_REST_NODE))) {
if (!((pm_rest_parameter_node_t *)parameters_node->rest)->name || PM_NODE_FLAG_P(parameters_node->rest, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
table_size++;
}
}
}

// def underscore_parameters(_, **_); _; end
// ^^^
if (parameters_node->keyword_rest && PM_NODE_FLAG_P(parameters_node->keyword_rest, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
table_size++;
}
}

if (posts_list) {
Expand All @@ -6346,7 +6367,16 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
// additional anonymous local not represented in the locals table
// We want to account for this in our table size
pm_node_t *required = posts_list->nodes[i];
if (PM_NODE_TYPE_P(required, PM_MULTI_TARGET_NODE)) {
if (PM_NODE_TYPE_P(required, PM_MULTI_TARGET_NODE) || PM_NODE_FLAG_P(required, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
table_size++;
}
}
}

if (keywords_list && keywords_list->size) {
for (size_t i = 0; i < keywords_list->size; i++) {
pm_node_t *keyword_parameter_node = keywords_list->nodes[i];
if (PM_NODE_FLAG_P(keyword_parameter_node, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
table_size++;
}
}
Expand All @@ -6356,6 +6386,12 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
table_size++;
}

if (parameters_node && parameters_node->block) {
if (PM_NODE_FLAG_P(parameters_node->block, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
table_size++;
}
}

// When we have a `...` as the keyword_rest, it's a forwarding_parameter_node and
// we need to leave space for 2 more locals on the locals table (`*` and `&`)
if (parameters_node && parameters_node->keyword_rest &&
Expand Down Expand Up @@ -6418,7 +6454,11 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
case PM_REQUIRED_PARAMETER_NODE: {
pm_required_parameter_node_t * param = (pm_required_parameter_node_t *)required;

if (!PM_NODE_FLAG_P(required, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
if (PM_NODE_FLAG_P(required, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
ID local = pm_constant_id_lookup(scope_node, param->name);
local_table_for_iseq->ids[local_index] = local;
}
else {
pm_insert_local_index(param->name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
break;
Expand All @@ -6440,8 +6480,16 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
body->param.flags.has_opt = true;

for (size_t i = 0; i < optionals_list->size; i++, local_index++) {
pm_constant_id_t name = ((pm_optional_parameter_node_t *)optionals_list->nodes[i])->name;
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
pm_node_t * node = optionals_list->nodes[i];
pm_constant_id_t name = ((pm_optional_parameter_node_t *)node)->name;

if (PM_NODE_FLAG_P(node, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
ID local = pm_constant_id_lookup(scope_node, name);
local_table_for_iseq->ids[local_index] = local;
}
else {
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
}
}

Expand All @@ -6459,7 +6507,13 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
if (name) {
// def foo(a, (b, *c, d), e = 1, *f, g, (h, *i, j), k:, l: 1, **m, &n)
// ^^
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
if (PM_NODE_FLAG_P(parameters_node->rest, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
ID local = pm_constant_id_lookup(scope_node, name);
local_table_for_iseq->ids[local_index] = local;
}
else {
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
}
else {
// def foo(a, (b, *c, d), e = 1, *, g, (h, *i, j), k:, l: 1, **m, &n)
Expand Down Expand Up @@ -6497,8 +6551,14 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
// ^
case PM_REQUIRED_PARAMETER_NODE: {
pm_required_parameter_node_t * param = (pm_required_parameter_node_t *)post_node;
if (PM_NODE_FLAG_P(param, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
ID local = pm_constant_id_lookup(scope_node, param->name);
local_table_for_iseq->ids[local_index] = local;
}
else {

pm_insert_local_index(param->name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
pm_insert_local_index(param->name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
break;
}
default: {
Expand Down Expand Up @@ -6533,7 +6593,13 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
name = ((pm_required_keyword_parameter_node_t *)keyword_parameter_node)->name;
keyword->required_num++;
ID local = pm_constant_id_lookup(scope_node, name);
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);

if (PM_NODE_FLAG_P(keyword_parameter_node, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
local_table_for_iseq->ids[local_index] = local;
}
else {
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
local_index++;
ids[kw_index++] = local;
}
Expand Down Expand Up @@ -6563,7 +6629,12 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
}

ID local = pm_constant_id_lookup(scope_node, name);
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
if (PM_NODE_FLAG_P(keyword_parameter_node, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
local_table_for_iseq->ids[local_index] = local;
}
else {
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
ids[kw_index++] = local;
local_index++;
}
Expand Down Expand Up @@ -6621,7 +6692,13 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,

pm_constant_id_t constant_id = kw_rest_node->name;
if (constant_id) {
pm_insert_local_index(constant_id, local_index, index_lookup_table, local_table_for_iseq, scope_node);
if (PM_NODE_FLAG_P(kw_rest_node, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
ID local = pm_constant_id_lookup(scope_node, constant_id);
local_table_for_iseq->ids[local_index] = local;
}
else {
pm_insert_local_index(constant_id, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
}
else {
local_table_for_iseq->ids[local_index] = PM_CONSTANT_POW;
Expand Down Expand Up @@ -6663,14 +6740,20 @@ pm_compile_node(rb_iseq_t *iseq, const pm_node_t *node, LINK_ANCHOR *const ret,
body->param.flags.has_block = true;

pm_constant_id_t name = ((pm_block_parameter_node_t *)parameters_node->block)->name;

if (name == 0) {
local_table_for_iseq->ids[local_index] = PM_CONSTANT_AND;
st_insert(index_lookup_table, PM_CONSTANT_AND, local_index);
}
else {
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
if (PM_NODE_FLAG_P(parameters_node->block, PM_PARAMETER_FLAGS_REPEATED_PARAMETER)) {
ID local = pm_constant_id_lookup(scope_node, name);
local_table_for_iseq->ids[local_index] = local;
}
else {
pm_insert_local_index(name, local_index, index_lookup_table, local_table_for_iseq, scope_node);
}
}

local_index++;
}
}
Expand Down
29 changes: 29 additions & 0 deletions test/ruby/test_compile_prism.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1545,6 +1545,35 @@ def self.prism_test_def_node(a, (b, *c, d), e = 1, *f, g, (h, *i, j), k:, l: 1,
CODE
end

def test_repeated_block_underscore
assert_prism_eval("def self.m(_, **_, &_); _; end; method(:m).parameters")
end

def test_repeated_kw_rest_underscore
assert_prism_eval("def self.m(_, **_); _; end; method(:m).parameters")
end

def test_repeated_required_keyword_underscore
assert_prism_eval("def self.m(_, _, *_, _, _:); _; end; method(:m).parameters")
assert_prism_eval("def self.m(_, _, *_, _, _:, _: 2); _; end; method(:m).parameters")
end

def test_repeated_required_post_underscore
assert_prism_eval("def self.m(_, _, *_, _); _; end; method(:m).parameters")
end

def test_repeated_splat_underscore
assert_prism_eval("def self.m(_, _, _ = 1, _ = 2, *_); end; method(:m).parameters")
end

def test_repeated_optional_underscore
assert_prism_eval("def self.m(a, _, _, _ = 1, _ = 2, b); end; method(:m).parameters")
end

def test_repeated_required_underscore
assert_prism_eval("def self.m(a, _, _, b); end; method(:m).parameters")
end

def test_locals_in_parameters
assert_prism_eval("def self.m(a = b = c = 1); [a, b, c]; end; self.m")
end
Expand Down