Type for consume token sometimes optional even though not

[jonestristand]

Very much an edge case, but just noticed that the generated types are slightly off: If a token is consumed in any path of an option, it's not optional - it will always exist. In the produced type file, however, the field will be listed as optional type (i.e. may be undefined) which leads to extra type checking vs. typescript errors :-)

ex.:

export interface AmountCstNode extends CstNode {
  name: "amount";
  children: AmountCstChildren;
}

export type AmountCstChildren = {
  DASH?: IToken[];
  CommodityText?: (IToken)[];
  Number?: (IToken)[];
};

[bd82]

Hello @jonestristand

What do you expect the generated type definitions to be in this case?

[jonestristand]

I would expect it to be:

export type AmountCstChildren = {
  DASH?: IToken[];
  CommodityText?: IToken[];
  Number: IToken[];
};

Though the logic for sorting this out would be more complicated, but I think that it should be possible by walking all possible routes in the graph, and listing consumed tokens. The problem would be that for large rules the time-complexity to that approach scales poorly...

[bd82]

I'm not sure if this analysis complexity would be more than linear. But I am also not sure how important this is and/or worth the effort to fix and maintain, because normally once the optional parts get too complex, you would split them up into separate grammar rules, e.g:

• https://github.com/Chevrotain/chevrotain/blob/master/examples/grammars/ecma5/ecma5_parser.js#L359-L387

So this issue would "disappear" if you utilize that pattern.

[jonestristand]

Yeah fair enough, I agree it's very much a non-essential edge-case, just bothered me because it wasn't correct :-D

I think in the case of my example the rule is small enough and logically 'the same' enough, that it makes sense to write as is, or do you think it would be better written (more 'canonical') as two separate rules even for this kind of case?

[bd82]

Your rule is small enough and you likely extract the same data from either alternative so it is fine to keep it all in one rule.

On a related note, in the case of error recovery, everything could be optional. Perhaps the dts generation API should expose an option to make everything optional

• https://chevrotain.io/documentation/10_3_0/types/GenerateDtsOptions.html

[bd82]

I'll close this as its a minor edge case which may be still be valid for error recovery scenarios.