Split KeywordParameterNode into Optional and Required

Prior to this commit, KeywordParameterNode included both optional and required keywords. With this commit, it is split in two, with `OptionalKeywordParameterNode`s no longer having a value field.
ruby · Oct 31, 2023 · 89084d9 · 89084d9
1 parent fcc9298
commit 89084d9
Show file tree

Hide file tree

Showing 38 changed files with 178 additions and 165 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a
 
 ## [Unreleased]
 
+### Changed
+
+- **BREAKING**: `KeywordParameterNode` is split into `OptionalKeywordParameterNode` and `RequiredKeywordParameterNode`. `OptionalKeywordParameterNode` has no `value` field.
+
 ## [0.16.0] - 2023-10-30
 
 ### Added

diff --git a/config.yml b/config.yml
@@ -1692,24 +1692,6 @@ nodes:
 
           foo(a: b)
               ^^^^
-  - name: KeywordParameterNode
-    fields:
-      - name: name
-        type: constant
-      - name: name_loc
-        type: location
-      - name: value
-        type: node?
-    comment: |
-      Represents a keyword parameter to a method, block, or lambda definition.
-
-          def a(b:)
-                ^^
-          end
-
-          def a(b: 1)
-                ^^^^
-          end
   - name: KeywordRestParameterNode
     fields:
       - name: name
@@ -1997,6 +1979,18 @@ nodes:
 
           $1
           ^^
+  - name: OptionalKeywordParameterNode
+    fields:
+      - name: name
+        type: constant
+      - name: name_loc
+        type: location
+    comment: |
+      Represents an optional keyword parameter to a method, block, or lambda definition.
+
+          def a(b: )
+                ^^
+          end
   - name: OptionalParameterNode
     fields:
       - name: name
@@ -2184,6 +2178,20 @@ nodes:
 
           /foo/i
           ^^^^^^
+  - name: RequiredKeywordParameterNode
+    fields:
+      - name: name
+        type: constant
+      - name: name_loc
+        type: location
+      - name: value
+        type: node
+    comment: |
+      Represents a required keyword parameter to a method, block, or lambda definition.
+
+          def a(b: 1)
+                ^^^^
+          end
   - name: RequiredParameterNode
     fields:
       - name: name

diff --git a/lib/prism/debug.rb b/lib/prism/debug.rb
@@ -114,8 +114,8 @@ def self.prism_locals(source)
                   AnonymousLocal
                 end
               end,
-              *params.keywords.reject(&:value).map(&:name),
-              *params.keywords.select(&:value).map(&:name)
+              *params.keywords.select { |kw| kw.is_a? OptionalKeywordParameterNode }.map(&:name),
+              *params.keywords.select { |kw| kw.is_a? RequiredKeywordParameterNode }.map(&:name),
             ]
 
             sorted << AnonymousLocal if params.keywords.any?

diff --git a/src/prism.c b/src/prism.c
@@ -3326,17 +3326,37 @@ pm_keyword_hash_node_elements_append(pm_keyword_hash_node_t *hash, pm_node_t *el
     hash->base.location.end = element->location.end;
 }
 
