Fix unsound Ok() switch guard by demoting all-residual conditional discriminants

compiler/back_end/cpp/header_generator.py

@@ -1679,14 +1679,33 @@ def _generate_optimized_ok_method_body(fields, ir, subexpressions):
             groups[key]["fields"].append(field)
             field_group_key[id(field)] = key
 
-    # Demote switch groups that wouldn't actually benefit from a switch.
-    # A switch's overhead — the temporary discriminant variable, the
-    # Known() guard, the scope braces — is only worthwhile if the switch
-    # dedupes either at least two distinct case labels (sharing one
-    # subexpression read) or at least two distinct fields (sharing one
-    # discriminant evaluation). When a group has just one entry total
-    # (one field, one bare case), the unoptimized has_${field}()-based
-    # check is strictly smaller.
+    # Demote switch groups back to per-field checks in two cases.
+    #
+    # (1) Size: a switch's overhead — the temporary discriminant variable,
+    #     the Known() guard, the scope braces — is only worthwhile if the
+    #     switch dedupes either at least two distinct case labels (sharing
+    #     one subexpression read) or at least two distinct fields (sharing
+    #     one discriminant evaluation). When a group has just one entry
+    #     total (one field, one bare case), the unoptimized has_${field}()-
+    #     based check is strictly smaller.
+    #
+    # (2) Correctness: the switch reads its discriminant once and guards it
+    #     with `if (!discrim.Known()) return false;`. That guard is only
+    #     sound when the discriminant is provably present OR the group has
+    #     at least one *bare* arm (existence condition exactly `discrim ==
+    #     K`, no residual). With a bare arm, an Unknown (out-of-bounds)
+    #     discriminant leaves that field's has_X() Unknown, so the
+    #     unoptimized Ok() bails for the same reason the guard does. But
+    #     when the discriminant is itself a conditionally-present field and
+    #     *every* arm carries a residual, a valid message can have an
+    #     Unknown discriminant while every arm's residual is statically
+    #     false — making every has_X() Known-false (absent). The blanket
+    #     guard would wrongly reject it. Demote that shape to per-field
+    #     has_${field}() checks, which handle an Unknown discriminant
+    #     correctly (an Unknown discriminant can never make has_X()
+    #     Known-true, so a Known-false residual leaves the field simply
+    #     absent). This is latent on the current test corpus — no existing
+    #     schema produces the shape — so no existing golden changes.
     for key in ordered_keys:
         group = groups[key]
         if group["type"] != "switch":
@@ -1697,6 +1716,16 @@ def _generate_optimized_ok_method_body(fields, ir, subexpressions):
         )
         if total_entries < 2:
             group["type"] = "demoted_to_if"
+            continue
+        has_bare_arm = any(
+            not has_residual
+            for case_entry in group["cases_by_label"].values()
+            for (_field, has_residual) in case_entry["entries"]
+        )
+        if not has_bare_arm and not _is_discriminant_provably_known(
+            group["discrim_expr"], fields
+        ):
+            group["type"] = "demoted_to_if"
 
     blocks = []
     for key in ordered_keys:

compiler/back_end/cpp/testcode/condition_test.cc

@@ -300,6 +300,217 @@ TEST(Conditional, StructWithNestedConditionIsOkWhenOuterConditionExists) {
   EXPECT_EQ(2U, writer.SizeInBytes());
 }
 
