Do not report regexp errors twice with captures

include/prism/internal/regexp.h

@@ -2,41 +2,25 @@
 #define PRISM_INTERNAL_REGEXP_H
 
 #include "prism/ast.h"
-#include "prism/internal/constant_pool.h"
 #include "prism/parser.h"
 
-/*
- * Accumulation state for named capture groups found during regexp parsing.
- * The caller initializes this with the call node and passes it to
- * pm_regexp_parse. The regexp parser populates match and names as groups
- * are found.
- */
-typedef struct {
-    /* The call node wrapping the regular expression node (for =~). */
-    pm_call_node_t *call;
-
-    /* The match write node being built, or NULL if no captures found yet. */
-    pm_match_write_node_t *match;
-
-    /* The set of capture names found so far (for deduplication). */
-    pm_constant_id_set_t names;
-} pm_regexp_name_data_t;
-
 /*
  * Callback invoked by pm_regexp_parse() for each named capture group found.
+ * The data pointer is whatever the caller passed in alongside the callback; the
+ * regexp parser passes it through without inspecting it.
  */
-typedef void (*pm_regexp_name_callback_t)(pm_parser_t *parser, const pm_string_t *name, bool shared, pm_regexp_name_data_t *data);
+typedef void (*pm_regexp_name_callback_t)(pm_parser_t *parser, const pm_string_t *name, void *data);
 
 /*
  * Parse a regular expression, validate its encoding, and optionally extract
  * named capture groups. Returns the encoding flags to set on the node.
  */
-PRISM_EXPORTED_FUNCTION pm_node_flags_t pm_regexp_parse(pm_parser_t *parser, pm_regular_expression_node_t *node, pm_regexp_name_callback_t name_callback, pm_regexp_name_data_t *name_data);
+PRISM_EXPORTED_FUNCTION pm_node_flags_t pm_regexp_parse(pm_parser_t *parser, pm_regular_expression_node_t *node, pm_regexp_name_callback_t name_callback, void *name_data);
 
 /*
  * Parse an interpolated regular expression for named capture groups only.
  * No encoding validation is performed.
  */
-void pm_regexp_parse_named_captures(pm_parser_t *parser, const uint8_t *source, size_t size, bool shared, bool extended_mode, pm_regexp_name_callback_t name_callback, pm_regexp_name_data_t *name_data);
+void pm_regexp_parse_named_captures(pm_parser_t *parser, const uint8_t *source, size_t size, bool extended_mode, pm_regexp_name_callback_t name_callback, void *name_data);
 
 #endif

src/prism.c

@@ -21466,14 +21466,34 @@ pm_named_capture_escape(pm_parser_t *parser, pm_buffer_t *unescaped, const uint8
     pm_buffer_append_string(unescaped, (const char *) cursor, (size_t) (end - cursor));
 }
 
+/** State for processing named capture groups found during regexp parsing. */
+typedef struct {
+    /** The call node wrapping the regular expression node (for =~). */
+    pm_call_node_t *call;
+
+    /** The match write node being built, or NULL if no captures found yet. */
+    pm_match_write_node_t *match;
+
+    /** The set of capture names found so far. */
+    pm_constant_id_set_t names;
+
+    /**
+     * Whether the content being parsed points into the original source. If it
+     * does, capture names can be located in the source directly. Otherwise
+     * they live in a separate copy and are located at the call's receiver.
+     */
+    bool shared;
+} pm_regexp_name_data_t;
+
 /**
  * This callback is called when the regular expression parser encounters a named
  * capture group.
  */
 static void
