From f18d1795a6d4b25886cc7415a0bd308e72c9c93a Mon Sep 17 00:00:00 2001 From: Mikita Taukachou Date: Thu, 30 Apr 2026 00:41:46 +0200 Subject: [PATCH] fix: reject multi-sub-roll group sort at parse time #101 Added a multi-sub-roll `Group` target reject in `parseSort` after the deep dice-pool guard, throwing `INVALID_SORT_TARGET` with a deferred-status message via `unwrapTransparent` (the helper from #92). Removed the `should accept multi-sub-roll group` parser shape test and added `errors` cases for `{1d6, 2d8}s` and the parens-wrapped `({1d6, 2d8})s` variant. Refreshed the `evalSort` JSDoc to state that multi-sub-roll Groups are rejected at parse time instead of falling through to the flat-pool path. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/evaluator/evaluator.ts | 6 +++--- src/parser/parser.test.ts | 25 +++++++++++++++++++------ src/parser/parser.ts | 17 +++++++++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/evaluator/evaluator.ts b/src/evaluator/evaluator.ts index 34b4adc..53b3aaf 100644 --- a/src/evaluator/evaluator.ts +++ b/src/evaluator/evaluator.ts @@ -801,9 +801,9 @@ function evalReroll(node: RerollNode, rng: RNG, ctx: EvalContext, env: EvalEnv): * * Rendering mirrors `evalExplode` / `evalModifier`: emits * `[]`, replacing any inline dice brackets - * the target itself rendered. Multi-sub-roll groups (`{4d6, 3d6}s`) fall - * through to this flat-pool path — hierarchical per-sub-roll sorting is - * deferred (see Stage 3 spec §3 "Group interaction"). + * the target itself rendered. Multi-sub-roll Group targets (`{4d6, 3d6}s`) + * are rejected at parse time with `INVALID_SORT_TARGET` until hierarchical + * per-sub-roll sorting (Stage 3 spec §3 "Group interaction") is implemented. */ function evalSort(node: SortNode, rng: RNG, ctx: EvalContext, env: EvalEnv): number { const targetCtx: EvalContext = { rolls: [], expressionParts: [], renderedParts: [] }; diff --git a/src/parser/parser.test.ts b/src/parser/parser.test.ts index afcb066..4d9dd8b 100644 --- a/src/parser/parser.test.ts +++ b/src/parser/parser.test.ts @@ -2196,12 +2196,6 @@ describe('Parser', () => { it('should accept single-sub-roll group', () => { expect(parse('{4d6}s')).toEqual(sort('ascending', group([dice(literal(4), literal(6))]))); }); - - it('should accept multi-sub-roll group', () => { - expect(parse('{4d6, 3d6}sd')).toEqual( - sort('descending', group([dice(literal(4), literal(6)), dice(literal(3), literal(6))])), - ); - }); }); describe('errors', () => { @@ -2219,6 +2213,25 @@ describe('Parser', () => { expect(() => parse('(1+2)sd')).toThrow(ParseError); }); + it('should reject sort on a multi-sub-roll group', () => { + expect(() => parse('{1d6, 2d8}s')).toThrow(ParseError); + try { + parse('{1d6, 2d8}s'); + } catch (e) { + expect((e as ParseError).code).toBe('INVALID_SORT_TARGET'); + expect((e as ParseError).message).toContain('not yet support'); + } + }); + + it('should reject sort on a parens-wrapped multi-sub-roll group', () => { + expect(() => parse('({1d6, 2d8})s')).toThrow(ParseError); + try { + parse('({1d6, 2d8})s'); + } catch (e) { + expect((e as ParseError).code).toBe('INVALID_SORT_TARGET'); + } + }); + it('should reject sort on SuccessCount target', () => { expect(() => parse('4d6>=4s')).toThrow(ParseError); try { diff --git a/src/parser/parser.ts b/src/parser/parser.ts index 947008e..18deb37 100644 --- a/src/parser/parser.ts +++ b/src/parser/parser.ts @@ -716,6 +716,23 @@ export class Parser { ); } + // ! Multi-sub-roll groups (`{a, b}s`, `({a, b})s`) need hierarchical + // sort per Stage 3 spec §3 (sort dice within each sub-roll, then sort + // sub-rolls by total) — `evalSort` only flat-sorts, so accepting the + // syntax would silently ship non-spec behaviour. Reject at parse time + // until the deferred Stage 4 implementation lands. Single-sub Groups + // keep passing through (the unwrap returns a `Group` with one + // expression, which is the user's flat-pool escape hatch). + const base = unwrapTransparent(target, ['Grouped', 'Modifier', 'Sort', 'CritThreshold']); + if (base.type === 'Group' && base.expressions.length >= 2) { + throw new ParseError( + `Sort modifier does not yet support multi-sub-roll groups`, + 'INVALID_SORT_TARGET', + token.position, + token, + ); + } + const order: SortNode['order'] = token.type === TokenType.SORT_ASC ? 'ascending' : 'descending'; // ? Chained sorts (`4d6ss`, `4d6sasd`) are allowed — sort is idempotent