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 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). (`ResidualConditionalDiscriminant` in
+    #     testdata/condition.emb exercises this shape.)
     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 (_, 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,218 @@ TEST(Conditional, StructWithNestedConditionIsOkWhenOuterConditionExists) {
   EXPECT_EQ(2U, writer.SizeInBytes());
 }
 
+// Regression tests for Ok() 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 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 (`tag == 0 || tag == 1`) on a conditional discriminant, all
+// carrying a residual: the message must be Ok() when the discriminant is
+// absent, and each disjunct must select the field when it is present.
+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());
+}
+
+// A single residual arm on a conditional discriminant: the message must be
+// Ok() when the discriminant is absent.
+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: the message must be
+// Ok() when the discriminant is absent.
+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());

doc/cpp-guide.md

@@ -145,6 +145,13 @@ class GenericMyStructView final {
   // MyStruct, at least 12 bytes), and all fields are Ok().  For this struct,
   // the Int and UInt fields are always Ok(), and the Bcd field is Ok() if none
   // of its nibbles has a value greater than 9.
+  //
+  // For structs with conditional (`if`) fields: a conditional field that is
+  // simply absent -- its condition is known to be false -- does not make Ok()
+  // false.  However, if a field's existence cannot be determined -- for
+  // example, its `if` condition reads another field that is itself absent or
+  // outside the bounds of the Storage -- then Ok() returns false, even when
+  // IsComplete() returns true.
   bool Ok() const;
 
   // IsComplete() returns true if the Storage is big enough for the struct.

testdata/condition.emb

@@ -229,3 +229,95 @@ struct ConditionalOnFlag:
     0 [+1]  Flag  enabled
   if enabled:
     1 [+1]  UInt  value
+
+
+struct ResidualConditionalDiscriminant:
+  # `tag` is itself conditional, and is the 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.  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 `ResidualConditionalDiscriminant`, but `a`/`b` reference the
+  # conditional discriminant `tag` without re-guarding `outer == 1`.  Here an
+  # out-of-bounds `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:
+  # Like `BareConditionalDiscriminant`, 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.
+  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.  Like `ResidualConditionalDiscriminant`, a
+  # message with `outer != 1` is valid even though `tag` is absent.
+  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.  When `outer != 1`,
+  # `tag` is absent and the message must still be Ok().
+  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.  When
+  # `outer != OnOff.ON`, `tag` is absent and the message must still be Ok().
+  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