[ruby/yarp] Disallow numbered parameters in multiple scopes

https://github.com/ruby/yarp/commit/5fd4d3b89a

5 files changed, 56 insertions, 4 deletions

diff --git a/test/yarp/errors_test.rb b/test/yarp/errors_test.rb
index 7003dceb38..89fafdf28d 100644
--- a/test/yarp/errors_test.rb
+++ b/test/yarp/errors_test.rb
@@ -1206,11 +1206,18 @@ module YARP
       ]
     end
 
+    def test_double_scope_numbered_parameters
+      source = "-> { _1 + -> { _2 } }"
+      errors = [["Numbered parameter is already used in outer scope", 15..17]]
+
+      assert_errors expression(source), source, errors, compare_ripper: false
+    end
+
     private
 
-    def assert_errors(expected, source, errors)
+    def assert_errors(expected, source, errors, compare_ripper: RUBY_ENGINE == "ruby")
       # Ripper behaves differently on JRuby/TruffleRuby, so only check this on CRuby
-      assert_nil Ripper.sexp_raw(source) if RUBY_ENGINE == "ruby"
+      assert_nil Ripper.sexp_raw(source) if compare_ripper
 
       result = YARP.parse(source)
       node = result.value.statements.body.last
diff --git a/yarp/diagnostic.c b/yarp/diagnostic.c
index 55e16e6c10..4611e8e6d6 100644
--- a/yarp/diagnostic.c
+++ b/yarp/diagnostic.c
@@ -193,6 +193,7 @@ static const char* const diagnostic_messages[YP_DIAGNOSTIC_ID_LEN] = {
     [YP_ERR_PARAMETER_NO_DEFAULT]               = "Expected a default value for the parameter",
     [YP_ERR_PARAMETER_NO_DEFAULT_KW]            = "Expected a default value for the keyword parameter",
     [YP_ERR_PARAMETER_NUMBERED_RESERVED]        = "Token reserved for a numbered parameter",
+    [YP_ERR_NUMBERED_PARAMETER_OUTER_SCOPE]     = "Numbered parameter is already used in outer scope",
     [YP_ERR_PARAMETER_ORDER]                    = "Unexpected parameter order",
     [YP_ERR_PARAMETER_SPLAT_MULTI]              = "Unexpected multiple `*` splat parameters",
     [YP_ERR_PARAMETER_STAR]                     = "Unexpected parameter `*`",
diff --git a/yarp/diagnostic.h b/yarp/diagnostic.h
index 07b5790072..6f17a23dd1 100644
--- a/yarp/diagnostic.h
+++ b/yarp/diagnostic.h
@@ -151,6 +151,7 @@ typedef enum {
     YP_ERR_NOT_EXPRESSION,
     YP_ERR_NUMBER_LITERAL_UNDERSCORE,
     YP_ERR_NUMBERED_PARAMETER_NOT_ALLOWED,
+    YP_ERR_NUMBERED_PARAMETER_OUTER_SCOPE,
     YP_ERR_OPERATOR_MULTI_ASSIGN,
     YP_ERR_OPERATOR_WRITE_BLOCK,
     YP_ERR_PARAMETER_ASSOC_SPLAT_MULTI,
diff --git a/yarp/parser.h b/yarp/parser.h
index 610521041e..89b0f2744b 100644
--- a/yarp/parser.h
+++ b/yarp/parser.h
@@ -288,6 +288,11 @@ typedef struct yp_scope {
     // This is necessary to determine whether or not numbered parameters are
     // allowed.
     bool explicit_params;
+
+    // A boolean indicating whether or not this scope has numbered parameters.
+    // This is necessary to determine if child blocks are allowed to use
+    // numbered parameters.
+    bool numbered_params;
 } yp_scope_t;
 
 // This struct represents the overall parser. It contains a reference to the
diff --git a/yarp/yarp.c b/yarp/yarp.c
index d9600f0c8f..242b03e2ed 100644
--- a/yarp/yarp.c
+++ b/yarp/yarp.c
@@ -4720,10 +4720,16 @@ yp_parser_scope_push(yp_parser_t *parser, bool closed) {
     yp_scope_t *scope = (yp_scope_t *) malloc(sizeof(yp_scope_t));
     if (scope == NULL) return false;
 
-    *scope = (yp_scope_t) { .previous = parser->current_scope, .closed = closed, .explicit_params = false };
-    yp_constant_id_list_init(&scope->locals);
+    *scope = (yp_scope_t) {
+        .previous = parser->current_scope,
+        .closed = closed,
+        .explicit_params = false,
+        .numbered_params = false
+    };
 
+    yp_constant_id_list_init(&scope->locals);
     parser->current_scope = scope;
+
     return true;
 }
 
@@ -10109,6 +10115,24 @@ parse_alias_argument(yp_parser_t *parser, bool first) {
     }
 }
 
+// Return true if any of the visible scopes to the current context are using
+// numbered parameters.
+static bool
+outer_scope_using_numbered_params_p(yp_parser_t *parser) {
+    yp_scope_t *scope = parser->current_scope;
+
+    // We can bail early here if we've already performed this lookup since it
+    // will be set on the current scope.
+    if (scope->numbered_params) return false;
+
+    // Otherwise we need to walk up the list of scopes.
+    for (scope = scope->previous; scope != NULL && !scope->closed; scope = scope->previous) {
+        if (scope->numbered_params) return true;
+    }
+
+    return false;
+}
+
 // Parse an identifier into either a local variable read or a call.
 static yp_node_t *
 parse_variable_call(yp_parser_t *parser) {
@@ -10127,7 +10151,18 @@ parse_variable_call(yp_parser_t *parser) {
             // node but add an error.
             if (parser->current_scope->explicit_params) {
                 yp_diagnostic_list_append(&parser->error_list, parser->previous.start, parser->previous.end, YP_ERR_NUMBERED_PARAMETER_NOT_ALLOWED);
+            } else if (outer_scope_using_numbered_params_p(parser)) {
+                yp_diagnostic_list_append(&parser->error_list, parser->previous.start, parser->previous.end, YP_ERR_NUMBERED_PARAMETER_OUTER_SCOPE);
             } else {
+                // Indicate that this scope is using numbered params so that
+                // child scopes cannot.
+                parser->current_scope->numbered_params = true;
+
+                // When you use a numbered parameter, it implies the existence
+                // of all of the locals that exist before it. For example,
+                // referencing _2 means that _1 must exist. Therefore here we
+                // loop through all of the possibilities and add them into the
+                // constant pool.
                 uint8_t number = parser->previous.start[1];
                 uint8_t current = '1';
                 uint8_t *value;
@@ -10139,6 +10174,9 @@ parse_variable_call(yp_parser_t *parser) {
                     yp_parser_local_add_owned(parser, value, 2);
                 }
 
+                // Now we can add the actual token that is being used. For
+                // this one we can add a shared version since it is directly
+                // referenced in the source.
                 yp_parser_local_add_token(parser, &parser->previous);
                 return (yp_node_t *) yp_local_variable_read_node_create(parser, &parser->previous, 0);
             }