From 3ed2154a87e5d4c1fcaf9d05819987a47bc32ec5 Mon Sep 17 00:00:00 2001 From: Rio Yu <52408936+rioyu123@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:59:36 +0800 Subject: [PATCH] fix(action-grammar): parse optional rule references --- ts/docs/architecture/core/actionGrammar.md | 33 +++-- .../actionGrammar/src/grammarCompiler.ts | 58 +++++--- .../actionGrammar/src/grammarRuleParser.ts | 22 ++- .../actionGrammar/src/grammarRuleWriter.ts | 1 + .../test/grammarRuleParser.spec.ts | 31 +++++ .../test/grammarRuleWriter.spec.ts | 18 +++ .../actionGrammar/test/nfaDfaParity.spec.ts | 129 ++++++++++++++++++ 7 files changed, 254 insertions(+), 38 deletions(-) diff --git a/ts/docs/architecture/core/actionGrammar.md b/ts/docs/architecture/core/actionGrammar.md index 84cb783ab3..945d7332e3 100644 --- a/ts/docs/architecture/core/actionGrammar.md +++ b/ts/docs/architecture/core/actionGrammar.md @@ -82,19 +82,26 @@ separated by `|`: #### Expression types -| Syntax | Meaning | -| ------------------- | -------------------------------------- | -| `word` | Literal token (case-insensitive match) | -| `$(var:wildcard)` | Capture any tokens as string | -| `$(var:number)` | Capture numeric token | -| `$(var:EntityType)` | Capture with entity validation | -| `$(var:)` | Capture via sub-rule match | -| `` | Reference another rule (no capture) | -| `( expr )` | Grouping | -| `expr?` | Optional (zero or one) | -| `expr*` | Zero or more | -| `expr+` | One or more | -| `alt1 \| alt2` | Alternation | +| Syntax | Meaning | +| -------------------- | -------------------------------------- | +| `word` | Literal token (case-insensitive match) | +| `$(var:wildcard)` | Capture any tokens as string | +| `$(var:number)` | Capture numeric token | +| `$(var:EntityType)` | Capture with entity validation | +| `$(var:type)?` | Optional captured variable | +| `$(var:)` | Capture via sub-rule match | +| `$(var:)?` | Optional captured sub-rule | +| `` | Reference another rule (no capture) | +| `?` | Optional rule reference | +| `( expr )` | Grouping | +| `( expr )?` | Optional group (zero or one) | +| `( expr )*` | Zero or more | +| `( expr )+` | One or more | +| `alt1 \| alt2` | Alternation | + +The `?` suffix must immediately follow the closing `)` or `>`. A literal +question mark immediately after a rule reference must be escaped as `\?`; +a question mark separated by whitespace remains literal. #### Value expressions diff --git a/ts/packages/actionGrammar/src/grammarCompiler.ts b/ts/packages/actionGrammar/src/grammarCompiler.ts index 7ede554fd9..dd924b3a39 100644 --- a/ts/packages/actionGrammar/src/grammarCompiler.ts +++ b/ts/packages/actionGrammar/src/grammarCompiler.ts @@ -1325,22 +1325,42 @@ function createGrammarRule( !isLocallyDefined && globalPhraseSetRegistry.isPhraseSetName(expr.refName.name) ) { + const partId = allocPartId( + context, + expr.pos, + `<${expr.refName.name}>${expr.optional ? "?" : ""}`, + ); parts.push( - createPhraseSetPart( - expr.refName.name, - undefined, - allocPartId( - context, - expr.pos, - `<${expr.refName.name}>`, - ), - ), + expr.optional + ? createRulesPart( + [ + { + parts: [ + createPhraseSetPart( + expr.refName.name, + ), + ], + value: undefined, + spacingMode, + }, + ], + { + optional: true, + name: expr.refName.name, + partId, + }, + ) + : createPhraseSetPart( + expr.refName.name, + undefined, + partId, + ), ); // Phrase sets don't produce a captured value on their own. // Use defaultValue=true so single-part rules using a phrase set // don't trip the "Start rule does not produce a value" check. defaultValue = true; - consumedInput(); // phrase sets always consume input + if (!expr.optional) consumedInput(); break; } const record = createNamedGrammarRules( @@ -1354,22 +1374,24 @@ function createGrammarRule( defaultValue = record.hasValue; parts.push( createRulesPart(record.grammarRules, { + optional: expr.optional, name: expr.refName.name, partId: allocPartId( context, expr.pos, - `<${expr.refName.name}>`, + `<${expr.refName.name}>${expr.optional ? "?" : ""}`, ), }), ); - // RuleRefExpr has no optional modifier; it is always non-optional. - // === false: only clear when *definitely* non-nullable (same - // asymmetry as the variable ruleRef case above). - if (record.nullable === false) { - currentEpr = new Set(); + if (!expr.optional) { + // === false: only clear when *definitely* non-nullable (same + // asymmetry as the variable ruleRef case above). + if (record.nullable === false) { + currentEpr = new Set(); + } + // ?? false: treat undefined (back-ref) as non-nullable. + ruleNullable = ruleNullable && (record.nullable ?? false); } - // ?? false: treat undefined (back-ref) as non-nullable. - ruleNullable = ruleNullable && (record.nullable ?? false); break; } case "rules": { diff --git a/ts/packages/actionGrammar/src/grammarRuleParser.ts b/ts/packages/actionGrammar/src/grammarRuleParser.ts index 247281946e..6f4ca61951 100644 --- a/ts/packages/actionGrammar/src/grammarRuleParser.ts +++ b/ts/packages/actionGrammar/src/grammarRuleParser.ts @@ -87,7 +87,7 @@ const debugParse = registerDebug("typeagent:grammar:parse"); * * ::= (":" ( | ))? * - * ::= + * ::= (immediately followed by "?")? * ::= "(" ( ")" | ")?" | ")*" | ")+" ) * * // ── Value (basic mode: enableValueExpressions=false) ────────────────────────── @@ -232,6 +232,7 @@ export type CommentedName = { export type RuleRefExpr = { type: "ruleReference"; refName: CommentedName; + optional?: boolean | undefined; pos?: number | undefined; leadingComments?: Comment[] | undefined; }; @@ -753,7 +754,7 @@ class GrammarRuleParser implements ValueExprParserContext { refPos = this.pos; if (this.isAt("<")) { ruleReference = true; - bracketedName = this.parseRuleName(); + bracketedName = this.parseRuleName().name; } else { bracketedName = this.parseNameWithComments("Type name"); } @@ -798,11 +799,17 @@ class GrammarRuleParser implements ValueExprParserContext { if (this.isAt("<")) { const pos = this.pos; + const { name: refName, end } = this.parseRuleName(); const node: RuleRefExpr = { type: "ruleReference", - refName: this.parseRuleName(), + refName, pos, }; + if (this.content.startsWith("?", end)) { + node.optional = true; + this.curr = end; + this.skipWhitespace(1); + } attach(node); expNodes.push(node); continue; @@ -1171,11 +1178,12 @@ class GrammarRuleParser implements ValueExprParserContext { return { name, leadingComments, trailingComments }; } - private parseRuleName(): CommentedName { + private parseRuleName(): { name: CommentedName; end: number } { this.consume("<", "at start of rule name"); - const result = this.parseNameWithComments("Rule identifier"); + const name = this.parseNameWithComments("Rule identifier"); + const end = this.curr + 1; this.consume(">", "at end of rule name"); - return result; + return { name, end }; } private parseRules(): Rule[] { @@ -1238,7 +1246,7 @@ class GrammarRuleParser implements ValueExprParserContext { afterExportComments?: Comment[], ): RuleDefinition { const pos = this.pos; - const rn = this.parseRuleName(); + const rn = this.parseRuleName().name; let spacingMode: SpacingMode; let spacingAnnotationComments: SpacingAnnotationComments | undefined; let beforeEqualsComments: Comment[] | undefined; diff --git a/ts/packages/actionGrammar/src/grammarRuleWriter.ts b/ts/packages/actionGrammar/src/grammarRuleWriter.ts index 52719e091e..23d6dbba92 100644 --- a/ts/packages/actionGrammar/src/grammarRuleWriter.ts +++ b/ts/packages/actionGrammar/src/grammarRuleWriter.ts @@ -925,6 +925,7 @@ function writeSingleExpr( } case "ruleReference": writeBracketedName(result, expr.refName); + if (expr.optional) result.write("?"); break; case "rules": { result.write("("); diff --git a/ts/packages/actionGrammar/test/grammarRuleParser.spec.ts b/ts/packages/actionGrammar/test/grammarRuleParser.spec.ts index f4dc089989..c4d211f890 100644 --- a/ts/packages/actionGrammar/test/grammarRuleParser.spec.ts +++ b/ts/packages/actionGrammar/test/grammarRuleParser.spec.ts @@ -88,6 +88,37 @@ describe("Grammar Rule Parser", () => { value: ["world"], }); }); + + it("rule with optional rule reference", () => { + const grammar = " = ? world;"; + const result = testParamGrammarRules("test.agr", grammar); + + expect(result[0].rules[0].expressions).toHaveLength(2); + expect(result[0].rules[0].expressions[0]).toEqual({ + type: "ruleReference", + refName: { name: "greeting" }, + optional: true, + }); + expect(result[0].rules[0].expressions[1]).toEqual({ + type: "string", + value: ["world"], + }); + }); + + it("requires the optional suffix to be adjacent", () => { + const grammar = " = ? world;"; + const result = testParamGrammarRules("test.agr", grammar); + + expect(result[0].rules[0].expressions).toHaveLength(2); + expect(result[0].rules[0].expressions[0]).toEqual({ + type: "ruleReference", + refName: { name: "greeting" }, + }); + expect(result[0].rules[0].expressions[1]).toEqual({ + type: "string", + value: ["?", "world"], + }); + }); }); describe("Expression Parsing", () => { diff --git a/ts/packages/actionGrammar/test/grammarRuleWriter.spec.ts b/ts/packages/actionGrammar/test/grammarRuleWriter.spec.ts index 799f72293c..d6b5598bf0 100644 --- a/ts/packages/actionGrammar/test/grammarRuleWriter.spec.ts +++ b/ts/packages/actionGrammar/test/grammarRuleWriter.spec.ts @@ -35,6 +35,24 @@ function validateRoundTrip(grammar: string) { } describe("Formatting layout", () => { + describe("rule references", () => { + it("preserves an optional rule-reference suffix", () => { + const source = " = ? world;"; + + expect(fmt(source)).toBe(`${source}\n`); + roundTrip(source); + }); + + it("does not turn a separated question mark into a suffix", () => { + roundTrip(" = ? world;"); + roundTrip(" = /* comment */ ? world;"); + }); + + it("keeps an adjacent escaped question mark literal", () => { + roundTrip(" = \\? world;"); + }); + }); + describe("rule alternatives — flat vs broken", () => { it("single alt always flat", () => { // No alternates — always stays on one line diff --git a/ts/packages/actionGrammar/test/nfaDfaParity.spec.ts b/ts/packages/actionGrammar/test/nfaDfaParity.spec.ts index 0bed6ac683..011b3063e8 100644 --- a/ts/packages/actionGrammar/test/nfaDfaParity.spec.ts +++ b/ts/packages/actionGrammar/test/nfaDfaParity.spec.ts @@ -485,6 +485,88 @@ describe("NFA/DFA Parity", () => { }); }); + describe("optional rule reference", () => { + const { grammar, nfa, dfa } = compile( + "optionalRuleReference", + ` + = add $(item:wildcard) to ? $(playlist:wildcard) playlist + -> { actionName: "add", parameters: { item, playlist } }; + = the | my; + `, + ); + + it.each([ + ["add track to the favorites playlist"], + ["add track to my favorites playlist"], + ["add track to favorites playlist"], + ])("matches '%s' in both matchers", (request) => { + assertMatchParity(grammar, nfa, dfa, request); + expect(matchGrammarWithNFA(grammar, nfa, request)).not.toHaveLength( + 0, + ); + expect( + matchDFAWithSplitting(dfa, tokenizeRequest(request)).matched, + ).toBe(true); + }); + + it("does not make an unsuffixed rule reference optional", () => { + const required = compile( + "requiredRuleReference", + ` + = add $(item:wildcard) to $(playlist:wildcard) playlist + -> { actionName: "add", parameters: { item, playlist } }; + = the | my; + `, + ); + + expect( + matchGrammarWithNFA( + required.grammar, + required.nfa, + "add track to favorites playlist", + ), + ).toHaveLength(0); + expect( + matchDFAWithSplitting( + required.dfa, + tokenizeRequest("add track to favorites playlist"), + ).matched, + ).toBe(false); + }); + + it.each(["the play", "play"])( + "keeps AST evaluation in parity for '%s'", + (request) => { + const noCaptures = compile( + "optionalRuleReferenceAST", + ` + = ? play -> { actionName: "play" }; + = the | my; + `, + ); + + assertASTMatchParity( + noCaptures.grammar, + noCaptures.nfa, + noCaptures.dfa, + request, + ); + }, + ); + + it("offers both the optional rule and the following wildcard", () => { + const completions = getDFACompletions(dfa, ["add", "track", "to"]); + + expect(completions.completions).toEqual( + expect.arrayContaining(["the", "my"]), + ); + expect(completions.properties).toContainEqual({ + actionName: "add", + propertyPath: "parameters.playlist", + }); + }); + }); + // ----------------------------------------------------------------------- // 7. Kleene plus (one-or-more) // ----------------------------------------------------------------------- @@ -1813,6 +1895,53 @@ describe("PhraseSet Completion Parity", () => { `, ); + const optionalPhraseSet = compile( + "optionalPhraseSetReference", + ` + = ? schedule $(desc:wildcard) + -> { actionName: "scheduleEvent", parameters: { desc } }; + `, + ); + + it.each(["please schedule meeting", "schedule meeting"])( + "matches an optional phrase-set reference in '%s'", + (request) => { + assertMatchParity( + optionalPhraseSet.grammar, + optionalPhraseSet.nfa, + optionalPhraseSet.dfa, + request, + ); + assertASTMatchParity( + optionalPhraseSet.grammar, + optionalPhraseSet.nfa, + optionalPhraseSet.dfa, + request, + ); + expect( + matchGrammarWithNFA( + optionalPhraseSet.grammar, + optionalPhraseSet.nfa, + request, + ), + ).not.toHaveLength(0); + expect( + matchDFAWithSplitting( + optionalPhraseSet.dfa, + tokenizeRequest(request), + ).matched, + ).toBe(true); + }, + ); + + it("suggests both the optional phrase set and the following token", () => { + const completions = getDFACompletions(optionalPhraseSet.dfa, []); + + expect(completions.completions).toEqual( + expect.arrayContaining(["please", "schedule"]), + ); + }); + it("empty prefix: DFA suggests phraseSet first-tokens", () => { const comp = getDFACompletions(dfa, []); const literals = [...(comp.completions ?? [])].sort();