+// Regression tests for the optimized Ok() switch when the discriminant (`tag`)
+// is itself a conditionally-present field.  In ResidualConditionalDiscriminant
+// every arm carries a residual (`outer == 1`), so when `outer != 1` the message
+// is valid even though `tag` is absent -- and the optimized Ok() must not
+// reject it just because the (out-of-bounds) discriminant reads as Unknown.
+TEST(Conditional,
+     ResidualConditionalDiscriminantOkWhenDiscriminantFieldIsAbsent) {
+  // outer == 0, so `tag` is absent and `a`/`b` cannot exist.  The 1-byte
+  // buffer is too short to read `tag`, but the message is still valid.
+  ::std::uint8_t buffer[1] = {0};
+  auto writer = ResidualConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_TRUE(writer.has_tag().Known());
+  EXPECT_FALSE(writer.has_tag().Value());
+  EXPECT_TRUE(writer.has_a().Known());
+  EXPECT_FALSE(writer.has_a().Value());
+  EXPECT_TRUE(writer.has_b().Known());
+  EXPECT_FALSE(writer.has_b().Value());
+}
+
+TEST(Conditional,
+     ResidualConditionalDiscriminantNotOkWhenDiscriminantPresentButTruncated) {
+  // outer == 1, so `tag` is present, but the buffer is too short to read it:
+  // whether `a`/`b` exist is indeterminate, so the message is invalid.
+  ::std::uint8_t buffer[1] = {1};
+  auto writer = ResidualConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_FALSE(writer.Ok());
+  EXPECT_TRUE(writer.has_tag().Known());
+  EXPECT_TRUE(writer.has_tag().Value());
+  EXPECT_FALSE(writer.has_a().Known());
+}
+
+TEST(Conditional, ResidualConditionalDiscriminantOkWhenGatedFieldIsPresent) {
+  // outer == 1, tag == 0, so `a` exists and is in bounds; `b` is absent.
+  ::std::uint8_t buffer[3] = {1, 0, 7};
+  auto writer = ResidualConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_TRUE(writer.has_a().Known());
+  EXPECT_TRUE(writer.has_a().Value());
+  EXPECT_TRUE(writer.has_b().Known());
+  EXPECT_FALSE(writer.has_b().Value());
+  EXPECT_EQ(7, writer.a().Read());
+}
+
+TEST(Conditional, ResidualConditionalDiscriminantNotOkWhenGatedFieldTruncated) {
+  // outer == 1, tag == 0, so `a` exists, but the buffer is too short for it.
+  ::std::uint8_t buffer[2] = {1, 0};
+  auto writer = ResidualConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.has_a().Known());
+  EXPECT_TRUE(writer.has_a().Value());
+  EXPECT_FALSE(writer.a().Ok());
+  EXPECT_FALSE(writer.Ok());
+}
+
+TEST(Conditional, ResidualConditionalDiscriminantOkWhenDiscriminantMatchesNoArm) {
+  // outer == 1, tag == 5: neither `a` nor `b` exists, so the message is valid.
+  ::std::uint8_t buffer[2] = {1, 5};
+  auto writer = ResidualConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_FALSE(writer.has_a().Value());
+  EXPECT_FALSE(writer.has_b().Value());
+}
+
+// BareConditionalDiscriminant keeps the simple discriminant Known() guard:
+// the arms are bare (`if tag == K:`), so an Unknown `tag` leaves has_a()/
+// has_b() Unknown and Ok() must report the message as invalid.
+TEST(Conditional, BareConditionalDiscriminantNotOkWhenDiscriminantTruncated) {
+  // 1-byte buffer: `tag` (offset 1) is out of bounds, so has_a()/has_b() are
+  // Unknown and the message's validity cannot be determined.
+  ::std::uint8_t buffer[1] = {0};
+  auto writer = BareConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_FALSE(writer.Ok());
+  EXPECT_FALSE(writer.has_a().Known());
+  EXPECT_FALSE(writer.has_b().Known());
+}
+
+TEST(Conditional, BareConditionalDiscriminantOkWhenGatedFieldIsPresent) {
+  // outer == 1 (so `tag` is present) and tag == 0, so `a` exists and is in
+  // bounds.
+  ::std::uint8_t buffer[3] = {1, 0, 7};
+  auto writer = BareConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_TRUE(writer.has_a().Known());
+  EXPECT_TRUE(writer.has_a().Value());
+  EXPECT_EQ(7, writer.a().Read());
+}
+
+TEST(Conditional, BareConditionalDiscriminantNotOkWhenGatedFieldTruncated) {
+  // outer == 1, tag == 0, so `a` exists, but the buffer is too short for it.
+  ::std::uint8_t buffer[2] = {1, 0};
+  auto writer = BareConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.has_a().Known());
+  EXPECT_TRUE(writer.has_a().Value());
+  EXPECT_FALSE(writer.Ok());
+}
+
+// Distinguishing test: a bare arm on a conditional discriminant whose field is
+// size-dominated by an always-present field.  IsComplete() stays true, so only
+// the discriminant Known() guard can reject the message.  This is the case
+// where a `if (has_tag().ValueOrDefault())` wrapper diverges (it would accept).
+TEST(Conditional, DominatedBareDiscriminantNotOkWhenDiscriminantAbsent) {
+  ::std::uint8_t buffer[4] = {0, 0, 0, 9};
+  auto writer = DominatedBareDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.IsComplete());
+  EXPECT_FALSE(writer.has_a().Known());
+  EXPECT_FALSE(writer.has_b().Known());
+  EXPECT_FALSE(writer.Ok());
+}
+
+TEST(Conditional, DominatedBareDiscriminantOkWhenDiscriminantPresent) {
+  // outer == 1, tag == 0: `a` present and readable; `tail` present.
+  ::std::uint8_t buffer[4] = {1, 0, 5, 9};
+  auto writer = DominatedBareDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_TRUE(writer.has_a().Value());
+  EXPECT_EQ(5, writer.a().Read());
+  EXPECT_EQ(9, writer.tail().Read());
+}
+
+// Disjunction arms (#256 matcher) on a conditional discriminant, all carrying a
+// residual -> the residual-only guarded switch with coalesced case labels.
+TEST(Conditional, DisjunctionConditionalDiscriminantOkWhenDiscriminantAbsent) {
+  ::std::uint8_t buffer[1] = {0};
+  auto writer = DisjunctionConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_FALSE(writer.has_a().Value());
+  EXPECT_FALSE(writer.has_b().Value());
+}
+
+TEST(Conditional, DisjunctionConditionalDiscriminantBothDisjunctsSelectField) {
+  // tag == 0 and tag == 1 both make `a` present.
+  ::std::uint8_t buffer0[3] = {1, 0, 7};
+  auto w0 = DisjunctionConditionalDiscriminantWriter(buffer0, sizeof buffer0);
+  EXPECT_TRUE(w0.Ok());
+  EXPECT_TRUE(w0.has_a().Value());
+  EXPECT_EQ(7, w0.a().Read());
+
+  ::std::uint8_t buffer1[3] = {1, 1, 8};
+  auto w1 = DisjunctionConditionalDiscriminantWriter(buffer1, sizeof buffer1);
+  EXPECT_TRUE(w1.Ok());
+  EXPECT_TRUE(w1.has_a().Value());
+  EXPECT_EQ(8, w1.a().Read());
+}
+
+TEST(Conditional, DisjunctionConditionalDiscriminantSecondArmAndNoMatch) {
+  // tag == 2 selects `b` (which overlaps `a` at offset 2); tag == 9 selects
+  // neither (still valid).
+  ::std::uint8_t buffer2[3] = {1, 2, 6};
+  auto w2 = DisjunctionConditionalDiscriminantWriter(buffer2, sizeof buffer2);
+  EXPECT_TRUE(w2.Ok());
+  EXPECT_TRUE(w2.has_b().Value());
+  EXPECT_EQ(6, w2.b().Read());
+
+  ::std::uint8_t buffer9[2] = {1, 9};
+  auto w9 = DisjunctionConditionalDiscriminantWriter(buffer9, sizeof buffer9);
+  EXPECT_TRUE(w9.Ok());
+  EXPECT_FALSE(w9.has_a().Value());
+  EXPECT_FALSE(w9.has_b().Value());
+}
+
+// Single-entry group on a conditional discriminant: demoted to a has_X() check
+// (#256), so no discriminant guard is emitted and the bug cannot arise.
+TEST(Conditional, SingleEntryConditionalDiscriminantOkWhenDiscriminantAbsent) {
+  ::std::uint8_t buffer[1] = {0};
+  auto writer = SingleEntryConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_TRUE(writer.has_a().Known());
+  EXPECT_FALSE(writer.has_a().Value());
+}
+
+TEST(Conditional, SingleEntryConditionalDiscriminantOkWhenPresent) {
+  ::std::uint8_t buffer[3] = {1, 0, 4};
+  auto writer = SingleEntryConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_TRUE(writer.has_a().Value());
+  EXPECT_EQ(4, writer.a().Read());
+}
+
+TEST(Conditional,
+     SingleEntryConditionalDiscriminantNotOkWhenDiscriminantTruncated) {
+  // outer == 1 but the buffer is too short for tag: a's existence is Unknown.
+  ::std::uint8_t buffer[1] = {1};
+  auto writer = SingleEntryConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_FALSE(writer.has_a().Known());
+  EXPECT_FALSE(writer.Ok());
+}
+
+// Enum-typed conditional discriminant with residual arms (static_cast case
+// labels in the guarded switch).
+TEST(Conditional, EnumConditionalDiscriminantOkWhenDiscriminantAbsent) {
+  ::std::uint8_t buffer[1] = {0};  // outer == OFF
+  auto writer = EnumConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_FALSE(writer.has_a().Value());
+  EXPECT_FALSE(writer.has_b().Value());
+}
+
+TEST(Conditional, EnumConditionalDiscriminantOkWhenPresent) {
+  ::std::uint8_t buffer[3] = {1, 0, 7};  // outer == ON, tag == OFF -> a present
+  auto writer = EnumConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_TRUE(writer.Ok());
+  EXPECT_TRUE(writer.has_a().Value());
+  EXPECT_EQ(7, writer.a().Read());
+}
+
+TEST(Conditional, EnumConditionalDiscriminantNotOkWhenDiscriminantTruncated) {
+  ::std::uint8_t buffer[1] = {1};  // outer == ON, tag truncated
+  auto writer = EnumConditionalDiscriminantWriter(buffer, sizeof buffer);
+  EXPECT_FALSE(writer.Ok());
+}
+
 TEST(Conditional, AlwaysMissingFieldDoesNotContributeToStaticSize) {
   EXPECT_EQ(0U, OnlyAlwaysFalseConditionWriter::SizeInBytes());
   EXPECT_EQ(1U, AlwaysFalseConditionWriter::SizeInBytes());

testdata/condition.emb

@@ -229,3 +229,99 @@ struct ConditionalOnFlag:
     0 [+1]  Flag  enabled
   if enabled:
     1 [+1]  UInt  value
+
+
+struct ResidualConditionalDiscriminant:
+  # `tag` is itself conditional, and is the switch discriminant for `a`/`b`
+  # (it is the first conjunct of their existence conditions).  Every arm
+  # carries a residual (`outer == 1`), so when `outer != 1` the message is
+  # valid even though `tag` is absent -- and on a buffer too short to hold
+  # `tag`, reading the discriminant is Unknown.  The optimized Ok() must not
+  # reject such a message.
+  0 [+1]    UInt  outer
+  if outer == 1:
+    1 [+1]  UInt  tag
+
+  if tag == 0 && outer == 1:
+    2 [+1]  UInt  a
+
+  if tag == 1 && outer == 1:
+    2 [+1]  UInt  b
+
+
+struct BareConditionalDiscriminant:
+  # Like above, but `a`/`b` reference the conditional discriminant `tag`
+  # without re-guarding `outer == 1`, so the switch arms are bare.  Here the
+  # discriminant Known() guard is sound: an Unknown `tag` leaves has_a()/
+  # has_b() Unknown, so Ok() correctly reports the message as invalid.
+  0 [+1]    UInt  outer
+  if outer == 1:
+    1 [+1]  UInt  tag
+
+  if tag == 0:
+    2 [+1]  UInt  a
+
+  if tag == 1:
+    2 [+1]  UInt  b
+
+
+struct DominatedBareDiscriminant:
+  # Bare arms on a conditional discriminant, plus an always-present `tail`
+  # field that dominates the structure size.  When `outer != 1`, `tag` is
+  # absent and has_a()/has_b() are Unknown, but `tail` keeps the size Known so
+  # 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.)
+  0 [+1]    UInt  outer
+  if outer == 1:
+    1 [+1]  UInt  tag
+
+  if tag == 0:
+    2 [+1]  UInt  a
+
+  if tag == 1:
+    2 [+1]  UInt  b
+
+  3 [+1]    UInt  tail
+
+
+struct DisjunctionConditionalDiscriminant:
+  # Disjunction arms (`tag == 0 || tag == 1`) on a conditional discriminant,
+  # each carrying a residual.  Exercises the disjunction matcher and case-label
+  # coalescing together with the residual-only guarded switch.
+  0 [+1]    UInt  outer
+  if outer == 1:
+    1 [+1]  UInt  tag
+
+  if (tag == 0 || tag == 1) && outer == 1:
+    2 [+1]  UInt  a
+
+  if (tag == 2 || tag == 3) && outer == 1:
+    2 [+1]  UInt  b
+
+
+struct SingleEntryConditionalDiscriminant:
+  # A single residual arm on a conditional discriminant.  The switch group has
+  # one entry, so it is demoted to a has_X() check instead of a switch; no
+  # discriminant guard is emitted, so the bug cannot arise here.
+  0 [+1]    UInt  outer
+  if outer == 1:
+    1 [+1]  UInt  tag
+
+  if tag == 0 && outer == 1:
+    2 [+1]  UInt  a
+
+
+struct EnumConditionalDiscriminant:
+  # Conditional discriminant of enum type, with residual arms.  Exercises the
+  # enum-typed case labels in the guarded switch.
+  0 [+1]    OnOff  outer
+  if outer == OnOff.ON:
+    1 [+1]  OnOff  tag
+
+  if tag == OnOff.OFF && outer == OnOff.ON:
+    2 [+1]  UInt   a
+
+  if tag == OnOff.ON && outer == OnOff.ON:
+    2 [+1]  UInt   b