Merge pull request #731 from wado-lang/claude/fix-cst-group-afN1p

Implement CST Group support for Gale parser generator

Author: gfx

package-gale/TODO.md

@@ -1,35 +1,49 @@
 # Gale TODO
 
-## CST Group Fix
+## Remove `store` parameter from `gen_element`
 
-CST Group support (storing `(rule_a | rule_b)*` results in the parse tree) is implemented but blocked by a compiler limitation.
+`gen_element` has a `store: bool` parameter that controls generated variable naming:
 
-### What's done
+- `store=true`: `let field_name = parse_xxx(p)?;` (meaningful name for struct field assignment)
+- `store=false`: `let tok = parse_xxx(p)?;` (throwaway name)
 
-- **generator.wado**: Group variant type generation (`SqlStmtListOrErrorGroup` etc.)
-- **parser_gen.wado**: Parser stores Group results in variant fields
-- **visitor_gen.wado**: Walker generates match dispatch for Group variants
-- **All code is tested and works** when compiled standalone
+`store=false` is a workaround for a naming collision problem in `dedup_name`. The underlying issue is that `dedup_name` assumes sequential variable creation, but branching code (optionals, groups, prediction trees) creates variables in different scopes:
 
-### What's blocked
+```
+// gen_optional_with_lookahead generates:
+if condition {
+    let k_limit = p.expect(...)?;
+    let expr = parse_expr(p)?;        // dedup count 0
+    if grp_kind == TK_K_OFFSET { ... }
+    let expr = parse_expr(p)?;        // dedup count 0 again → collision!
+}
+```
 
-The walker generates `for let item of &node.group_list` which iterates `&Array<GroupVariant>`. This requires `IntoIterator for &Array<T>` → `ArrayRefIter<T>` → `Iterator` trait impl.
+With `store=false`, both become `let tok = ...` / `let tok_2 = ...` (separate namespace), avoiding the collision. But this is a hack — all generated variables should use meaningful names.
 
-`ArrayRefIter<T>` now implements `Iterator` (e2e tests pass), but the monomorphizer **eagerly instantiates all default methods** (collect, map, filter, fold, etc.) for every `T`, causing OOM when compiling large programs like gale itself.
+### Root cause
 
-### Fix path
+`dedup_name` tracks a flat counter per name. Optional/group paths increment this counter even though their variables are scoped to if-blocks. When mandatory elements follow, `gen_field_assignments` expects names at count N but the parsing code produced count N+K (shifted by optional paths).
 
-```
-1. Lazy monomorphization of trait default methods
-   (only instantiate methods that are actually called)
-     ↓
-2. Gale compiles without OOM (ArrayRefIter<T> already implements Iterator)
-     ↓
-3. CST Group fix can use `for let item of &node.group_list`
-     ↓
-4. SQLite parser's to_tree() test passes
-```
+### Fix approach
+
+1. **Scope-aware naming**: Track which variables are in which scope. Variables inside if/else blocks don't affect the parent scope's counter.
+2. **Or**: Make `gen_field_assignments` use the same `name_counts` instance that the parsing code used, instead of replaying from scratch.
+3. **Or**: Wrap all non-field parsing (groups, optionals, prediction branches) in labeled scope blocks (`__scope: { ... }`) and use fresh `name_counts` inside. This isolates variable names to their scope.
+
+Approach 3 was partially implemented but incomplete — optional paths that share elements with the enclosing alt (e.g., `LIMIT expr ((OFFSET|',') expr)?` where `expr` appears both in the alt and inside the optional) need the parent's dedup counter to produce `expr_2` for the second occurrence.
+
+A complete fix likely requires combining approaches: scope blocks for isolation + parent counter awareness for shared element types.
+
+## Incomplete CST walker coverage
+
+The XML unparse output is missing tokens/nodes for several patterns due to `store=false` and non-simple groups:
+
+- **Repeated separators**: `(',' result_column)*` — the `','` and subsequent `result_column` are not stored, so `SELECT a, b` only shows `a`.
+- **Token-only groups**: `(K_INSERT | K_REPLACE)` — groups with only TokenRef alternatives are not `is_simple_cst_group` (requires RuleRef alternatives), so `INSERT` keyword is missing from `insert_stmt`.
+- **Multi-element single-alt groups**: `(';'+ sql_stmt)*` — the Star inner group has two elements (Plus and RuleRef), not a simple CST group, so second statements are not stored.
 
