From 3a7f3cb6d5612f488b7ef1ff224b1282a66178c5 Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Tue, 20 Oct 2026 12:28:58 -0400 Subject: [PATCH] Add index and at APIs for constants So that consumers don't unintentionally encode the fact that a constant id is actually 1 + its index in the constants table. Ideally this makes it so that we can change the underlying implementation if we want to. --- include/prism/constant_pool.h | 17 ++++++++++ include/prism/parser.h | 40 ++++++++++++++---------- rust/ruby-prism-sys/build/main.rs | 2 +- rust/ruby-prism-sys/src/bindings.rs | 48 ++++++++++++++++------------- src/parser.c | 15 +++++---- templates/ext/prism/api_node.c.erb | 32 +++++++++---------- 6 files changed, 91 insertions(+), 63 deletions(-) diff --git a/include/prism/constant_pool.h b/include/prism/constant_pool.h index 8244c0d747..61221ca9b3 100644 --- a/include/prism/constant_pool.h +++ b/include/prism/constant_pool.h @@ -11,6 +11,7 @@ #define PRISM_CONSTANT_POOL_H #include "prism/compiler/exported.h" +#include "prism/compiler/inline.h" #include "prism/compiler/nodiscard.h" #include "prism/compiler/nonnull.h" @@ -28,6 +29,22 @@ extern "C" { */ typedef uint32_t pm_constant_id_t; +/** + * Returns the index of the constant with the given id among the constants of + * the parser that produced it. The indices run from 0 to one less than + * pm_parser_constants_size, so they can index a cache with one entry per + * constant that was filled using pm_parser_constant_at. + * + * @param constant_id the id of the constant. Optional constant fields on nodes + * hold 0 when they are absent, which is not a valid id, so check for it + * before calling this function. + * @returns the index of the constant + */ +static PRISM_INLINE size_t +pm_constant_id_index(pm_constant_id_t constant_id) { + return (size_t) constant_id - 1; +} + /** * A list of constant IDs. Usually used to represent a set of locals. */ diff --git a/include/prism/parser.h b/include/prism/parser.h index 0a05776983..4644204d84 100644 --- a/include/prism/parser.h +++ b/include/prism/parser.h @@ -364,30 +364,38 @@ PRISM_EXPORTED_FUNCTION const pm_diagnostic_t * pm_parser_warnings_first(const p PRISM_EXPORTED_FUNCTION size_t pm_parser_constants_size(const pm_parser_t *parser) PRISM_NONNULL(1); /** - * A callback function that can be used to process constants found while - * parsing. - */ -typedef void (*pm_constant_callback_t)(const pm_constant_t *constant, void *data); - -/** - * Iterates over the constants in the constant pool associated with the given - * parser and calls the given callback for each constant. + * Returns the constant at the given index in the constant pool associated with + * the given parser. Together with pm_constant_id_index, this lets a caller + * build a cache with one entry per constant and then find the entry for any + * constant id, as in: + * + * ```c + * size_t size = pm_parser_constants_size(parser); + * for (size_t index = 0; index < size; index++) { + * cache[index] = convert(pm_parser_constant_at(parser, index)); + * } + * + * value = cache[pm_constant_id_index(constant_id)]; + * ``` * - * @param parser the parser whose constants we want to iterate over - * @param callback the callback function to call for each constant. This function - * will be passed a pointer to the constant and the data parameter passed to - * this function. - * @param data the data to pass to the callback function for each constant. This - * can be NULL if no data needs to be passed to the callback function. + * For any constant id, pm_parser_constant_at(parser, + * pm_constant_id_index(constant_id)) is pm_parser_constant(parser, + * constant_id). + * + * @param parser the parser whose constant pool we want to look up from + * @param index the index of the constant, which must be less than + * pm_parser_constants_size(parser) + * @returns a pointer to the constant at the given index */ -PRISM_EXPORTED_FUNCTION void pm_parser_constants_each(const pm_parser_t *parser, pm_constant_callback_t callback, void *data) PRISM_NONNULL(1); +PRISM_EXPORTED_FUNCTION const pm_constant_t * pm_parser_constant_at(const pm_parser_t *parser, size_t index) PRISM_NONNULL(1); /** * Returns a pointer to the constant at the given id in the constant pool * associated with the given parser. * * @param parser the parser whose constant pool we want to look up from - * @param constant_id the id of the constant to look up (1-based) + * @param constant_id the id of the constant to look up. Optional constant + * fields on nodes hold 0 when they are absent, which is not a valid id. * @returns a pointer to the constant at the given id */ PRISM_EXPORTED_FUNCTION const pm_constant_t * pm_parser_constant(const pm_parser_t *parser, pm_constant_id_t constant_id) PRISM_NONNULL(1); diff --git a/rust/ruby-prism-sys/build/main.rs b/rust/ruby-prism-sys/build/main.rs index acd4b14244..0081939d3e 100644 --- a/rust/ruby-prism-sys/build/main.rs +++ b/rust/ruby-prism-sys/build/main.rs @@ -219,7 +219,7 @@ fn generate_bindings(ruby_include_path: &Path, clang_args: &[String], layout_tes .allowlist_function("pm_parser_comments_first") .allowlist_function("pm_parser_comments_size") .allowlist_function("pm_parser_constant") - .allowlist_function("pm_parser_constants_each") + .allowlist_function("pm_parser_constant_at") .allowlist_function("pm_parser_constants_size") .allowlist_function("pm_parser_data_loc") .allowlist_function("pm_parser_errors_first") diff --git a/rust/ruby-prism-sys/src/bindings.rs b/rust/ruby-prism-sys/src/bindings.rs index c05e1e2525..357f73dbe5 100644 --- a/rust/ruby-prism-sys/src/bindings.rs +++ b/rust/ruby-prism-sys/src/bindings.rs @@ -2583,15 +2583,6 @@ pub type pm_x_string_node_t = pm_x_string_node; @extends pm_node_t */ pub type pm_yield_node_t = pm_yield_node; -/** A callback function that can be used to process constants found while - parsing. -*/ -pub type pm_constant_callback_t = ::std::option::Option< - unsafe extern "C" fn( - constant: *const pm_constant_t, - data: *mut ::std::os::raw::c_void, - ), ->; /** Represents the results of a slice query. */ pub type pm_string_query_t = ::std::os::raw::c_int; @@ -12042,26 +12033,39 @@ unsafe extern "C" { given parser */ pub fn pm_parser_constants_size(parser: *const pm_parser_t) -> usize; - /** Iterates over the constants in the constant pool associated with the given - parser and calls the given callback for each constant. + /** Returns the constant at the given index in the constant pool associated with + the given parser. Together with pm_constant_id_index, this lets a caller + build a cache with one entry per constant and then find the entry for any + constant id, as in: - @param parser the parser whose constants we want to iterate over - @param callback the callback function to call for each constant. This function - will be passed a pointer to the constant and the data parameter passed to - this function. - @param data the data to pass to the callback function for each constant. This - can be NULL if no data needs to be passed to the callback function. + ```c + size_t size = pm_parser_constants_size(parser); + for (size_t index = 0; index < size; index++) { + cache[index] = convert(pm_parser_constant_at(parser, index)); + } + + value = cache[pm_constant_id_index(constant_id)]; + ``` + + For any constant id, pm_parser_constant_at(parser, + pm_constant_id_index(constant_id)) is pm_parser_constant(parser, + constant_id). + + @param parser the parser whose constant pool we want to look up from + @param index the index of the constant, which must be less than + pm_parser_constants_size(parser) + @returns a pointer to the constant at the given index */ - pub fn pm_parser_constants_each( + pub fn pm_parser_constant_at( parser: *const pm_parser_t, - callback: pm_constant_callback_t, - data: *mut ::std::os::raw::c_void, - ); + index: usize, + ) -> *const pm_constant_t; /** Returns a pointer to the constant at the given id in the constant pool associated with the given parser. @param parser the parser whose constant pool we want to look up from - @param constant_id the id of the constant to look up (1-based) + @param constant_id the id of the constant to look up. Optional constant + fields on nodes hold 0 when they are absent, which is not a valid id. @returns a pointer to the constant at the given id */ pub fn pm_parser_constant( diff --git a/src/parser.c b/src/parser.c index b9103d7c25..1b3cce1df8 100644 --- a/src/parser.c +++ b/src/parser.c @@ -7,6 +7,7 @@ #include "prism/internal/magic_comments.h" #include "prism/internal/xxh3.h" +#include #include #include @@ -338,15 +339,13 @@ pm_parser_constants_size(const pm_parser_t *parser) { } /** - * Iterates over the constants in the constant pool associated with the given - * parser and calls the given callback for each constant. + * Returns the constant at the given index in the constant pool associated with + * the given parser. */ -void -pm_parser_constants_each(const pm_parser_t *parser, pm_constant_callback_t callback, void *data) { - for (uint32_t index = 0; index < parser->constant_pool.size; index++) { - const pm_constant_t *constant = &parser->constant_pool.constants[index]; - callback(constant, data); - } +const pm_constant_t * +pm_parser_constant_at(const pm_parser_t *parser, size_t index) { + assert(index < parser->constant_pool.size); + return &parser->constant_pool.constants[index]; } /** diff --git a/templates/ext/prism/api_node.c.erb b/templates/ext/prism/api_node.c.erb index 37338724eb..522ca82023 100644 --- a/templates/ext/prism/api_node.c.erb +++ b/templates/ext/prism/api_node.c.erb @@ -127,17 +127,14 @@ pm_node_stack_pop(pm_node_stack_node_t **stack) { return visit; } -typedef struct { - VALUE constants; - rb_encoding *encoding; -} pm_ast_constants_each_data_t; - -static void -pm_ast_constants_each(const pm_constant_t *constant, void *data) { - pm_ast_constants_each_data_t *constants_data = (pm_ast_constants_each_data_t *) data; +/** + * Build the Symbol for the given constant. + */ +static VALUE +pm_ast_constant(const pm_constant_t *constant, rb_encoding *encoding) { int state = 0; - VALUE string = rb_enc_str_new((const char *) pm_constant_start(constant), pm_constant_length(constant), constants_data->encoding); + VALUE string = rb_enc_str_new((const char *) pm_constant_start(constant), pm_constant_length(constant), encoding); VALUE value = rb_protect(rb_str_intern, string, &state); if (state != 0) { @@ -145,14 +142,17 @@ pm_ast_constants_each(const pm_constant_t *constant, void *data) { rb_set_errinfo(Qnil); } - rb_ary_push(constants_data->constants, value); + return value; } VALUE pm_ast_new(const pm_parser_t *parser, const pm_node_t *node, rb_encoding *encoding, VALUE source, bool freeze) { - VALUE constants = rb_ary_new_capa(pm_parser_constants_size(parser)); - pm_ast_constants_each_data_t constants_data = { .constants = constants, .encoding = encoding }; - pm_parser_constants_each(parser, pm_ast_constants_each, &constants_data); + size_t constants_size = pm_parser_constants_size(parser); + VALUE constants = rb_ary_new_capa((long) constants_size); + + for (size_t index = 0; index < constants_size; index++) { + rb_ary_push(constants, pm_ast_constant(pm_parser_constant_at(parser, index), encoding)); + } pm_arena_t *node_arena = pm_arena_new(); pm_node_stack_node_t *node_stack = NULL; @@ -237,15 +237,15 @@ pm_ast_new(const pm_parser_t *parser, const pm_node_t *node, rb_encoding *encodi <%- when Prism::Template::ConstantField -%> #line <%= __LINE__ + 1 %> "prism/templates/ext/prism/<%= File.basename(__FILE__) %>" assert(cast-><%= field.name %> != 0); - argv[<%= index %>] = RARRAY_AREF(constants, cast-><%= field.name %> - 1); + argv[<%= index %>] = RARRAY_AREF(constants, pm_constant_id_index(cast-><%= field.name %>)); <%- when Prism::Template::OptionalConstantField -%> - argv[<%= index %>] = cast-><%= field.name %> == 0 ? Qnil : RARRAY_AREF(constants, cast-><%= field.name %> - 1); + argv[<%= index %>] = cast-><%= field.name %> == 0 ? Qnil : RARRAY_AREF(constants, pm_constant_id_index(cast-><%= field.name %>)); <%- when Prism::Template::ConstantListField -%> #line <%= __LINE__ + 1 %> "prism/templates/ext/prism/<%= File.basename(__FILE__) %>" argv[<%= index %>] = rb_ary_new_capa(cast-><%= field.name %>.size); for (size_t index = 0; index < cast-><%= field.name %>.size; index++) { assert(cast-><%= field.name %>.ids[index] != 0); - rb_ary_push(argv[<%= index %>], RARRAY_AREF(constants, cast-><%= field.name %>.ids[index] - 1)); + rb_ary_push(argv[<%= index %>], RARRAY_AREF(constants, pm_constant_id_index(cast-><%= field.name %>.ids[index]))); } if (freeze) rb_obj_freeze(argv[<%= index %>]); <%- when Prism::Template::LocationField -%>