Reject endless method as a block parameter default

[Earlopain]

Fixes [Bug #21661]

It's not particularly nice to me to now have 3 bool parameters for expressions. But I guess this follows b624e09

[Earlopain]

This is it basically. Pass this all the way down to parse_expression_prefix. Every other place I have left at the previous behaviour (accept endless method)

[Earlopain]

The other place of interest

[Earlopain]

I would say these error messages look kind of ugly. The second one (unexpected =) is telling, the rest are meh. The last two code snippets are already rejected, the error message didn't change for them.

I would not know how to improve these error messages, if that is necessary.

[kddnewton]

This one is a bit bigger, so will take a minute to review. I'm out this week but will get back to stuff in earnest next week.

[tenderlove]

Seems good to me. Most of this patch is just updating function signatures.

[kddnewton]

Could you potentially do this with a new context instead? We already have PM_CONTEXT_DEFAULT_PARAMS, and there are a couple of other places in the file where we already check the context (like context_def_p). If we split up PM_CONTEXT_DEFAULT_PARAMS to be DEF_DEFAULT/BLOCK_DEFAULT, I think we could simplify this without adding an additional boolean parameter.

[Earlopain]

Should be possible, yeah. I did start out with a new context but there are so few places that actually use them in such a way and that did turn me away from that. Will try to rework this

[Earlopain]

I have this patch:

Patch

diff --git a/config.yml b/config.yml
index 7d71d52d..2ce8babf 100644
--- a/config.yml
+++ b/config.yml
@@ -3720,6 +3720,7 @@ nodes:
           - RequiredParameterNode # def m((a,b)); end
           - on error: BackReferenceReadNode # a, (b, $&) = z
           - on error: NumberedReferenceReadNode # a, (b, $1) = z
+          - on error: CallNode # p do |a = def f = 1, b| end
         comment: |
           Represents the targets expressions before a splat node.
 
diff --git a/include/prism/parser.h b/include/prism/parser.h
index 992729d6..d378ff0c 100644
--- a/include/prism/parser.h
+++ b/include/prism/parser.h
@@ -290,6 +290,9 @@ typedef enum {
     /** expressions in block arguments using braces */
     PM_CONTEXT_BLOCK_BRACES,
 
+    /** expressions in block parameters `foo do |...| end ` */
+    PM_CONTEXT_BLOCK_PARAMETERS,
+
     /** expressions in block arguments using do..end */
     PM_CONTEXT_BLOCK_KEYWORDS,
 
diff --git a/src/prism.c b/src/prism.c
index 02dd2f11..23460610 100644
--- a/src/prism.c
+++ b/src/prism.c
@@ -8603,6 +8603,7 @@ static const uint32_t context_terminators[] = {
     [PM_CONTEXT_BEGIN_ELSE] = (1U << PM_TOKEN_KEYWORD_ENSURE) | (1U << PM_TOKEN_KEYWORD_END),
     [PM_CONTEXT_BEGIN_RESCUE] = (1U << PM_TOKEN_KEYWORD_ENSURE) | (1U << PM_TOKEN_KEYWORD_RESCUE) | (1U << PM_TOKEN_KEYWORD_ELSE) | (1U << PM_TOKEN_KEYWORD_END),
     [PM_CONTEXT_BLOCK_BRACES] = (1U << PM_TOKEN_BRACE_RIGHT),
+    [PM_CONTEXT_BLOCK_PARAMETERS] = 0,
     [PM_CONTEXT_BLOCK_KEYWORDS] = (1U << PM_TOKEN_KEYWORD_END) | (1U << PM_TOKEN_KEYWORD_RESCUE) | (1U << PM_TOKEN_KEYWORD_ENSURE),
     [PM_CONTEXT_BLOCK_ENSURE] = (1U << PM_TOKEN_KEYWORD_END),
     [PM_CONTEXT_BLOCK_ELSE] = (1U << PM_TOKEN_KEYWORD_ENSURE) | (1U << PM_TOKEN_KEYWORD_END),
@@ -8754,6 +8755,7 @@ context_human(pm_context_t context) {
             assert(false && "unreachable");
             return "";
         case PM_CONTEXT_BEGIN: return "begin statement";
+        case PM_CONTEXT_BLOCK_PARAMETERS: return "'|'..'|' block parameter";
         case PM_CONTEXT_BLOCK_BRACES: return "'{'..'}' block";
         case PM_CONTEXT_BLOCK_KEYWORDS: return "'do'..'end' block";
         case PM_CONTEXT_CASE_WHEN: return "'when' clause";
@@ -15357,6 +15359,9 @@ parse_block_parameters(
 ) {
     pm_parameters_node_t *parameters = NULL;
     if (!match1(parser, PM_TOKEN_SEMICOLON)) {
+        if (!is_lambda_literal) {
+            context_push(parser, PM_CONTEXT_BLOCK_PARAMETERS);
+        }
         parameters = parse_parameters(
             parser,
             is_lambda_literal ? PM_BINDING_POWER_DEFINED : PM_BINDING_POWER_INDEX,
@@ -15367,6 +15372,9 @@ parse_block_parameters(
             true,
             (uint16_t) (depth + 1)
         );
+        if (!is_lambda_literal) {
+            context_pop(parser);
+        }
     }
 
     pm_block_parameters_node_t *block_parameters = pm_block_parameters_node_create(parser, parameters, opening);
@@ -15717,6 +15725,7 @@ parse_return(pm_parser_t *parser, pm_node_t *node) {
                 // These contexts are invalid for a return.
                 pm_parser_err_node(parser, node, PM_ERR_RETURN_INVALID);
                 return;
+            case PM_CONTEXT_BLOCK_PARAMETERS:
             case PM_CONTEXT_BLOCK_BRACES:
             case PM_CONTEXT_BLOCK_ELSE:
             case PM_CONTEXT_BLOCK_ENSURE:
@@ -15754,6 +15763,7 @@ static void
 parse_block_exit(pm_parser_t *parser, pm_node_t *node) {
     for (pm_context_node_t *context_node = parser->current_context; context_node != NULL; context_node = context_node->prev) {
         switch (context_node->context) {
+            case PM_CONTEXT_BLOCK_PARAMETERS:
             case PM_CONTEXT_BLOCK_BRACES:
             case PM_CONTEXT_BLOCK_KEYWORDS:
             case PM_CONTEXT_BLOCK_ELSE:
@@ -17973,6 +17983,7 @@ parse_retry(pm_parser_t *parser, const pm_node_t *node) {
                 assert(false && "unreachable");
                 break;
             case PM_CONTEXT_BEGIN:
+            case PM_CONTEXT_BLOCK_PARAMETERS:
             case PM_CONTEXT_BLOCK_BRACES:
             case PM_CONTEXT_BLOCK_KEYWORDS:
             case PM_CONTEXT_CASE_IN:
@@ -18051,6 +18062,7 @@ parse_yield(pm_parser_t *parser, const pm_node_t *node) {
             case PM_CONTEXT_BEGIN_ELSE:
             case PM_CONTEXT_BEGIN_ENSURE:
             case PM_CONTEXT_BEGIN_RESCUE:
+            case PM_CONTEXT_BLOCK_PARAMETERS:
             case PM_CONTEXT_BLOCK_BRACES:
             case PM_CONTEXT_BLOCK_KEYWORDS:
             case PM_CONTEXT_BLOCK_ELSE:
@@ -19611,7 +19623,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, b
             pm_token_t equal;
             pm_token_t end_keyword;
 
-            if (accept1(parser, PM_TOKEN_EQUAL)) {
+            if (!context_p(parser, PM_CONTEXT_BLOCK_PARAMETERS) && accept1(parser, PM_TOKEN_EQUAL)) {
                 if (token_is_setter_name(&name)) {
                     pm_parser_err_token(parser, &name, PM_ERR_DEF_ENDLESS_SETTER);
                 }

It's more concise but also not correct. For example, it rejects foo do |a = (def f = 1)|; end. Generally I have not much clue how to correctly add a new context. Maybe it does add more complexity that necessary, or can you implement using a context instead?

[Earlopain]

@kddnewton I've sent you an email two days ago to your git commit email. Did you receive it/any other way to get in contact with you?

[kddnewton]

Sorry yes I will respond

[kddnewton]

I rewrote it slightly to use a new context, I think I'm going to roll with this approach. Thanks for the patch, it helped!