-### Workaround (if lazy monomorphization is too large)
+Fixing these requires either:
 
-Change the walker to use value iteration (`for let item of node.group_list` without `&`). This copies array elements but avoids the `&Array<T>` IntoIterator issue. The walker already works with value semantics for non-Group fields.
+1. Extending `is_simple_cst_group` to handle token-only and mixed groups (generating variant types for tokens too).
+2. Or making `gen_repeat` for Star/Plus store all inner elements (requires struct types for group alternatives with multiple elements).

package-gale/src/gen_util.wado

@@ -1,3 +1,5 @@
+use { Alternative } from "./ir.wado";
+
 pub fn to_pascal_case(name: &String) -> String {
     let mut result = String::with_capacity(name.len());
     let mut capitalize_next = true;
@@ -195,6 +197,49 @@ pub fn strip_suffix(s: &String, suffix: &String) -> String {
     return result;
 }
 
+pub fn is_simple_cst_group(alts: &Array<Alternative>) -> bool {
+    if alts.len() < 2 { return false; }
+    for let alt of alts {
+        if alt.elements.len() != 1 { return false; }
+        if let RuleRef(_) = alt.elements[0] {
+            // ok
+        } else {
+            return false;
+        }
+    }
+    return true;
+}
+
+pub fn group_variant_type_name(alts: &Array<Alternative>) -> String {
+    let mut result = String::with_capacity(64);
+    for let mut i = 0; i < alts.len(); i += 1 {
+        if i > 0 { result.append("Or"); }
+        if let RuleRef(name) = &alts[i].elements[0] {
+            result.append(&to_pascal_case(name));
+        }
+    }
+    result.append("Group");
+    return result;
+}
+
+pub fn group_field_base_name(alts: &Array<Alternative>) -> String {
+    let mut result = String::with_capacity(64);
+    for let mut i = 0; i < alts.len(); i += 1 {
+        if i > 0 { result.append("_or_"); }
+        if let RuleRef(name) = &alts[i].elements[0] {
+            result.append(&to_snake_case(name));
+        }
+    }
+    return result;
+}
+
+pub fn group_case_name(alt: &Alternative) -> String {
+    if let RuleRef(name) = &alt.elements[0] {
+        return to_pascal_case(name);
+    }
+    return "Unknown";
+}
+
 pub fn array_contains_str(arr: &Array<String>, value: &String) -> bool {
     for let item of arr {
         if *item == *value {

package-gale/src/generator.wado

@@ -1,5 +1,5 @@
 use { Grammar, ParserRule, Alternative, Element, RepeatElement, LabelElement, RepeatKind, LexerRule } from "./ir.wado";
-use { to_pascal_case, to_snake_case, to_lower, rule_type_name, literal_field_name, strip_suffix } from "./gen_util.wado";
+use { to_pascal_case, to_snake_case, to_lower, rule_type_name, literal_field_name, strip_suffix, is_simple_cst_group, group_variant_type_name, group_field_base_name, group_case_name } from "./gen_util.wado";
 use { collect_literal_tokens, gen_token_constants, gen_lexer } from "./lexer_gen.wado";
 use { gen_parser } from "./parser_gen.wado";
 use { gen_visitor } from "./visitor_gen.wado";
@@ -78,6 +78,7 @@ fn gen_parser_rule_type(w: &mut CodeWriter, rule: &ParserRule) {
 }
 
 fn gen_struct_for_alt(w: &mut CodeWriter, type_name: &String, alt: &Alternative) {
+    gen_group_variant_types(w, &alt.elements);
     let fields = collect_fields(&alt.elements);
     let mut s = StructSpec::new(type_name);
     s.set_pub();
@@ -89,6 +90,57 @@ fn gen_struct_for_alt(w: &mut CodeWriter, type_name: &String, alt: &Alternative)
     w.blank();
 }
 
+fn repeat_inner_field(elem: &Element) -> Option<Field> {
+    if let Group(alts) = *elem {
+        if is_simple_cst_group(&alts) {
+            return Option::<Field>::Some(Field {
+                name: group_field_base_name(&alts),
+                type_str: group_variant_type_name(&alts),
+            });
+        }
+        return null;
+    }
+    return element_to_field(elem);
+}
+
+fn gen_group_variant_types(w: &mut CodeWriter, elements: &Array<Element>) {
+    for let elem of elements {
+        let alts = extract_group_alts(&elem);
+        if let Some(group_alts) = alts {
+            if is_simple_cst_group(group_alts) {
+                gen_group_variant_type(w, group_alts);
+            }
+        }
+    }
+}
+
+fn extract_group_alts(elem: &Element) -> Option<&Array<Alternative>> {
+    if let Group(alts) = elem {
+        return Option::Some(alts);
+    }
+    if let Repeat(rep) = elem {
+        if let Group(alts) = &rep.element {
+            return Option::Some(alts);
+        }
+    }
+    return null;
+}
+
+fn gen_group_variant_type(w: &mut CodeWriter, alts: &Array<Alternative>) {
+    let type_name = group_variant_type_name(alts);
+    let mut v = VariantSpec::new(&type_name);
+    v.set_pub();
+    for let alt of alts {
+        if let RuleRef(name) = &alt.elements[0] {
+            let case_name = to_pascal_case(name);
+            let payload = rule_type_name(name);
+            v.add_case_with_payload(&case_name, &payload);
+        }
+    }
+    v.emit(w);
+    w.blank();
+}
+
 struct Field {
     name: String,
     type_str: String,
@@ -148,7 +200,8 @@ fn element_to_field(elem: &Element) -> Option<Field> {
         });
     }
     if let Repeat(rep) = *elem {
-        if let Some(inner) = element_to_field(&rep.element) {
+        let inner_field = repeat_inner_field(&rep.element);
+        if let Some(inner) = inner_field {
             let field = match rep.kind {
                 Star | Plus => Field {
                     name: `{inner.name}_list`,
@@ -172,6 +225,15 @@ fn element_to_field(elem: &Element) -> Option<Field> {
         }
         return null;
     }
+    if let Group(alts) = *elem {
+        if is_simple_cst_group(&alts) {
+            return Option::<Field>::Some(Field {
+                name: group_field_base_name(&alts),
+                type_str: `Option<{group_variant_type_name(&alts)}>`,
+            });
+        }
+        return null;
+    }
     return null;
 }
 

package-gale/src/generator_test.wado

@@ -180,6 +180,68 @@ test "generate duplicate field names" {
     assert str_contains(&output, "    pub item_2: ItemNode,");
 }
 
+test "generate group variant type for repeated group" {
+    let grammar = Grammar {
+        name: "Test",
+        parser_rules: [
+            ParserRule {
+                name: "parse",
+                alternatives: [
+                    Alternative {
+                        label: null,
+                        elements: [
+                            Element::Repeat(RepeatElement {
+                                kind: RepeatKind::Star,
+                                element: Element::Group([
+                                    Alternative { label: null, elements: [Element::RuleRef("stmt")] },
+                                    Alternative { label: null, elements: [Element::RuleRef("error")] },
+                                ]),
+                                non_greedy: false,
+                            }),
+                            Element::TokenRef("EOF"),
+                        ],
+                    },
+                ],
+            },
+        ],
+        lexer_rules: [],
+    };
+    let output = generate(&grammar);
+    assert str_contains(&output, "pub variant StmtOrErrorGroup {");
+    assert str_contains(&output, "    Stmt(StmtNode),");
+    assert str_contains(&output, "    Error(ErrorNode),");
+    assert str_contains(&output, "    pub stmt_or_error_list: Array<StmtOrErrorGroup>,");
+}
+
+test "generate standalone group as optional field" {
+    let grammar = Grammar {
+        name: "Test",
+        parser_rules: [
+            ParserRule {
+                name: "cmd",
+                alternatives: [
+                    Alternative {
+                        label: null,
+                        elements: [
+                            Element::Group([
+                                Alternative { label: null, elements: [Element::RuleRef("foo")] },
+                                Alternative { label: null, elements: [Element::RuleRef("bar")] },
+                            ]),
+                            Element::TokenRef("EOF"),
+                        ],
+                    },
+                ],
+            },
+        ],
+        lexer_rules: [],
+    };
+    let output = generate(&grammar);
+    assert str_contains(&output, "pub variant FooOrBarGroup {");
+    assert str_contains(&output, "    Foo(FooNode),");
+    assert str_contains(&output, "    Bar(BarNode),");
+    assert str_contains(&output, "    pub foo_or_bar: Option<FooOrBarGroup>,");
+}
+
 fn str_contains(haystack: &String, needle: &String) -> bool {
     let h_len = haystack.len();
     let n_len = needle.len();