From c9b06230ec4c6cb57c0f38101ace625a778b56f9 Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Tue, 20 Oct 2026 15:03:37 -0400 Subject: [PATCH] Make strings just a view into memory We do not want strings to have the ability or the contract of having to be freed. This makes the API simpler and keeps us away from dynamic allocation. --- CHANGELOG.md | 2 ++ cpp/headers.cpp | 2 +- ext/prism/extension.c | 2 +- include/prism/internal/stringy.h | 7 +---- include/prism/options.h | 3 ++- include/prism/stringy.h | 35 ++++++------------------- rust/ruby-prism-sys/build/main.rs | 2 +- rust/ruby-prism-sys/src/bindings.rs | 32 +++++++---------------- rust/ruby-prism/src/lib.rs | 7 ++--- src/options.c | 18 ++++--------- src/prism.c | 40 +++++++++++------------------ src/regexp.c | 1 - src/stringy.c | 36 +++----------------------- 13 files changed, 52 insertions(+), 135 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cabf08cdcb..a32c82b894 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,8 @@ Note that for this major version, almost every change listed below is breaking. - You may now build side tables of comment attachments through the `attach_comments` option. - `newline_list` was renamed to `list_offset_list`. - Visitor callbacks passed to `pm_visit_node` and `pm_visit_child_nodes` now return a `pm_visit_result_t` instead of a `bool`. Returning `PM_VISIT_DESCEND` or `PM_VISIT_SKIP` replaces returning `true` or `false`, and returning `PM_VISIT_STOP` ends the walk. Both functions now return `false` if the walk was stopped. +- `pm_string_t` is now a view of bytes owned by someone else. It no longer has a `type` field, `pm_string_owned_init` was removed, and `pm_string_constant_init` was renamed to `pm_string_init`, which takes the bytes as `const uint8_t *`. +- `pm_options_free` no longer frees the names of scope locals. The parser copies them when it is created, so they only need to remain valid until then. #### Rust diff --git a/cpp/headers.cpp b/cpp/headers.cpp index 8c40bc808b..c9afcf76f1 100644 --- a/cpp/headers.cpp +++ b/cpp/headers.cpp @@ -48,7 +48,7 @@ Function functions[] = { reinterpret_cast(pm_source_constant_new), reinterpret_cast(pm_parse_stream), reinterpret_cast(pm_string_query_local), - reinterpret_cast(pm_string_constant_init), + reinterpret_cast(pm_string_init), reinterpret_cast(pm_version) }; diff --git a/ext/prism/extension.c b/ext/prism/extension.c index 1aabfd253e..78d60731a7 100644 --- a/ext/prism/extension.c +++ b/ext/prism/extension.c @@ -192,7 +192,7 @@ build_options_scopes(pm_options_t *options, VALUE scopes) { // Add the local to the scope. pm_string_t *scope_local = pm_options_scope_local_mut(options_scope, local_index); const char *name = rb_id2name(SYM2ID(local)); - pm_string_constant_init(scope_local, name, strlen(name)); + pm_string_init(scope_local, (const uint8_t *) name, strlen(name)); } // Now set the forwarding options. diff --git a/include/prism/internal/stringy.h b/include/prism/internal/stringy.h index f6716c8790..de80192d2d 100644 --- a/include/prism/internal/stringy.h +++ b/include/prism/internal/stringy.h @@ -12,7 +12,7 @@ static const uint8_t empty_source[] = ""; * Defines an empty string. This is useful for initializing a string that will * be filled in later. */ -#define PM_STRING_EMPTY ((pm_string_t) { .type = PM_STRING_CONSTANT, .source = empty_source, .length = 0 }) +#define PM_STRING_EMPTY ((pm_string_t) { .source = empty_source, .length = 0 }) /* * Initialize a shared string that is based on initial input. @@ -27,9 +27,4 @@ void pm_string_shared_init(pm_string_t *string, const uint8_t *start, const uint */ int pm_string_compare(const pm_string_t *left, const pm_string_t *right); -/* - * Free the associated memory of the given string. - */ -void pm_string_cleanup(pm_string_t *string); - #endif diff --git a/include/prism/options.h b/include/prism/options.h index 0192b557d7..1b73c7b3e8 100644 --- a/include/prism/options.h +++ b/include/prism/options.h @@ -339,7 +339,8 @@ PRISM_EXPORTED_FUNCTION const pm_string_t * pm_options_scope_local(const pm_opti /** * Return a mutable pointer to the local at the given index within the given - * scope. + * scope, for setting the local's name with pm_string_init. The parser copies + * the name when it is created, so its bytes must remain valid until then. * * @param scope The scope struct to get the local from. * @param index The index of the local to get. diff --git a/include/prism/stringy.h b/include/prism/stringy.h index 19a3986381..d132a41ffb 100644 --- a/include/prism/stringy.h +++ b/include/prism/stringy.h @@ -1,7 +1,7 @@ /** * @file stringy.h * - * A generic string type that can have various ownership semantics. + * A view of a sequence of bytes owned by someone else. */ #ifndef PRISM_STRINGY_H #define PRISM_STRINGY_H @@ -17,7 +17,8 @@ extern "C" { #endif /** - * A generic string type that can have various ownership semantics. + * A view of a sequence of bytes. The string does not own the bytes, so they + * must remain valid for as long as the string is in use. */ typedef struct { /** A pointer to the start of the string. */ @@ -25,37 +26,17 @@ typedef struct { /** The length of the string in bytes of memory. */ size_t length; - - /** The type of the string. This field determines how the string should be freed. */ - enum { - /** This string is a constant string, and should not be freed. */ - PM_STRING_CONSTANT, - - /** This is a slice of another string, and should not be freed. */ - PM_STRING_SHARED, - - /** This string owns its memory, and should be freed internally. */ - PM_STRING_OWNED - } type; } pm_string_t; /** - * Initialize a constant string that doesn't own its memory source. - * - * @param string The string to initialize. - * @param source The source of the string. - * @param length The length of the string. - */ -PRISM_EXPORTED_FUNCTION void pm_string_constant_init(pm_string_t *string, const char *source, size_t length) PRISM_NONNULL(1); - -/** - * Initialize an owned string that is responsible for freeing allocated memory. + * Initialize a string that views the given bytes. The bytes are not copied, so + * they must remain valid for as long as the string is in use. * * @param string The string to initialize. - * @param source The source of the string. - * @param length The length of the string. + * @param source The start of the bytes. + * @param length The number of bytes. */ -PRISM_EXPORTED_FUNCTION void pm_string_owned_init(pm_string_t *string, uint8_t *source, size_t length) PRISM_NONNULL(1, 2); +PRISM_EXPORTED_FUNCTION void pm_string_init(pm_string_t *string, const uint8_t *source, size_t length) PRISM_NONNULL(1); /** * Returns the length associated with the string. diff --git a/rust/ruby-prism-sys/build/main.rs b/rust/ruby-prism-sys/build/main.rs index 0081939d3e..5035228799 100644 --- a/rust/ruby-prism-sys/build/main.rs +++ b/rust/ruby-prism-sys/build/main.rs @@ -235,7 +235,7 @@ fn generate_bindings(ruby_include_path: &Path, clang_args: &[String], layout_tes .allowlist_function("pm_parser_warnings_first") .allowlist_function("pm_parser_warnings_size") .allowlist_function("pm_size_to_native") - .allowlist_function("pm_string_constant_init") + .allowlist_function("pm_string_init") .allowlist_function("pm_string_length") .allowlist_function("pm_string_query_constant") .allowlist_function("pm_string_query_keyword") diff --git a/rust/ruby-prism-sys/src/bindings.rs b/rust/ruby-prism-sys/src/bindings.rs index 357f73dbe5..ecf065b07e 100644 --- a/rust/ruby-prism-sys/src/bindings.rs +++ b/rust/ruby-prism-sys/src/bindings.rs @@ -7,9 +7,6 @@ /** A constant id is a unique identifier for a constant in the constant pool. */ pub type pm_constant_id_t = u32; -/** The type of the string. This field determines how the string should be freed. -*/ -pub type pm_string_t__bindgen_ty_1 = ::std::os::raw::c_uint; /** This is the type of node embedded in the node struct. We explicitly control the size of it here to avoid having the variable-width enum. */ @@ -2636,7 +2633,8 @@ pub struct pm_integer_t { */ pub negative: bool, } -/** A generic string type that can have various ownership semantics. +/** A view of a sequence of bytes. The string does not own the bytes, so they + must remain valid for as long as the string is in use. */ #[repr(C)] #[derive(Debug, Copy, Clone)] @@ -2647,7 +2645,6 @@ pub struct pm_string_t { /** The length of the string in bytes of memory. */ pub length: usize, - pub type_: pm_string_t__bindgen_ty_1, } /** This struct represents a slice in the source code, defined by an offset and a length. Note that we have confirmation that we can represent all locations @@ -9660,15 +9657,6 @@ pub struct pm_options_t { pub struct pm_parser_t { _unused: [u8; 0], } -/** This string is a constant string, and should not be freed. -*/ -pub const PM_STRING_CONSTANT: pm_string_t__bindgen_ty_1 = 0; -/** This is a slice of another string, and should not be freed. -*/ -pub const PM_STRING_SHARED: pm_string_t__bindgen_ty_1 = 1; -/** This string owns its memory, and should be freed internally. -*/ -pub const PM_STRING_OWNED: pm_string_t__bindgen_ty_1 = 2; /** The default value for parameters. */ pub const PM_OPTIONS_SCOPE_FORWARDING_NONE: u8 = 0; @@ -11628,17 +11616,14 @@ unsafe extern "C" { @returns The length of the constant. */ pub fn pm_constant_length(constant: *const pm_constant_t) -> usize; - /** Initialize a constant string that doesn't own its memory source. + /** Initialize a string that views the given bytes. The bytes are not copied, so + they must remain valid for as long as the string is in use. @param string The string to initialize. - @param source The source of the string. - @param length The length of the string. + @param source The start of the bytes. + @param length The number of bytes. */ - pub fn pm_string_constant_init( - string: *mut pm_string_t, - source: *const ::std::os::raw::c_char, - length: usize, - ); + pub fn pm_string_init(string: *mut pm_string_t, source: *const u8, length: usize); /** Returns the length associated with the string. @param string The string to get the length of. @@ -11886,7 +11871,8 @@ unsafe extern "C" { */ pub fn pm_options_scope_init(scope: *mut pm_options_scope_t, locals_count: usize); /** Return a mutable pointer to the local at the given index within the given - scope. + scope, for setting the local's name with pm_string_init. The parser copies + the name when it is created, so its bytes must remain valid until then. @param scope The scope struct to get the local from. @param index The index of the local to get. diff --git a/rust/ruby-prism/src/lib.rs b/rust/ruby-prism/src/lib.rs index 6106009441..1b3213a9de 100644 --- a/rust/ruby-prism/src/lib.rs +++ b/rust/ruby-prism/src/lib.rs @@ -31,7 +31,7 @@ pub use self::string_query::{InvalidEncodingError, StringQuery}; use ruby_prism_sys::{ pm_arena_new, pm_options_command_line_set, pm_options_encoding_locked_set, pm_options_encoding_set, pm_options_filepath_set, pm_options_free, pm_options_frozen_string_literal_set, pm_options_line_set, pm_options_main_script_set, pm_options_new, pm_options_partial_script_set, - pm_options_scope_forwarding_set, pm_options_scope_init, pm_options_scope_local_mut, pm_options_scope_mut, pm_options_scopes_init, pm_options_t, pm_options_version_set, pm_parse, pm_parser_new, pm_string_constant_init, + pm_options_scope_forwarding_set, pm_options_scope_init, pm_options_scope_local_mut, pm_options_scope_mut, pm_options_scopes_init, pm_options_t, pm_options_version_set, pm_parse, pm_parser_new, pm_string_init, }; /// The version of Ruby syntax to parse with. @@ -305,7 +305,7 @@ impl Options { for (local_index, local) in scope.locals.iter().enumerate() { let pm_local = unsafe { pm_options_scope_local_mut(pm_scope, local_index) }; - unsafe { pm_string_constant_init(pm_local, local.as_ptr().cast(), local.len()) }; + unsafe { pm_string_init(pm_local, local.as_ptr(), local.len()) }; } let forwarding = scope.forwarding.iter().fold(0u8, |acc, &flag| acc | u8::from(flag)); @@ -328,7 +328,8 @@ impl Options { /// any [`ParseResult`] created with [`parse_with_options`]. pub struct ParseOptions { options: *mut pm_options_t, - // These CStrings back the constant pm_string_t values inside `options`. + // These CStrings back the pm_string_t values inside `options`, which only + // view their bytes. // They must not be dropped before `options` is freed. _filepath: Option, _encoding: Option, diff --git a/src/options.c b/src/options.c index c55d8e97b5..09c6db8d8f 100644 --- a/src/options.c +++ b/src/options.c @@ -27,16 +27,8 @@ pm_options_new(void) { */ void pm_options_cleanup(pm_options_t *options) { - pm_string_cleanup(&options->filepath); - pm_string_cleanup(&options->encoding); - for (size_t scope_index = 0; scope_index < options->scopes_count; scope_index++) { pm_options_scope_t *scope = &options->scopes[scope_index]; - - for (size_t local_index = 0; local_index < scope->locals_count; local_index++) { - pm_string_cleanup(&scope->locals[local_index]); - } - xfree_sized(scope->locals, scope->locals_count * sizeof(pm_string_t)); } @@ -76,7 +68,7 @@ pm_options_filepath(const pm_options_t *options) { */ void pm_options_filepath_set(pm_options_t *options, const char *filepath) { - pm_string_constant_init(&options->filepath, filepath, strlen(filepath)); + pm_string_init(&options->filepath, (const uint8_t *) filepath, strlen(filepath)); } /** @@ -84,7 +76,7 @@ pm_options_filepath_set(pm_options_t *options, const char *filepath) { */ void pm_options_encoding_set(pm_options_t *options, const char *encoding) { - pm_string_constant_init(&options->encoding, encoding, strlen(encoding)); + pm_string_init(&options->encoding, (const uint8_t *) encoding, strlen(encoding)); } /** @@ -397,7 +389,7 @@ pm_options_read(pm_options_t *options, const char *data) { data += 4; if (filepath_length > 0) { - pm_string_constant_init(&options->filepath, data, filepath_length); + pm_string_init(&options->filepath, (const uint8_t *) data, filepath_length); data += filepath_length; } @@ -408,7 +400,7 @@ pm_options_read(pm_options_t *options, const char *data) { data += 4; if (encoding_length > 0) { - pm_string_constant_init(&options->encoding, data, encoding_length); + pm_string_init(&options->encoding, (const uint8_t *) data, encoding_length); data += encoding_length; } @@ -441,7 +433,7 @@ pm_options_read(pm_options_t *options, const char *data) { uint32_t local_length = pm_options_read_u32(data); data += 4; - pm_string_constant_init(&scope->locals[local_index], data, local_length); + pm_string_init(&scope->locals[local_index], (const uint8_t *) data, local_length); data += local_length; } } diff --git a/src/prism.c b/src/prism.c index c58e6a330f..01ef1b4345 100644 --- a/src/prism.c +++ b/src/prism.c @@ -6820,7 +6820,7 @@ pm_symbol_node_synthesized_create(pm_parser_t *parser, const char *content) { ((pm_string_t) { 0 }) ); - pm_string_constant_init(&node->unescaped, content, strlen(content)); + pm_string_init(&node->unescaped, (const uint8_t *) content, strlen(content)); return node; } @@ -9658,7 +9658,7 @@ lex_question_mark(pm_parser_t *parser) { // Copy buffer data into the arena and free the heap buffer. void *arena_data = pm_arena_memdup(parser->arena, buffer.value, buffer.length, PRISM_ALIGNOF(uint8_t)); - pm_string_constant_init(&parser->current_string, (const char *) arena_data, buffer.length); + pm_string_init(&parser->current_string, (const uint8_t *) arena_data, buffer.length); pm_buffer_cleanup(&buffer); return PM_TOKEN_CHARACTER_LITERAL; @@ -9982,7 +9982,7 @@ pm_token_buffer_copy(pm_parser_t *parser, pm_token_buffer_t *token_buffer) { // Copy buffer data into the arena and free the heap buffer. size_t len = pm_buffer_length(&token_buffer->buffer); void *arena_data = pm_arena_memdup(parser->arena, pm_buffer_value(&token_buffer->buffer), len, PRISM_ALIGNOF(uint8_t)); - pm_string_constant_init(&parser->current_string, (const char *) arena_data, len); + pm_string_init(&parser->current_string, (const uint8_t *) arena_data, len); pm_buffer_cleanup(&token_buffer->buffer); } @@ -16798,18 +16798,13 @@ parse_method_definition_name(pm_parser_t *parser) { static void parse_heredoc_dedent_string(pm_arena_t *arena, pm_string_t *string, size_t common_whitespace) { - // Make a writable copy in the arena if the string isn't already writable. - // We keep a mutable pointer to the arena memory so we can memmove into it - // below without casting away const from the string's source field. - uint8_t *writable; - - if (string->type != PM_STRING_OWNED) { - size_t length = pm_string_length(string); - writable = (uint8_t *) pm_arena_memdup(arena, pm_string_source(string), length, PRISM_ALIGNOF(uint8_t)); - pm_string_constant_init(string, (const char *) writable, length); - } else { - writable = (uint8_t *) string->source; - } + // Make a writable copy of the string in the arena, since strings only view + // their bytes. We keep a mutable pointer to the arena memory so we can + // memmove into it below without casting away const from the string's + // source field. + size_t length = pm_string_length(string); + uint8_t *writable = (uint8_t *) pm_arena_memdup(arena, pm_string_source(string), length, PRISM_ALIGNOF(uint8_t)); + pm_string_init(string, writable, length); // Now get the bounds of the existing string. We'll use this as a // destination to move bytes into. We'll also use it for bounds checking @@ -22022,9 +22017,7 @@ parse_match(pm_parser_t *parser, pm_node_t *node, pm_binding_power_t binding_pow } if (!interpolated && total_length > 0) { - void *memory = xmalloc(total_length); - if (!memory) abort(); - + uint8_t *memory = (uint8_t *) pm_arena_alloc(&parser->metadata_arena, total_length, PRISM_ALIGNOF(uint8_t)); uint8_t *cursor = memory; PM_NODE_LIST_FOREACH(parts, index, part) { pm_string_t *unescaped = &((pm_string_node_t *) part)->unescaped; @@ -22034,11 +22027,9 @@ parse_match(pm_parser_t *parser, pm_node_t *node, pm_binding_power_t binding_pow cursor += length; } - pm_string_t owned; - pm_string_owned_init(&owned, (uint8_t *) memory, total_length); - - result = parse_interpolated_regular_expression_named_captures(parser, &owned, call, PM_NODE_FLAG_P(node, PM_REGULAR_EXPRESSION_FLAGS_EXTENDED)); - pm_string_cleanup(&owned); + pm_string_t content; + pm_string_init(&content, memory, total_length); + result = parse_interpolated_regular_expression_named_captures(parser, &content, call, PM_NODE_FLAG_P(node, PM_REGULAR_EXPRESSION_FLAGS_EXTENDED)); } } else if (PM_NODE_TYPE_P(node, PM_REGULAR_EXPRESSION_NODE)) { /* If we have a regular expression node, then we can parse the named @@ -22048,7 +22039,7 @@ parse_match(pm_parser_t *parser, pm_node_t *node, pm_binding_power_t binding_pow .call = call, .match = NULL, .names = { 0 }, - .shared = regexp->unescaped.type == PM_STRING_SHARED, + .shared = pm_string_source(®exp->unescaped) == parser->start + regexp->content_loc.start, }; pm_node_flag_set(UP(regexp), pm_regexp_parse(parser, regexp, parse_regular_expression_named_capture, &name_data)); @@ -23574,7 +23565,6 @@ pm_parser_new(pm_arena_t *arena, const uint8_t *source, size_t size, const pm_op */ void pm_parser_cleanup(pm_parser_t *parser) { - pm_string_cleanup(&parser->filepath); pm_arena_cleanup(&parser->metadata_arena); while (parser->lex_modes.index >= PM_LEX_STACK_SIZE) { diff --git a/src/regexp.c b/src/regexp.c index e6e060b224..6a1c19b985 100644 --- a/src/regexp.c +++ b/src/regexp.c @@ -161,7 +161,6 @@ pm_regexp_parser_named_capture(pm_regexp_parser_t *parser, const uint8_t *start, pm_string_t string; pm_string_shared_init(&string, start, end); parser->name_callback(parser->parser, &string, parser->name_data); - pm_string_cleanup(&string); } /** diff --git a/src/stringy.c b/src/stringy.c index d6f4c4a777..75463447ee 100644 --- a/src/stringy.c +++ b/src/stringy.c @@ -1,9 +1,6 @@ #include "prism/internal/stringy.h" -#include "prism/internal/allocator.h" - #include -#include #include /** @@ -14,34 +11,17 @@ pm_string_shared_init(pm_string_t *string, const uint8_t *start, const uint8_t * assert(start <= end); *string = (pm_string_t) { - .type = PM_STRING_SHARED, .source = start, .length = (size_t) (end - start) }; } /** - * Initialize an owned string that is responsible for freeing allocated memory. - */ -void -pm_string_owned_init(pm_string_t *string, uint8_t *source, size_t length) { - *string = (pm_string_t) { - .type = PM_STRING_OWNED, - .source = source, - .length = length - }; -} - -/** - * Initialize a constant string that doesn't own its memory source. + * Initialize a string that views the given bytes. */ void -pm_string_constant_init(pm_string_t *string, const char *source, size_t length) { - *string = (pm_string_t) { - .type = PM_STRING_CONSTANT, - .source = (const uint8_t *) source, - .length = length - }; +pm_string_init(pm_string_t *string, const uint8_t *source, size_t length) { + *string = (pm_string_t) { .source = source, .length = length }; } /** @@ -79,13 +59,3 @@ const uint8_t * pm_string_source(const pm_string_t *string) { return string->source; } - -/** - * Free the associated memory of the given string. - */ -void -pm_string_cleanup(pm_string_t *string) { - if (string->type == PM_STRING_OWNED) { - xfree_sized((void *) string->source, string->length); - } -}