tree-sitter / tree-sitter-c-sharp

C# Grammar for tree-sitter
MIT License
177 stars 47 forks source link

Change `parameters` field name in lambda expression #314

Closed tamasvajk closed 1 year ago

tamasvajk commented 1 year ago

Changing the below:

--- a/grammar.js
+++ b/grammar.js
@@ -1178,7 +1178,7 @@ module.exports = grammar({
       repeat($.attribute_list),
       optional(alias(choice('async', 'static', seq('async', 'static'), seq('static', 'async')), $.modifier)),
       optional($._type),
-      choice(field('parameters', $.parameter_list), $.identifier),
+      field('parameters', choice($.parameter_list, $.identifier)),
       '=>',
       field('body', choice($.block, $._expression))
     )),

results in a significant parser size reduction:

--- a/script/file_sizes.txt
+++ b/script/file_sizes.txt
@@ -1,5 +1,5 @@
 src/grammar.json       0.2MB        10975
-src/node-types.json    0.1MB         7685
-src/parser.c           48.9MB     1534188
+src/node-types.json    0.1MB         7689
+src/parser.c           41.2MB     1300296
 src/scanner.c          0.0MB           37
-total                  49.3MB     1552885
+total                  41.5MB     1318997
tamasvajk commented 1 year ago

The output is slightly changed by this modification, which might or might not be acceptable. @damieng what do you think?

--- a/corpus/contextual-keywords.txt
+++ b/corpus/contextual-keywords.txt
@@ -277,7 +277,7 @@ class scoped { }
               (identifier)
               (equals_value_clause
                 (lambda_expression
-                  (identifier)
+                  parameters: (identifier)
                   body: (null_literal))))))
         (local_declaration_statement
           (variable_declaration
tamasvajk commented 1 year ago

I think it could be improved slightly by making it more similar to:

                (lambda_expression
                  parameters: (parameter_list
                    (parameter
                      type: (identifier)
                      name: (identifier)))

Something along these lines:

                (lambda_expression
                  parameters: (implicit_parameter_list
                    (parameter
                      name: (identifier)))

What do you think?

damieng commented 1 year ago

This looks like an improvement all round - wonder why that one has such an impact on the size.

tamasvajk commented 1 year ago

This looks like an improvement all round - wonder why that one has such an impact on the size.

I have no clue yet. I'll have a look at other field calls inside a choice. I pushed some extra commits to adjust the trees:

--- a/corpus/contextual-keywords.txt
+++ b/corpus/contextual-keywords.txt
@@ -277,7 +277,9 @@ class scoped { }
               (identifier)
               (equals_value_clause
                 (lambda_expression
-                  parameters: (identifier)
+                  parameters: (implicit_parameter_list
+                    (parameter
+                      name: (identifier)))
                   body: (null_literal))))))
         (local_declaration_statement
           (variable_declaration
tamasvajk commented 1 year ago

I couldn't find another field that would have similar impact on the parser size. I don't know why this one is so special.

damieng commented 1 year ago

Nice savings.