feat(strict): add DEFAULT_MATCHES_FIRST_ALLOWED check for default==allowed[0] - #512
Merged
erikbosch merged 3 commits intoApr 30, 2026
Conversation
…lowed[0] Closes COVESA#507. Follow-up from COVESA#502 (proto3 nested-enum generation for string fields with allowed values). Proto3 uses allowed[0] as the implicit wire-unset default, so VSS default must match allowed[0] for the intent to survive serialization. Canonical VSS pattern per erikbosch + sschleemilch (see COVESA#507 comments): allowed: ['UNKNOWN', ...] + default: 'UNKNOWN'. Implementation mirrors existing NAME_STYLE / UNKNOWN_ATTRIBUTE strict checks: - StrictOption.DEFAULT_MATCHES_FIRST_ALLOWED enum member - StrictExceptions.defaults exemption set - load_strict_exceptions dispatch (+ null-options fallback) - VSSNode.get_default_first_allowed_violations tree walk - check_default_first_allowed_violations strict gate in main.py - DefaultFirstAllowedException signal class - aborts CLI choice extended Gated behind --strict or --aborts default-matches-first-allowed; exempt per-FQN via the existing --strict-exceptions file (new option value or null for all-exceptions). No behavior change on default run. Tests: 5 new (1 strict.py enum + dispatch, 4 tree.py violation collector covering clean/mismatch/no-allowed/no-default), plus existing strict.py tests extended with .defaults-set assertions for consistency. 132/132 pass on tests/ (pre-existing test_binary failure unrelated). Chain A: COVESA#502 -> COVESA#507. Signed-off-by: Akihiko Komada <aki1770@gmail.com>
…ruff-format
Array datatypes (`string[]` etc.) don't have a proto3 nested-enum analog
for the default-equals-allowed[0] rule — per-element allowed-membership
is already enforced at the pydantic VSSDataDatatype layer. Previous
implementation incorrectly flagged
`tests/vspec/test_datatypes_pattern/test_pattern_ok.vspec` (A.Colors:
string[] with allowed=[white,...,blue] and default=[white,green,red])
as a violation because default (a list) != allowed[0] (a scalar).
Adds early-continue on `node.data.datatype.endswith("[]")` and a
regression test covering the exact fixture shape.
Also applies ruff-format on main.py and cli_options.py to satisfy the
pre-commit ruff-format hook (whitespace-only changes).
Signed-off-by: Akihiko Komada <aki1770@gmail.com>
Signed-off-by: Akihiko Komada <aki1770@gmail.com>
sschleemilch
approved these changes
Apr 25, 2026
Contributor
Author
|
Thanks, Sebastian. Should I land this myself, or are you handling the merge? |
Collaborator
|
MoM:
|
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
3 participants
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.
Closes #507.
Per @erikbosch (#507 comment 2026-04-15): "I think check b (i.e. that default value matches first value) at least should be part of the 'strict' check we have". Per @sschleemilch: original proposal (a) "default ∈ allowed" is already enforced in pydantic
VSSDataDatatype(seetests/test_model.py#L39), so this PR lands only the narrowed (b) check.Why this matters
Proto3 nested-enum generation from
allowed(landed in #502) usesallowed[0]as the implicit wire-unset default value — proto3 has no mechanism to set a non-zero default for strings or ints. If VSSdefaultdoes not matchallowed[0], the serialized-and-deserialized value silently becomesallowed[0], diverging from the VSS author's declared intent.The canonical VSS pattern is:
See
Powertrain/CombustionEngine.vspecConfigurationfor an in-tree example (referenced in #507 body).What this PR does
Adds a new strict-check option, parallel to the existing
NAME_STYLEandUNKNOWN_ATTRIBUTEmachinery:StrictOption.DEFAULT_MATCHES_FIRST_ALLOWEDenum memberStrictExceptions.defaultsper-FQN exemption setload_strict_exceptionsdispatch for the new option (including the null-options fallback that exempts an FQN from all strict checks)VSSNode.get_default_first_allowed_violations()tree walkercheck_default_first_allowed_violations()enforcement inget_root()DefaultFirstAllowedExceptionsignal--abortsCLI choice extended withdefault-matches-first-allowedGated behind
--strictor--aborts default-matches-first-allowed. Existing runs without either flag are unchanged — no behavior regression.Error message
When a violation is detected (under strict/abort mode), the warning is:
Testing
test_strict.pyfor the new enum + dispatch; 4 intest_vss_node.pyfor the tree-walk violation collector covering clean / mismatch / no-allowed / no-default branches)test_strict.pytests extended with.defaultsset assertions for consistency with.names/.attributestests/ --ignore=tests/vspec --ignore=tests/binary(pre-existingtest_binaryfailure verified unrelated — reproduces on cleanmaster)ruff check src/ tests/test_strict.py tests/test_vss_node.pycleanChain context
Chain A: #502 → #507. PR #502 adds the proto3 nested-enum exporter that creates the default-vs-allowed divergence risk; this PR adds the strict validation that prevents authors from committing VSS specs with that silent-divergence.