-// Allocate a new KeywordParameterNode node.
-static pm_keyword_parameter_node_t *
-pm_keyword_parameter_node_create(pm_parser_t *parser, const pm_token_t *name, pm_node_t *value) {
-    pm_keyword_parameter_node_t *node = PM_ALLOC_NODE(parser, pm_keyword_parameter_node_t);
+// Allocate a new OptionalKeywordParameterNode node.
+static pm_optional_keyword_parameter_node_t *
+pm_optional_keyword_parameter_node_create(pm_parser_t *parser, const pm_token_t *name) {
+    pm_optional_keyword_parameter_node_t *node = PM_ALLOC_NODE(parser, pm_optional_keyword_parameter_node_t);
 
-    *node = (pm_keyword_parameter_node_t) {
+    *node = (pm_optional_keyword_parameter_node_t) {
         {
-            .type = PM_KEYWORD_PARAMETER_NODE,
+            .type = PM_OPTIONAL_KEYWORD_PARAMETER_NODE,
             .location = {
                 .start = name->start,
-                .end = value == NULL ? name->end : value->location.end
+                .end = name->end
+            },
+        },
+        .name = pm_parser_constant_id_location(parser, name->start, name->end - 1),
+        .name_loc = PM_LOCATION_TOKEN_VALUE(name),
+    };
+
+    return node;
+}
+
+// Allocate a new RequiredKeywordParameterNode node.
+static pm_required_keyword_parameter_node_t *
+pm_required_keyword_parameter_node_create(pm_parser_t *parser, const pm_token_t *name, pm_node_t *value) {
+    pm_required_keyword_parameter_node_t *node = PM_ALLOC_NODE(parser, pm_required_keyword_parameter_node_t);
+
+    *node = (pm_required_keyword_parameter_node_t) {
+        {
+            .type = PM_REQUIRED_KEYWORD_PARAMETER_NODE,
+            .location = {
+                .start = name->start,
+                .end = value->location.end
             },
         },
         .name = pm_parser_constant_id_location(parser, name->start, name->end - 1),
@@ -10482,7 +10502,7 @@ parse_parameters(
                     case PM_TOKEN_COMMA:
                     case PM_TOKEN_PARENTHESIS_RIGHT:
                     case PM_TOKEN_PIPE: {
-                        pm_node_t *param = (pm_node_t *) pm_keyword_parameter_node_create(parser, &name, NULL);
+                        pm_node_t *param = (pm_node_t *) pm_optional_keyword_parameter_node_create(parser, &name);
                         pm_parameters_node_keywords_append(params, param);
                         break;
                     }
@@ -10493,19 +10513,23 @@ parse_parameters(
                             break;
                         }
 
-                        pm_node_t *param = (pm_node_t *) pm_keyword_parameter_node_create(parser, &name, NULL);
+                        pm_node_t *param = (pm_node_t *) pm_optional_keyword_parameter_node_create(parser, &name);
                         pm_parameters_node_keywords_append(params, param);
                         break;
                     }
                     default: {
-                        pm_node_t *value = NULL;
+                        pm_node_t *param;
+
                         if (token_begins_expression_p(parser->current.type)) {
                             context_push(parser, PM_CONTEXT_DEFAULT_PARAMS);
-                            value = parse_expression(parser, binding_power, PM_ERR_PARAMETER_NO_DEFAULT_KW);
+                            pm_node_t *value = parse_expression(parser, binding_power, PM_ERR_PARAMETER_NO_DEFAULT_KW);
                             context_pop(parser);
+                            param = (pm_node_t *) pm_required_keyword_parameter_node_create(parser, &name, value);
+                        }
+                        else {
+                            param = (pm_node_t *) pm_optional_keyword_parameter_node_create(parser, &name);
                         }
 
-                        pm_node_t *param = (pm_node_t *) pm_keyword_parameter_node_create(parser, &name, value);
                         pm_parameters_node_keywords_append(params, param);
 
                         // If parsing the value of the parameter resulted in error recovery,

diff --git a/test/prism/errors_test.rb b/test/prism/errors_test.rb
@@ -746,7 +746,7 @@ def test_keywords_parameters_before_required_parameters
           [],
           nil,
           [RequiredParameterNode(:a)],
-          [KeywordParameterNode(:b, Location(), nil)],
+          [OptionalKeywordParameterNode(:b, Location())],
           nil,
           nil
         ),
@@ -774,7 +774,7 @@ def test_rest_keywords_parameters_before_required_parameters
           [],
           nil,
           [],
-          [KeywordParameterNode(:b, Location(), nil)],
+          [OptionalKeywordParameterNode(:b, Location())],
           KeywordRestParameterNode(:rest, Location(), Location()),
           nil
         ),
@@ -824,7 +824,7 @@ def test_multiple_error_in_parameters_order
           [],
           nil,
           [RequiredParameterNode(:a)],
-          [KeywordParameterNode(:b, Location(), nil)],
+          [OptionalKeywordParameterNode(:b, Location())],
           KeywordRestParameterNode(:args, Location(), Location()),
           nil
         ),
@@ -854,7 +854,7 @@ def test_switching_to_optional_arguments_twice
           [],
           nil,
           [RequiredParameterNode(:a)],
-          [KeywordParameterNode(:b, Location(), nil)],
+          [OptionalKeywordParameterNode(:b, Location())],
           KeywordRestParameterNode(:args, Location(), Location()),
           nil
         ),
@@ -884,7 +884,7 @@ def test_switching_to_named_arguments_twice
           [],
           nil,
           [RequiredParameterNode(:a)],
-          [KeywordParameterNode(:b, Location(), nil)],
+          [OptionalKeywordParameterNode(:b, Location())],
           KeywordRestParameterNode(:args, Location(), Location()),
           nil
         ),

diff --git a/test/prism/location_test.rb b/test/prism/location_test.rb
@@ -502,16 +502,6 @@ def test_KeywordHashNode
       assert_location(KeywordHashNode, "foo(a, b: 1)", 7...11) { |node| node.arguments.arguments[1] }
     end
 
-    def test_KeywordParameterNode
-      assert_location(KeywordParameterNode, "def foo(bar:); end", 8...12) do |node|
-        node.parameters.keywords.first
-      end
-
-      assert_location(KeywordParameterNode, "def foo(bar: nil); end", 8...16) do |node|
-        node.parameters.keywords.first
-      end
-    end
-
     def test_KeywordRestParameterNode
       assert_location(KeywordRestParameterNode, "def foo(**); end", 8...10) do |node|
         node.parameters.keyword_rest
@@ -609,6 +599,12 @@ def test_NumberedReferenceReadNode
       assert_location(NumberedReferenceReadNode, "$1")
     end
 
+    def test_OptionalKeywordParameterNode
+      assert_location(OptionalKeywordParameterNode, "def foo(bar:); end", 8...12) do |node|
+        node.parameters.keywords.first
+      end
+    end
+
     def test_OptionalParameterNode
       assert_location(OptionalParameterNode, "def foo(bar = nil); end", 8...17) do |node|
         node.parameters.optionals.first
@@ -673,6 +669,12 @@ def test_RegularExpressionNode
       assert_location(RegularExpressionNode, "/foo/")
     end
 
+    def test_RequiredKeywordParameterNode
+      assert_location(RequiredKeywordParameterNode, "def foo(bar: nil); end", 8...16) do |node|
+        node.parameters.keywords.first
+      end
+    end
+
     def test_RequiredParameterNode
       assert_location(RequiredParameterNode, "def foo(bar); end", 8...11) do |node|
         node.parameters.requireds.first

diff --git a/test/prism/snapshots/blocks.txt b/test/prism/snapshots/blocks.txt
diff --git a/test/prism/snapshots/lambda.txt b/test/prism/snapshots/lambda.txt