Skip to content

Commit 66a2f8e

Browse files
committed
fix(codegen): fix Option match tag inversion in decision tree (C4)
Option match arms were swapped — `Some` mapped to tag 1 and `None` to tag 0, but construction uses `Some=0, None=1`. This caused silent miscompilation in both AOT and eval paths, producing wrong results for any `match` on `Option<T>`. Fixed in 3 locations: flatten.rs (decision tree compilation), emit.rs (variant field lookup), and the eval decision tree walker. Fixes 114 previously-failing spec tests.
1 parent a6f5532 commit 66a2f8e

6 files changed

Lines changed: 79 additions & 23 deletions

File tree

compiler/ori_arc/src/decision_tree/emit.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -297,8 +297,8 @@ fn lookup_variant_field_type(
297297
}
298298
}
299299
Tag::Option => {
300-
// Some (index 1) has one field (the inner type), None (index 0) has none.
301-
if variant_index == 1 && field_index == 0 {
300+
// Some (index 0) has one field (the inner type), None (index 1) has none.
301+
if variant_index == 0 && field_index == 0 {
302302
return pool.option_inner(resolved);
303303
}
304304
}

compiler/ori_arc/src/decision_tree/flatten.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ impl<'a> FlattenCtx<'a> {
231231
///
232232
/// Handles three cases:
233233
/// - `Tag::Enum`: looks up variant by name in the enum definition
234-
/// - `Tag::Option`: `None` = 0, `Some` = 1
234+
/// - `Tag::Option`: `Some` = 0, `None` = 1
235235
/// - `Tag::Result`: `Ok` = 0, `Err` = 1
236236
#[expect(
237237
clippy::cast_possible_truncation,
@@ -251,8 +251,8 @@ impl<'a> FlattenCtx<'a> {
251251
}
252252
}
253253
Tag::Option => {
254-
// Convention: None = 0, Some = 1 (matches evaluator's Value::Some/None)
255-
return u32::from(self.interner.lookup(variant_name) == "Some");
254+
// Convention: Some = 0, None = 1 (matches construction in lower_some/lower_none)
255+
return u32::from(self.interner.lookup(variant_name) == "None");
256256
}
257257
Tag::Result => {
258258
// Convention: Ok = 0, Err = 1 (matches evaluator's Value::Ok/Err)
@@ -273,7 +273,7 @@ impl<'a> FlattenCtx<'a> {
273273
/// Get the field types for a specific variant of an enum, Option, or Result.
274274
///
275275
/// - `Tag::Enum`: returns field types from the enum definition
276-
/// - `Tag::Option`: `Some` (index 1) has one field (the inner type), `None` (index 0) has none
276+
/// - `Tag::Option`: `Some` (index 0) has one field (the inner type), `None` (index 1) has none
277277
/// - `Tag::Result`: `Ok` (index 0) has one field (ok type), `Err` (index 1) has one field (err type)
278278
fn resolve_variant_field_types(
279279
&self,

compiler/ori_eval/src/exec/decision_tree/mod.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,12 @@ fn resolve_bindings(
254254
// Test matching
255255

256256
/// Check if a runtime value matches a decision tree edge's test value.
257+
#[expect(
258+
clippy::match_same_arms,
259+
reason = "Option and Result are independent type families with independently-defined \
260+
tag conventions. Merging Some|Ok and None|Err arms hides the per-family \
261+
semantics and caused the original C4 tag inversion bug."
262+
)]
257263
fn test_matches(
258264
value: &Value,
259265
_test_kind: TestKind,
@@ -272,8 +278,12 @@ fn test_matches(
272278
variant_name: vn, ..
273279
} => *vn == *variant_name,
274280
// Some/None/Ok/Err are represented as special Value variants.
275-
Value::Some(_) | Value::Err(_) => *variant_index == 1,
276-
Value::None | Value::Ok(_) => *variant_index == 0,
281+
// Convention: Some = 0, None = 1 (matches lower_some/lower_none)
282+
Value::Some(_) => *variant_index == 0,
283+
Value::None => *variant_index == 1,
284+
// Convention: Ok = 0, Err = 1 (matches lower_ok/lower_err)
285+
Value::Ok(_) => *variant_index == 0,
286+
Value::Err(_) => *variant_index == 1,
277287
_ => false,
278288
},
279289

compiler/ori_eval/src/exec/decision_tree/tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ fn switch_option_tag() {
189189
edges: vec![
190190
(
191191
TestValue::Tag {
192-
variant_index: 1,
192+
variant_index: 0,
193193
variant_name: interner.intern("Some"),
194194
},
195195
DecisionTree::Leaf {
@@ -199,7 +199,7 @@ fn switch_option_tag() {
199199
),
200200
(
201201
TestValue::Tag {
202-
variant_index: 0,
202+
variant_index: 1,
203203
variant_name: interner.intern("None"),
204204
},
205205
DecisionTree::Leaf {

compiler/ori_llvm/tests/aot/spec.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,52 @@ fn test_aot_result_err_check() {
765765
);
766766
}
767767

768+
/// C4 regression: Option match tag inversion — switch labels must match construction tags.
769+
/// Construction: Some=tag 0, None=tag 1. Match must use the same mapping.
770+
#[test]
771+
fn test_aot_option_match_tag_correctness() {
772+
assert_aot_success(
773+
r#"
774+
@unwrap_or (opt: Option<int>, default: int) -> int =
775+
match opt { Some(v) -> v, None -> default }
776+
777+
@main () -> int = {
778+
let some_val = unwrap_or(opt: Some(42), default: 0);
779+
let none_val = unwrap_or(opt: None, default: 99);
780+
// some_val should be 42 (not 0), none_val should be 99 (not garbage)
781+
if some_val == 42 then {
782+
if none_val == 99 then 0 else 1
783+
} else 1
784+
}
785+
"#,
786+
"option_match_tag_correctness",
787+
);
788+
}
789+
790+
/// C4 regression: match on Option inside if/else producing Option values.
791+
#[test]
792+
fn test_aot_option_match_with_construction() {
793+
assert_aot_success(
794+
r#"
795+
@safe_div (a: int, b: int) -> Option<int> =
796+
if b == 0 then None else Some(a / b);
797+
798+
@unwrap_or (opt: Option<int>, default: int) -> int =
799+
match opt { Some(v) -> v, None -> default }
800+
801+
@main () -> int = {
802+
let a = unwrap_or(opt: safe_div(a: 100, b: 5), default: 0);
803+
let b = unwrap_or(opt: safe_div(a: 100, b: 0), default: 5);
804+
// a should be 20, b should be 5
805+
if a == 20 then {
806+
if b == 5 then 0 else 1
807+
} else 1
808+
}
809+
"#,
810+
"option_match_with_construction",
811+
);
812+
}
813+
768814
#[test]
769815
fn test_aot_option_some_unwrap() {
770816
assert_aot_success(

plans/llvm-codegen-fixes/section-01-critical-correctness.md

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
22
section: "01"
33
title: "Critical Correctness"
4-
status: not-started
4+
status: in-progress
55
goal: "All 12 code journeys produce correct results in both eval and AOT paths"
66
inspired_by:
77
- "Rust rustc_codegen_llvm/mir/operand.rs — OperandValue variant handling"
@@ -10,7 +10,7 @@ depends_on: []
1010
sections:
1111
- id: "01.1"
1212
title: "Fix C4 — Option match tag inversion"
13-
status: not-started
13+
status: complete
1414
- id: "01.2"
1515
title: "Fix C3 — Payload sum type $eq codegen"
1616
status: not-started
@@ -62,14 +62,14 @@ switch i64 %proj.0, label %bb4 [
6262

6363
**Root cause hypothesis:** The match codegen for monomorphized built-in generic types uses a different variant ordering than construction. The `?` operator avoids this because it uses a direct `icmp eq` rather than a `switch`.
6464

65-
- [ ] Reproduce with Journey 12's code — AOT returns 144 instead of 33
66-
- [ ] Write targeted test: `Option<int>` construction + match, compare eval vs AOT
67-
- [ ] Write test: user-defined `type MyOpt<T> = MySome(v: T) | MyNone` — should pass (confirms isolation)
68-
- [ ] Trace the match codegen path for `Option<int>` — find where variant tag → switch label mapping diverges
69-
- [ ] Find the divergence between user-defined sum type match (correct) and built-in generic match (inverted)
70-
- [ ] Fix the root cause — ensure match switch labels match construction tags for all sum types
71-
- [ ] Verify `?` operator still works (it uses a different code path)
72-
- [ ] Journey 12 AOT returns 33
65+
- [x] Reproduce with Journey 12's code — AOT returns 144 instead of 33
66+
- [x] Write targeted test: `Option<int>` construction + match, compare eval vs AOT
67+
- [x] Write test: user-defined `type MyOpt<T> = MySome(v: T) | MyNone` — should pass (confirms isolation)
68+
- [x] Trace the match codegen path for `Option<int>` — find where variant tag → switch label mapping diverges
69+
- [x] Find the divergence between user-defined sum type match (correct) and built-in generic match (inverted)
70+
- [x] Fix the root cause — ensure match switch labels match construction tags for all sum types
71+
- [x] Verify `?` operator still works (it uses a different code path)
72+
- [x] Journey 12 AOT returns 33
7373

7474
---
7575

@@ -175,9 +175,9 @@ PANIC: ValueId 4294967295 out of bounds
175175
- [ ] Journey 5 (closures): AOT returns 27, matching eval
176176
- [ ] Journey 10: list indexing test case returns correct element value
177177
- [ ] Journey 11 (derived Eq): AOT returns 33, matching eval
178-
- [ ] Journey 12 (Option match): AOT returns 33, matching eval
178+
- [x] Journey 12 (Option match): AOT returns 33, matching eval
179179
- [ ] All 12 journeys: `./scripts/dual-exec-verify.sh` — 0 mismatches
180-
- [ ] No new regressions: `./test-all.sh` green
181-
- [ ] `./clippy-all.sh` green
180+
- [x] No new regressions: `./test-all.sh` green
181+
- [x] `./clippy-all.sh` green
182182

183183
**Exit Criteria:** All 4 critical bugs fixed. `./scripts/dual-exec-verify.sh` runs all 12 journey programs through both eval and AOT, producing 0 mismatches. No test regressions.

0 commit comments

Comments
 (0)