From c384ef07991d08dc378bf6450363aaa654099813 Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Tue, 5 Sep 2023 12:34:16 -0400 Subject: [PATCH] [ruby/yarp] Introduce a BlockLocalVariableNode This is a tradeoff that I think is worth it. Right now we have a location list that tracks the location of each of the block locals. Instead, I'd like to make that a node list that has a proper node in each spot in the list. In doing so, we eliminate the need to have a location list at all, making it simpler on all of the various consumers as we have one fewer field type. There should be minimal memory implications here since this syntax is exceedingly rare. https://github.com/ruby/yarp/commit/04d329ddf0 --- test/yarp/location_test.rb | 6 ++++ test/yarp/snapshots/procs.txt | 4 ++- .../snapshots/seattlerb/block_arg_scope.txt | 2 +- .../snapshots/seattlerb/block_arg_scope2.txt | 3 +- test/yarp/snapshots/seattlerb/block_scope.txt | 7 +++- .../snapshots/seattlerb/pipe_semicolon.txt | 7 +++- .../snapshots/seattlerb/stabby_proc_scope.txt | 2 +- .../unparser/corpus/literal/block.txt | 9 +++--- .../unparser/corpus/literal/lambda.txt | 2 +- test/yarp/snapshots/whitequark/arg_scope.txt | 7 +++- test/yarp/snapshots/whitequark/blockargs.txt | 4 +-- .../whitequark/send_lambda_args_shadow.txt | 3 +- yarp/config.yml | 11 ++++++- yarp/node.h | 4 --- yarp/templates/ext/yarp/api_node.c.erb | 6 ---- yarp/templates/include/yarp/ast.h.erb | 7 ---- yarp/templates/lib/yarp/node.rb.erb | 2 +- yarp/templates/lib/yarp/serialize.rb.erb | 1 - yarp/templates/src/node.c.erb | 28 ---------------- yarp/templates/src/prettyprint.c.erb | 7 ---- yarp/templates/src/serialize.c.erb | 6 ---- yarp/templates/template.rb | 12 ------- yarp/yarp.c | 32 +++++++++++++++---- 23 files changed, 77 insertions(+), 95 deletions(-) diff --git a/test/yarp/location_test.rb b/test/yarp/location_test.rb index 192cfdd716cdb4..6e544e86b011eb 100644 --- a/test/yarp/location_test.rb +++ b/test/yarp/location_test.rb @@ -67,6 +67,12 @@ def test_BlockArgumentNode assert_location(BlockArgumentNode, "foo(&bar)", 4...8) { |node| node.arguments.arguments.last } end + def test_BlockLocalVariableNode + assert_location(BlockLocalVariableNode, "foo { |;bar| }", 8...11) do |node| + node.block.parameters.locals.first + end + end + def test_BlockNode assert_location(BlockNode, "foo {}", 4...6, &:block) assert_location(BlockNode, "foo do end", 4...10, &:block) diff --git a/test/yarp/snapshots/procs.txt b/test/yarp/snapshots/procs.txt index adfb0637079532..4510160c44178a 100644 --- a/test/yarp/snapshots/procs.txt +++ b/test/yarp/snapshots/procs.txt @@ -16,7 +16,9 @@ ProgramNode(0...266)( nil, nil ), - [(7...8), (10...11), (13...14)], + [BlockLocalVariableNode(7...8)(:b), + BlockLocalVariableNode(10...11)(:c), + BlockLocalVariableNode(13...14)(:d)], (3...4), (14...15) ), diff --git a/test/yarp/snapshots/seattlerb/block_arg_scope.txt b/test/yarp/snapshots/seattlerb/block_arg_scope.txt index 498d41cbe39aa3..be99e78eecc891 100644 --- a/test/yarp/snapshots/seattlerb/block_arg_scope.txt +++ b/test/yarp/snapshots/seattlerb/block_arg_scope.txt @@ -20,7 +20,7 @@ ProgramNode(0...12)( nil, nil ), - [(8...9)], + [BlockLocalVariableNode(8...9)(:c)], (4...5), (9...10) ), diff --git a/test/yarp/snapshots/seattlerb/block_arg_scope2.txt b/test/yarp/snapshots/seattlerb/block_arg_scope2.txt index 787e8cb10a7437..fcee4898aa00a2 100644 --- a/test/yarp/snapshots/seattlerb/block_arg_scope2.txt +++ b/test/yarp/snapshots/seattlerb/block_arg_scope2.txt @@ -20,7 +20,8 @@ ProgramNode(0...14)( nil, nil ), - [(7...8), (10...11)], + [BlockLocalVariableNode(7...8)(:c), + BlockLocalVariableNode(10...11)(:d)], (3...4), (11...12) ), diff --git a/test/yarp/snapshots/seattlerb/block_scope.txt b/test/yarp/snapshots/seattlerb/block_scope.txt index 223268c471d574..bcdb26caa70706 100644 --- a/test/yarp/snapshots/seattlerb/block_scope.txt +++ b/test/yarp/snapshots/seattlerb/block_scope.txt @@ -10,7 +10,12 @@ ProgramNode(0...10)( nil, BlockNode(2...10)( [:b], - BlockParametersNode(4...8)(nil, [(6...7)], (4...5), (7...8)), + BlockParametersNode(4...8)( + nil, + [BlockLocalVariableNode(6...7)(:b)], + (4...5), + (7...8) + ), nil, (2...3), (9...10) diff --git a/test/yarp/snapshots/seattlerb/pipe_semicolon.txt b/test/yarp/snapshots/seattlerb/pipe_semicolon.txt index cd40d9ac0b18b4..14dc672e1c2339 100644 --- a/test/yarp/snapshots/seattlerb/pipe_semicolon.txt +++ b/test/yarp/snapshots/seattlerb/pipe_semicolon.txt @@ -10,7 +10,12 @@ ProgramNode(0...18)( nil, BlockNode(4...18)( [:c], - BlockParametersNode(7...14)(nil, [(11...12)], (7...8), (13...14)), + BlockParametersNode(7...14)( + nil, + [BlockLocalVariableNode(11...12)(:c)], + (7...8), + (13...14) + ), nil, (4...6), (15...18) diff --git a/test/yarp/snapshots/seattlerb/stabby_proc_scope.txt b/test/yarp/snapshots/seattlerb/stabby_proc_scope.txt index c4594997ebd59e..d8e06afa0fadec 100644 --- a/test/yarp/snapshots/seattlerb/stabby_proc_scope.txt +++ b/test/yarp/snapshots/seattlerb/stabby_proc_scope.txt @@ -16,7 +16,7 @@ ProgramNode(0...11)( nil, nil ), - [(6...7)], + [BlockLocalVariableNode(6...7)(:b)], (2...3), (7...8) ), diff --git a/test/yarp/snapshots/unparser/corpus/literal/block.txt b/test/yarp/snapshots/unparser/corpus/literal/block.txt index a115e84135b5f9..6ee2fbb1fa8803 100644 --- a/test/yarp/snapshots/unparser/corpus/literal/block.txt +++ b/test/yarp/snapshots/unparser/corpus/literal/block.txt @@ -91,7 +91,7 @@ ProgramNode(0...737)( nil, nil ), - [(44...45)], + [BlockLocalVariableNode(44...45)(:x)], (39...40), (45...46) ), @@ -326,7 +326,7 @@ ProgramNode(0...737)( nil, nil ), - [(181...182)], + [BlockLocalVariableNode(181...182)(:b)], (176...177), (182...183) ), @@ -366,7 +366,7 @@ ProgramNode(0...737)( nil, nil ), - [(200...201)], + [BlockLocalVariableNode(200...201)(:b)], (196...197), (201...202) ), @@ -398,7 +398,8 @@ ProgramNode(0...737)( [:a, :b], BlockParametersNode(215...223)( nil, - [(218...219), (221...222)], + [BlockLocalVariableNode(218...219)(:a), + BlockLocalVariableNode(221...222)(:b)], (215...216), (222...223) ), diff --git a/test/yarp/snapshots/unparser/corpus/literal/lambda.txt b/test/yarp/snapshots/unparser/corpus/literal/lambda.txt index 13161f9c881262..8c036bd70dfe60 100644 --- a/test/yarp/snapshots/unparser/corpus/literal/lambda.txt +++ b/test/yarp/snapshots/unparser/corpus/literal/lambda.txt @@ -110,7 +110,7 @@ ProgramNode(0...80)( nil, nil ), - [(74...75)], + [BlockLocalVariableNode(74...75)(:c)], (67...68), (75...76) ), diff --git a/test/yarp/snapshots/whitequark/arg_scope.txt b/test/yarp/snapshots/whitequark/arg_scope.txt index fd5ae0f61d51fe..baa33596becad9 100644 --- a/test/yarp/snapshots/whitequark/arg_scope.txt +++ b/test/yarp/snapshots/whitequark/arg_scope.txt @@ -10,7 +10,12 @@ ProgramNode(0...13)( nil, BlockNode(6...13)( [:a], - BlockParametersNode(7...11)(nil, [(9...10)], (7...8), (10...11)), + BlockParametersNode(7...11)( + nil, + [BlockLocalVariableNode(9...10)(:a)], + (7...8), + (10...11) + ), StatementsNode(11...12)([LocalVariableReadNode(11...12)(:a, 0)]), (6...7), (12...13) diff --git a/test/yarp/snapshots/whitequark/blockargs.txt b/test/yarp/snapshots/whitequark/blockargs.txt index 3115c58df12ff9..76f9dd4c5183cf 100644 --- a/test/yarp/snapshots/whitequark/blockargs.txt +++ b/test/yarp/snapshots/whitequark/blockargs.txt @@ -250,7 +250,7 @@ ProgramNode(0...550)( [:a], BlockParametersNode(117...123)( nil, - [(120...121)], + [BlockLocalVariableNode(120...121)(:a)], (117...118), (122...123) ), @@ -272,7 +272,7 @@ ProgramNode(0...550)( [:a], BlockParametersNode(130...134)( nil, - [(132...133)], + [BlockLocalVariableNode(132...133)(:a)], (130...131), (133...134) ), diff --git a/test/yarp/snapshots/whitequark/send_lambda_args_shadow.txt b/test/yarp/snapshots/whitequark/send_lambda_args_shadow.txt index 9a6c888a930c90..cee0016fa79d2c 100644 --- a/test/yarp/snapshots/whitequark/send_lambda_args_shadow.txt +++ b/test/yarp/snapshots/whitequark/send_lambda_args_shadow.txt @@ -16,7 +16,8 @@ ProgramNode(0...19)( nil, nil ), - [(6...9), (11...14)], + [BlockLocalVariableNode(6...9)(:foo), + BlockLocalVariableNode(11...14)(:bar)], (2...3), (14...15) ), diff --git a/yarp/config.yml b/yarp/config.yml index a7fb8651ad3bd7..b9e03b5fbacbfa 100644 --- a/yarp/config.yml +++ b/yarp/config.yml @@ -520,6 +520,15 @@ nodes: bar(&args) ^^^^^^^^^^ + - name: BlockLocalVariableNode + fields: + - name: name + type: constant + comment: | + Represents a block local variable. + + a { |; b| } + ^ - name: BlockNode fields: - name: locals @@ -556,7 +565,7 @@ nodes: type: node? kind: ParametersNode - name: locals - type: location[] + type: node[] - name: opening_loc type: location? - name: closing_loc diff --git a/yarp/node.h b/yarp/node.h index f2d5be8bf22e91..1b546f086f0004 100644 --- a/yarp/node.h +++ b/yarp/node.h @@ -4,9 +4,6 @@ #include "yarp/defines.h" #include "yarp/parser.h" -// Append a token to the given list. -void yp_location_list_append(yp_location_list_t *list, const yp_token_t *token); - // Append a new node onto the end of the node list. void yp_node_list_append(yp_node_list_t *list, yp_node_t *node); @@ -31,7 +28,6 @@ YP_EXPORTED_FUNCTION void yp_node_memsize(yp_node_t *node, yp_memsize_t *memsize YP_EXPORTED_FUNCTION const char * yp_node_type_to_str(yp_node_type_t node_type); #define YP_EMPTY_NODE_LIST ((yp_node_list_t) { .nodes = NULL, .size = 0, .capacity = 0 }) -#define YP_EMPTY_LOCATION_LIST ((yp_location_list_t) { .locations = NULL, .size = 0, .capacity = 0 }) #endif // YARP_NODE_H diff --git a/yarp/templates/ext/yarp/api_node.c.erb b/yarp/templates/ext/yarp/api_node.c.erb index b8407350589711..c3a232ec5fa468 100644 --- a/yarp/templates/ext/yarp/api_node.c.erb +++ b/yarp/templates/ext/yarp/api_node.c.erb @@ -152,12 +152,6 @@ yp_ast_new(yp_parser_t *parser, yp_node_t *node, rb_encoding *encoding) { } <%- when YARP::StringField -%> argv[<%= index %>] = yp_string_new(&cast-><%= field.name %>, encoding); - <%- when YARP::LocationListField -%> - argv[<%= index %>] = rb_ary_new_capa(cast-><%= field.name %>.size); - for (size_t index = 0; index < cast-><%= field.name %>.size; index++) { - yp_location_t location = cast-><%= field.name %>.locations[index]; - rb_ary_push(argv[<%= index %>], yp_location_new(parser, location.start, location.end, source)); - } <%- when YARP::ConstantField -%> argv[<%= index %>] = rb_id2sym(constants[cast-><%= field.name %> - 1]); <%- when YARP::ConstantListField -%> diff --git a/yarp/templates/include/yarp/ast.h.erb b/yarp/templates/include/yarp/ast.h.erb index f7e1d0b503ac22..48eaefda2447ad 100644 --- a/yarp/templates/include/yarp/ast.h.erb +++ b/yarp/templates/include/yarp/ast.h.erb @@ -32,12 +32,6 @@ typedef struct { const uint8_t *end; } yp_location_t; -typedef struct { - yp_location_t *locations; - size_t size; - size_t capacity; -} yp_location_list_t; - struct yp_node; typedef struct yp_node_list { @@ -95,7 +89,6 @@ typedef struct yp_<%= node.human %> { <%= case field when YARP::NodeField, YARP::OptionalNodeField then "struct #{field.c_type} *#{field.name}" when YARP::NodeListField then "struct yp_node_list #{field.name}" - when YARP::LocationListField then "yp_location_list_t #{field.name}" when YARP::ConstantField then "yp_constant_id_t #{field.name}" when YARP::ConstantListField then "yp_constant_id_list_t #{field.name}" when YARP::StringField then "yp_string_t #{field.name}" diff --git a/yarp/templates/lib/yarp/node.rb.erb b/yarp/templates/lib/yarp/node.rb.erb index 8919cb7ad091c6..105830836f8a1d 100644 --- a/yarp/templates/lib/yarp/node.rb.erb +++ b/yarp/templates/lib/yarp/node.rb.erb @@ -100,7 +100,7 @@ module YARP <%- case field -%> <%- when YARP::NodeListField -%> inspector << "<%= pointer %><%= field.name %>: #{inspector.list("#{inspector.prefix}<%= preadd %>", <%= field.name %>)}" - <%- when YARP::LocationListField, YARP::ConstantListField -%> + <%- when YARP::ConstantListField -%> inspector << "<%= pointer %><%= field.name %>: #{<%= field.name %>.inspect}\n" <%- when YARP::NodeField -%> inspector << "<%= pointer %><%= field.name %>:\n" diff --git a/yarp/templates/lib/yarp/serialize.rb.erb b/yarp/templates/lib/yarp/serialize.rb.erb index 230522ae2bf9f4..ee0b8666bfd3e7 100644 --- a/yarp/templates/lib/yarp/serialize.rb.erb +++ b/yarp/templates/lib/yarp/serialize.rb.erb @@ -185,7 +185,6 @@ module YARP when YARP::OptionalNodeField then "load_optional_node" when YARP::StringField then "load_string" when YARP::NodeListField then "Array.new(load_varint) { load_node }" - when YARP::LocationListField then "Array.new(load_varint) { load_location }" when YARP::ConstantField then "load_constant" when YARP::ConstantListField then "Array.new(load_varint) { load_constant }" when YARP::LocationField then "load_location" diff --git a/yarp/templates/src/node.c.erb b/yarp/templates/src/node.c.erb index aa756ed4f9a81f..647950d5824048 100644 --- a/yarp/templates/src/node.c.erb +++ b/yarp/templates/src/node.c.erb @@ -8,30 +8,6 @@ void yp_node_clear(yp_node_t *node) { node->location = location; } -// Calculate the size of the token list in bytes. -static size_t -yp_location_list_memsize(yp_location_list_t *list) { - return sizeof(yp_location_list_t) + (list->capacity * sizeof(yp_location_t)); -} - -// Append a token to the given list. -void -yp_location_list_append(yp_location_list_t *list, const yp_token_t *token) { - if (list->size == list->capacity) { - list->capacity = list->capacity == 0 ? 2 : list->capacity * 2; - list->locations = (yp_location_t *) realloc(list->locations, sizeof(yp_location_t) * list->capacity); - } - list->locations[list->size++] = (yp_location_t) { .start = token->start, .end = token->end }; -} - -// Free the memory associated with the token list. -static void -yp_location_list_free(yp_location_list_t *list) { - if (list->locations != NULL) { - free(list->locations); - } -} - static void yp_node_memsize_node(yp_node_t *node, yp_memsize_t *memsize); @@ -95,8 +71,6 @@ yp_node_destroy(yp_parser_t *parser, yp_node_t *node) { yp_string_free(&cast-><%= field.name %>); <%- when YARP::NodeListField -%> yp_node_list_free(parser, &cast-><%= field.name %>); - <%- when YARP::LocationListField -%> - yp_location_list_free(&cast-><%= field.name %>); <%- when YARP::ConstantListField -%> yp_constant_id_list_free(&cast-><%= field.name %>); <%- else -%> @@ -141,8 +115,6 @@ yp_node_memsize_node(yp_node_t *node, yp_memsize_t *memsize) { memsize->memsize += yp_string_memsize(&cast-><%= field.name %>); <%- when YARP::NodeListField -%> yp_node_list_memsize(&cast-><%= field.name %>, memsize); - <%- when YARP::LocationListField -%> - memsize->memsize += yp_location_list_memsize(&cast-><%= field.name %>); <%- when YARP::ConstantListField -%> memsize->memsize += yp_constant_id_list_memsize(&cast-><%= field.name %>); <%- else -%> diff --git a/yarp/templates/src/prettyprint.c.erb b/yarp/templates/src/prettyprint.c.erb index 4c1f49fdd2f017..793519a1acede9 100644 --- a/yarp/templates/src/prettyprint.c.erb +++ b/yarp/templates/src/prettyprint.c.erb @@ -45,13 +45,6 @@ prettyprint_node(yp_buffer_t *buffer, yp_parser_t *parser, yp_node_t *node) { prettyprint_node(buffer, parser, (yp_node_t *) ((yp_<%= node.human %>_t *) node)-><%= field.name %>.nodes[index]); } yp_buffer_append_str(buffer, "]", 1); - <%- when YARP::LocationListField -%> - yp_buffer_append_str(buffer, "[", 1); - for (uint32_t index = 0; index < ((yp_<%= node.human %>_t *)node)-><%= field.name %>.size; index++) { - if (index != 0) yp_buffer_append_str(buffer, ", ", 2); - prettyprint_location(buffer, parser, &((yp_<%= node.human %>_t *)node)-><%= field.name %>.locations[index]); - } - yp_buffer_append_str(buffer, "]", 1); <%- when YARP::ConstantField -%> char <%= field.name %>_buffer[12]; snprintf(<%= field.name %>_buffer, sizeof(<%= field.name %>_buffer), "%u", ((yp_<%= node.human %>_t *)node)-><%= field.name %>); diff --git a/yarp/templates/src/serialize.c.erb b/yarp/templates/src/serialize.c.erb index b1049ba11668bf..a6411baaf3a4ff 100644 --- a/yarp/templates/src/serialize.c.erb +++ b/yarp/templates/src/serialize.c.erb @@ -86,12 +86,6 @@ yp_serialize_node(yp_parser_t *parser, yp_node_t *node, yp_buffer_t *buffer) { for (uint32_t index = 0; index < <%= field.name %>_size; index++) { yp_serialize_node(parser, (yp_node_t *) ((yp_<%= node.human %>_t *)node)-><%= field.name %>.nodes[index], buffer); } - <%- when YARP::LocationListField -%> - uint32_t <%= field.name %>_size = yp_sizet_to_u32(((yp_<%= node.human %>_t *)node)-><%= field.name %>.size); - yp_buffer_append_u32(buffer, <%= field.name %>_size); - for (uint32_t index = 0; index < <%= field.name %>_size; index++) { - yp_serialize_location(parser, &((yp_<%= node.human %>_t *)node)-><%= field.name %>.locations[index], buffer); - } <%- when YARP::ConstantField -%> yp_buffer_append_u32(buffer, yp_sizet_to_u32(((yp_<%= node.human %>_t *)node)-><%= field.name %>)); <%- when YARP::ConstantListField -%> diff --git a/yarp/templates/template.rb b/yarp/templates/template.rb index 64f1d75f85efb0..428f3d5b3769f7 100755 --- a/yarp/templates/template.rb +++ b/yarp/templates/template.rb @@ -73,17 +73,6 @@ def java_type end end - # This represents a field on a node that is a list of locations. - class LocationListField < Field - def rbs_class - "Array[Location]" - end - - def java_type - "Location[]" - end - end - # This represents a field on a node that is the ID of a string interned # through the parser's constant pool. class ConstantField < Field @@ -205,7 +194,6 @@ def field_type_for(name) when "node?" then OptionalNodeField when "node[]" then NodeListField when "string" then StringField - when "location[]" then LocationListField when "constant" then ConstantField when "constant[]" then ConstantListField when "location" then LocationField diff --git a/yarp/yarp.c b/yarp/yarp.c index 45c26471bbb98e..a820751b42f1ff 100644 --- a/yarp/yarp.c +++ b/yarp/yarp.c @@ -1175,7 +1175,7 @@ yp_block_parameters_node_create(yp_parser_t *parser, yp_parameters_node_t *param .parameters = parameters, .opening_loc = YP_OPTIONAL_LOCATION_TOKEN_VALUE(opening), .closing_loc = { .start = NULL, .end = NULL }, - .locals = YP_EMPTY_LOCATION_LIST + .locals = YP_EMPTY_NODE_LIST }; return node; @@ -1190,14 +1190,30 @@ yp_block_parameters_node_closing_set(yp_block_parameters_node_t *node, const yp_ node->closing_loc = YP_LOCATION_TOKEN_VALUE(closing); } +// Allocate and initialize a new BlockLocalVariableNode node. +static yp_block_local_variable_node_t * +yp_block_local_variable_node_create(yp_parser_t *parser, const yp_token_t *name) { + assert(name->type == YP_TOKEN_IDENTIFIER || name->type == YP_TOKEN_MISSING); + yp_block_local_variable_node_t *node = YP_ALLOC_NODE(parser, yp_block_local_variable_node_t); + + *node = (yp_block_local_variable_node_t) { + { + .type = YP_NODE_BLOCK_LOCAL_VARIABLE_NODE, + .location = YP_LOCATION_TOKEN_VALUE(name), + }, + .name = yp_parser_constant_id_token(parser, name) + }; + + return node; +} + // Append a new block-local variable to a BlockParametersNode node. static void -yp_block_parameters_node_append_local(yp_block_parameters_node_t *node, const yp_token_t *local) { - assert(local->type == YP_TOKEN_IDENTIFIER || local->type == YP_TOKEN_MISSING); +yp_block_parameters_node_append_local(yp_block_parameters_node_t *node, const yp_block_local_variable_node_t *local) { + yp_node_list_append(&node->locals, (yp_node_t *) local); - yp_location_list_append(&node->locals, local); - if (node->base.location.start == NULL) node->base.location.start = local->start; - node->base.location.end = local->end; + if (node->base.location.start == NULL) node->base.location.start = local->base.location.start; + node->base.location.end = local->base.location.end; } // Allocate and initialize a new BreakNode node. @@ -9283,7 +9299,9 @@ parse_block_parameters( do { expect(parser, YP_TOKEN_IDENTIFIER, "Expected a local variable name."); yp_parser_local_add_token(parser, &parser->previous); - yp_block_parameters_node_append_local(block_parameters, &parser->previous); + + yp_block_local_variable_node_t *local = yp_block_local_variable_node_create(parser, &parser->previous); + yp_block_parameters_node_append_local(block_parameters, local); } while (accept(parser, YP_TOKEN_COMMA)); }