-parse_regular_expression_named_capture(pm_parser_t *parser, const pm_string_t *capture, bool shared, pm_regexp_name_data_t *callback_data) {
-    pm_call_node_t *call = callback_data->call;
-    pm_constant_id_set_t *names = &callback_data->names;
+parse_regular_expression_named_capture(pm_parser_t *parser, const pm_string_t *capture, void *data) {
+    pm_regexp_name_data_t *name_data = (pm_regexp_name_data_t *) data;
+    pm_call_node_t *call = name_data->call;
+    pm_constant_id_set_t *names = &name_data->names;
 
     const uint8_t *source = pm_string_source(capture);
     size_t length = pm_string_length(capture);
@@ -21489,7 +21509,7 @@ parse_regular_expression_named_capture(pm_parser_t *parser, const pm_string_t *c
     // unescaped, which is what we need.
     const uint8_t *cursor = pm_memchr(source, '\\', length, parser->encoding_changed, parser->encoding);
     if (PRISM_UNLIKELY(cursor != NULL)) {
-        pm_named_capture_escape(parser, &unescaped, source, length, cursor, shared ? NULL : &call->receiver->location);
+        pm_named_capture_escape(parser, &unescaped, source, length, cursor, name_data->shared ? NULL : &call->receiver->location);
 
         /*
          * Escapes like \u{} write nothing, so the unescaped name can be empty.
@@ -21513,7 +21533,7 @@ parse_regular_expression_named_capture(pm_parser_t *parser, const pm_string_t *c
         return;
     }
 
-    if (shared) {
+    if (name_data->shared) {
         // If the unescaped string is a slice of the source, then we can
         // copy the names directly. The pointers will line up.
         start = source;
@@ -21552,16 +21572,16 @@ parse_regular_expression_named_capture(pm_parser_t *parser, const pm_string_t *c
 
         // Here we lazily create the MatchWriteNode since we know we're
         // about to add a target.
-        if (callback_data->match == NULL) {
-            callback_data->match = pm_match_write_node_create(parser, call);
+        if (name_data->match == NULL) {
+            name_data->match = pm_match_write_node_create(parser, call);
         }
 
         // Next, create the local variable target and add it to the list of
         // targets for the match.
         pm_token_t token = { .type = 0, .start = start, .end = end };
         pm_location_t token_loc = TOK2LOC(parser, &token);
         pm_node_t *target = UP(pm_local_variable_target_node_create(parser, &token_loc, name, depth == -1 ? 0 : (uint32_t) depth));
-        pm_node_list_append(parser->arena, &callback_data->match->targets, target);
+        pm_node_list_append(parser->arena, &name_data->match->targets, target);
     }
 
     pm_buffer_cleanup(&unescaped);
@@ -21574,16 +21594,17 @@ parse_regular_expression_named_capture(pm_parser_t *parser, const pm_string_t *c
  */
 static pm_node_t *
 parse_interpolated_regular_expression_named_captures(pm_parser_t *parser, const pm_string_t *content, pm_call_node_t *call, bool extended_mode) {
-    pm_regexp_name_data_t callback_data = {
+    pm_regexp_name_data_t name_data = {
         .call = call,
         .match = NULL,
         .names = { 0 },
+        .shared = false,
     };
 
-    pm_regexp_parse_named_captures(parser, pm_string_source(content), pm_string_length(content), false, extended_mode, parse_regular_expression_named_capture, &callback_data);
+    pm_regexp_parse_named_captures(parser, pm_string_source(content), pm_string_length(content), extended_mode, parse_regular_expression_named_capture, &name_data);
 
-    if (callback_data.match != NULL) {
-        return UP(callback_data.match);
+    if (name_data.match != NULL) {
+        return UP(name_data.match);
     } else {
         return UP(call);
     }
@@ -22126,6 +22147,7 @@ parse_expression_infix(pm_parser_t *parser, pm_node_t *node, pm_binding_power_t
                     .call = call,
                     .match = NULL,
                     .names = { 0 },
+                    .shared = regexp->unescaped.type == PM_STRING_SHARED,
                 };
 
                 pm_node_flag_set(UP(regexp), pm_regexp_parse(parser, regexp, parse_regular_expression_named_capture, &name_data));

src/regexp.c

@@ -40,7 +40,7 @@ typedef struct {
     pm_regexp_name_callback_t name_callback;
 
     /** The data to pass to the name callback. */
-    pm_regexp_name_data_t *name_data;
+    void *name_data;
 
     /** The start of the regexp node (for error locations). */
     const uint8_t *node_start;
@@ -90,8 +90,13 @@ typedef struct {
     /** Whether the encoding has changed from the default. */
     bool encoding_changed;
 
-    /** Whether the source content is shared (for named capture callback). */
-    bool shared;
+    /**
+     * Whether syntax errors are reported. The named capture pass walks content
+     * that may be an owned copy of the source (so pointers into it have no
+     * offset in the source), and whose errors the validation pass has already
+     * reported, so it reports nothing.
+     */
+    bool report_errors;
 
     /** Whether a `\u{...}` escape with value >= 0x80 was seen. */
     bool has_unicode_escape;
@@ -122,42 +127,29 @@ typedef struct {
 } pm_regexp_parser_t;
 
 /**
- * Append a syntax error to the parser's error list. If the source is shared
- * (points into the original source), we can point to the exact error location.
- * Otherwise, we point to the whole regexp node.
+ * Append a syntax error to the parser's error list. Errors are only reported
+ * when report_errors is set, which is the case for the validation pass that
+ * walks the raw source, so start and end always point into the original source.
  */
 static PRISM_INLINE void
-pm_regexp_parse_error(pm_regexp_parser_t *parser, const uint8_t *start, const uint8_t *end, const char *message) {
-    pm_parser_t *pm = parser->parser;
-    uint32_t loc_start, loc_length;
-
-    if (parser->shared) {
-        loc_start = (uint32_t) (start - pm->start);
-        loc_length = (uint32_t) (end - start);
-    } else {
-        loc_start = (uint32_t) (parser->node_start - pm->start);
-        loc_length = (uint32_t) (parser->node_end - parser->node_start);
+pm_regexp_parse_error(pm_regexp_parser_t *regexp_parser, const uint8_t *start, const uint8_t *end, const char *message) {
+    if (regexp_parser->report_errors) {
+        pm_parser_t *parser = regexp_parser->parser;
+        pm_diagnostic_list_append_format(&parser->metadata_arena, &parser->error_list, (uint32_t) (start - parser->start), (uint32_t) (end - start), PM_ERR_REGEXP_PARSE_ERROR, message);
     }
-
-    pm_diagnostic_list_append_format(&pm->metadata_arena, &pm->error_list, loc_start, loc_length, PM_ERR_REGEXP_PARSE_ERROR, message);
 }
 
 /**
- * Append a formatted diagnostic error with proper shared/non-shared location
- * handling. This is a macro because we need variadic args for the format string.
+ * Append a formatted syntax error to the parser's error list, following the
+ * same rules as pm_regexp_parse_error. This is a macro because we need
+ * variadic args for the format string.
  */
-#define pm_regexp_parse_error_format(parser_, err_start_, err_end_, diag_id, ...) \
+#define pm_regexp_parse_error_format(regexp_parser_, err_start_, err_end_, diag_id, ...) \
     do { \
-        pm_parser_t *pm__ = (parser_)->parser; \
-        uint32_t loc_start__, loc_length__; \
-        if ((parser_)->shared) { \
-            loc_start__ = (uint32_t) ((err_start_) - pm__->start); \
-            loc_length__ = (uint32_t) ((err_end_) - (err_start_)); \
-        } else { \
-            loc_start__ = (uint32_t) ((parser_)->node_start - pm__->start); \
-            loc_length__ = (uint32_t) ((parser_)->node_end - (parser_)->node_start); \
+        if ((regexp_parser_)->report_errors) { \
+            pm_parser_t *parser_ = (regexp_parser_)->parser; \
+            pm_diagnostic_list_append_format(&parser_->metadata_arena, &parser_->error_list, (uint32_t) ((err_start_) - parser_->start), (uint32_t) ((err_end_) - (err_start_)), diag_id, __VA_ARGS__); \
         } \
-        pm_diagnostic_list_append_format(&pm__->metadata_arena, &pm__->error_list, loc_start__, loc_length__, diag_id, __VA_ARGS__); \
     } while (0)
 
 /**
@@ -168,7 +160,7 @@ static void
 pm_regexp_parser_named_capture(pm_regexp_parser_t *parser, const uint8_t *start, const uint8_t *end) {
     pm_string_t string;
     pm_string_shared_init(&string, start, end);
-    parser->name_callback(parser->parser, &string, parser->shared, parser->name_data);
+    parser->name_callback(parser->parser, &string, parser->name_data);
     pm_string_cleanup(&string);
 }
 
@@ -1607,7 +1599,7 @@ pm_regexp_validate_encoding(pm_regexp_parser_t *parser, bool ascii_only, pm_node
  * (e.g., line continuations) have already been processed by the lexer.
  */
 pm_node_flags_t
-pm_regexp_parse(pm_parser_t *parser, pm_regular_expression_node_t *node, pm_regexp_name_callback_t name_callback, pm_regexp_name_data_t *name_data) {
+pm_regexp_parse(pm_parser_t *parser, pm_regular_expression_node_t *node, pm_regexp_name_callback_t name_callback, void *name_data) {
     const uint8_t *source = parser->start + node->content_loc.start;
     size_t size = node->content_loc.length;
     bool extended_mode = PM_NODE_FLAG_P(node, PM_REGULAR_EXPRESSION_FLAGS_EXTENDED);
@@ -1627,7 +1619,7 @@ pm_regexp_parse(pm_parser_t *parser, pm_regular_expression_node_t *node, pm_rege
         .encoding = parser->encoding,
         .name_callback = NULL,
         .name_data = NULL,
-        .shared = true,
+        .report_errors = true,
         .node_start = node_start,
         .node_end = node_end,
         .has_unicode_escape = false,
@@ -1663,8 +1655,7 @@ pm_regexp_parse(pm_parser_t *parser, pm_regular_expression_node_t *node, pm_rege
 
     // Second pass: walk unescaped content for named capture extraction.
     if (name_callback != NULL) {
-        bool shared = node->unescaped.type == PM_STRING_SHARED;
-        pm_regexp_parse_named_captures(parser, pm_string_source(&node->unescaped), pm_string_length(&node->unescaped), shared, extended_mode, name_callback, name_data);
+        pm_regexp_parse_named_captures(parser, pm_string_source(&node->unescaped), pm_string_length(&node->unescaped), extended_mode, name_callback, name_data);
     }
 
     return encoding_flags;
@@ -1681,7 +1672,7 @@ pm_regexp_parse(pm_parser_t *parser, pm_regular_expression_node_t *node, pm_rege
  * them as they walk through the content.
  */
 void
-pm_regexp_parse_named_captures(pm_parser_t *parser, const uint8_t *source, size_t size, bool shared, bool extended_mode, pm_regexp_name_callback_t name_callback, pm_regexp_name_data_t *name_data) {
+pm_regexp_parse_named_captures(pm_parser_t *parser, const uint8_t *source, size_t size, bool extended_mode, pm_regexp_name_callback_t name_callback, void *name_data) {
     pm_regexp_parser_t regexp_parser = {
         .parser = parser,
         .start = source,
@@ -1692,9 +1683,7 @@ pm_regexp_parse_named_captures(pm_parser_t *parser, const uint8_t *source, size_
         .encoding = parser->encoding,
         .name_callback = name_callback,
         .name_data = name_data,
-        .shared = shared,
-        .node_start = source,
-        .node_end = source + size,
+        .report_errors = false,
         .has_unicode_escape = false,
         .has_hex_escape = false,
         .last_escape_was_unicode = false,

test/prism/errors/regexp_unmatched_parenthesis_with_escape_in_match.txt

@@ -0,0 +1,3 @@
+/(\0/ =~ a
+  ^~ end pattern with unmatched parenthesis
+