Fix unsound Ok() switch guard by demoting all-residual conditional discriminants#271
Open
AaronWebster wants to merge 2 commits into
Open
Fix unsound Ok() switch guard by demoting all-residual conditional discriminants#271AaronWebster wants to merge 2 commits into
AaronWebster wants to merge 2 commits into
Conversation
…scriminants The optimized Ok() switch reads its discriminant once and guards it with `if (!discrim.Known()) return false;`. That guard is unsound when the discriminant is itself a conditionally-present field AND every arm carries a residual: a valid message can have an out-of-bounds (Unknown) discriminant while every arm's residual is statically false, so every has_X() is Known-false (absent) and the message is valid -- but the blanket guard rejects it. Fix it in the demotion pass rather than with a bespoke guarded-switch template: an all-residual, not-provably-present switch group is demoted to plain per-field has_X() checks, which handle an Unknown discriminant correctly (an Unknown discriminant can never make has_X() Known-true, so a Known-false residual simply leaves the field absent). Groups with at least one bare arm keep the sound Known()-guarded switch. This is latent on the current corpus -- no existing schema produces the shape -- so every existing golden is byte-for-byte identical; only the new condition.emb regression structs add codegen. Supersedes the guarded-switch approach in the earlier fix-conditional-switch-guard branch with a simpler one (no new template, no fallback path). Adds regression structs to condition.emb (ResidualConditionalDiscriminant, BareConditionalDiscriminant, DominatedBareDiscriminant, DisjunctionConditionalDiscriminant, SingleEntryConditionalDiscriminant, EnumConditionalDiscriminant) with tests in condition_test.cc covering the discriminant absent / present-but-truncated / gated-field / no-arm-match cases for both the demoted and the still-guarded shapes.
This was referenced Jul 19, 2026
This was referenced Jul 19, 2026
robrussell
approved these changes
Jul 20, 2026
| # IsComplete() is true. Ok() must still be false: a/b's existence is | ||
| # indeterminate, and the discriminant Known() guard -- not IsComplete() -- | ||
| # is what enforces that. (A `if (has_tag().ValueOrDefault())` wrapper would | ||
| # wrongly accept this message.) |
Member
There was a problem hiding this comment.
This last phrase is unclear
"(A if (has_tag().ValueOrDefault()) wrapper would wrongly accept this message.)"
Does that reflect the state before this PR or after? Is that intended behaviour? Some of the explanations in the comments for these tests might be helpful in the language docs (either as clarifications or examples).
…() with conditional fields - Remove "optimized"/"guarded switch"/demotion codegen internals and the confusing hypothetical-wrapper aside from testdata/condition.emb struct comments; describe schema shape and required observable behavior instead. - Drop bare #NNN PR references from condition_test.cc comments. - Make the demotion-pass comment in header_generator.py timeless (the "latent on the current test corpus" claim is untrue once condition.emb exercises the shape) and use _ for the unused loop variable. - Document Ok() semantics for conditional fields in doc/cpp-guide.md: absent field != invalid, indeterminate existence -> Ok() false even when IsComplete() is true.
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.
Problem
The optimized
Ok()switch (introduced in #253, extended in #256/#257) reads itsdiscriminant once and emits, as a safety net:
When the discriminant is itself a conditionally-present field read from a buffer too
short to contain it, it reads as
Unknown, so this guard makesOk()returnfalse—even though the message is valid, because the fields gated on the (absent) discriminant
simply don't exist. This is latent on
master: no existing test schema produces theshape.
Same bug that #266 fixes, reached by a simpler route.
Fix
Instead of #266's dedicated
ok_method_switch_block_guardedtemplate, add one rule tothe demotion pass in
_generate_optimized_ok_method_body: a switch group that is notprovably-known and has no bare arm (every entry carries a residual) is demoted to
per-field
has_X()checks — the existingok_method_testpath — instead of beingemitted as a guarded
switch. After this,_emit_switch_blockonly ever seesprovably-known or bare-arm-guarded forms, so the unsound guarded-switch template is
deleted outright.
Soundness is observationally identical to #266's guarded fallback: with the discriminant
Unknown, a statically-false residual makeshas_X()Known-false (field absent → valid,the exact case #266 fixed), otherwise
has_X()isUnknown→Ok()returns false; withthe discriminant
Knownit computes the fullhas_X(), same as a dispatched arm minusthe pointless
switch.Verification
confined to
testdata/golden_cpp/condition.emb.h.testdata/condition.emb+compiler/back_end/cpp/testcode/condition_test.cc); assertions unchanged, commentsreworded per review.
bazel test //...green (206/206).correctness fix, not a size optimization.
Supersedes
Supersedes #266 (same bug, simpler fix; #266's regression tests are carried over here).