fix(model): reject invalid conformance choice/optionality constructs#3991
Merged
Conversation
The conformance DSL parser silently mis-parsed three constructs that are invalid per the spec grammar: range-form choice (§7.3.14, e.g. "a2-4"), mixed "+"/"-" choice modifiers (§7.3.14), and partial optionality (§7.3.4, e.g. "[AA] & BB"). The parser now emits INVALID_CHOICE / INVALID_OPTIONALITY errors for these instead of producing a wrong AST, so any future spec/model authoring bug surfaces loudly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens the @matter/model conformance DSL parser to explicitly reject a few invalid Matter-spec grammar constructs that were previously being mis-parsed into an incorrect AST, ensuring spec/model authoring errors surface via conformance.error(...). It adds targeted tests for the newly rejected forms and documents the change in the changelog.
Changes:
- Update conformance parsing to reject (and drain) invalid choice suffix patterns (mixed/repeated
+/-, and unsupported “range-form” trailing numbers). - Detect and reject “partial optionality” where a top-level
[...]group is combined with a binary operator (e.g.[AA] & BB). - Add test coverage for the reject cases and add a
@matter/modelchangelog entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/model/src/aspects/Conformance.ts | Adds explicit erroring and recovery logic for invalid choice/range and partial-optional constructs. |
| packages/model/test/aspects/ConformanceTest.ts | Adds tests for invalid constructs and expands valid round-trip definitions for - choice suffix. |
| CHANGELOG.md | Notes the conformance parser behavior change (and also contains an unrelated edit needing confirmation). |
…ty error The partial-optionality recovery drained to the next comma, but whitespace also separates top-level conformance terms, so "[AA] & BB CC" discarded the valid "CC". Drain only the offending binary expression (operator + operands) so following terms still parse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The modifier error fires for repeated modifiers too (e.g. "a2++"), so "but not both" was misleading; reword to "at most one modifier". Reword the range-form error from "not supported" to "invalid" and mention both "a2-4" and "a2+4" forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apollon77
added
the
automerge
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The conformance DSL parser silently mis-parsed three constructs that are invalid per the Matter spec grammar. It now rejects them via
conformance.error(...)instead of producing a wrong AST, so any future spec/model authoring bug surfaces loudly.O.a2-4(range-form choice, §7.3.14)"O.a2-, 4"silentlyINVALID_CHOICEAB.a2+-(mixed+/-, §7.3.14)"AB.a2"silentlyINVALID_CHOICE[AA] & BB(partial optionality, §7.3.4)"[AA], BB"+ misleading token errorINVALID_OPTIONALITYThese forms are latent in the 1.6 model (zero occurrences); the change reverses the prior lenient-parser stance for these specific invalid forms only.
Implementation
+/-are now mutually exclusive; a repeated/second sign emits a mixed-modifier error and is drained. A barevaluetoken trailing a choice is flagged as an unsupported range form.[...]group is flagged as partial optionality and drained to the next separator.Valid forms still parse and round-trip:
AB.a,AB.a+,AB.a-,AB.a2,AB.a2+,AB.a2-,[AB].a,[X | Y].a+, O, qualified names.Verification
@matter/modelsuite green.ValidateModel: 0 total errors, 0 new-error hits — confirms latent, no real strings affected.format-verify,lint(oxlint),build --cleanall clean.🤖 Generated with Claude Code