Handle double-negation preserving not-null knowledge

1 files changed, 187 insertions, 0 deletions

diff --git a/docs/chapter-backports.md b/docs/chapter-backports.md
index 190e5351..1828b16b 100644
--- a/docs/chapter-backports.md
+++ b/docs/chapter-backports.md
@@ -23,6 +23,7 @@ the correction into the chapter's representation, not the entire modern file.
 | B10 | Preserve widening state in integer nonzero refinement | **Audit** first chapter with both widening and `nonZero`; no later than 24 | Chapter 25 `TypeInteger.nonZero` preserves `_widen`. Check lattice laws and loop refinement. Exclude TypeScalar, storage-type, and serialization changes. |
 | B11 | Diagnose return types using the optimized return expression | 18; 19 already has the correction | `ReturnNode.err()` uses `expr()._type` instead of the parse-time `mt` aggregate. Chapter 18 rejects `struct S { u8 x; }; return new S; return 0;` with a mixed integer/reference error; Chapter 19 accepts it. Also test genuinely reachable incompatible returns. This is a candidate, not yet an applied or isolated-patch-verified fix. |
 | B12 | Encode the actual destination register for two-address immediate multiply | 21 | `MulIX86` inherits `ImmX86.encoding`, whose ModRM.reg is fixed to zero and whose REX.R is clear. With source/destination both `rcx`, multiplying by 11 emits `48 6b c1 0b` (destination `rax`) instead of `48 6b c9 0b`. With both `r9`, it emits `49 6b c1 0b` instead of `4d 6b c9 0b`. Give multiply its proper register fields while preserving Chapter 21's two-address allocation contract; do not change the opcode-extension fields used by other `ImmX86` subclasses. Later chapters use a separate multiply encoder. |
+| B14 | Check leaf type kinds before cyclic equality | 23-24; 25 already checks | `Type.cycle_eq` calls leaf `eq` without checking `_type`. Intern a struct with a `Type.BOTTOM` field, then same-named structs with float constants 1.0 and 2.0; meeting the latter throws ClassCastException in `TypeFloat.eq`. Reproduced independently in 23 and 24; the adapted probe passes in 25. Add a deterministic TypeTest before backporting the type-kind check. This is separate from B13; no type implementation changed. |
 
 After the first two reviews, batch only corrections with established independence
 and regressions. Keep one logical correction per commit across affected chapters.
@@ -76,6 +77,192 @@ starts, so run `make lib` separately before `make tests`.
 
 ## Validation record
 
+### Issue #246: Chapter 25 reproduction and local correction
+
+On 2026-09-19, rebuilt Chapter 25 with
+`make build/classes/main/.mtag build/classes/test/.ttag` and ran the following
+through `new CodeGen(src).driver(CodeGen.Phase.TypeCheck)` with Java assertions
+enabled. Before the correction it failed in `MemOpNode.err()` with
+`Might be null accessing 'x'`.
+The original issue's `new Point { x = 42; }` syntax is no longer accepted;
+only object construction was adapted to Chapter 25's constructor syntax.
+
+```text
+struct Point { int x; new Point = { int v -> x = v; }; };
+Point?[] !points = new Point?[2];
+points[arg] = new Point(42);
+Point? p = points[1];
+if (p != null)
+    return p.x;
+return -1;
+```
+
+That failure prevented evaluation of either expected runtime result.
+
+Debugging isolated the first bad phase to parsing. `Parser.parseEquality`
+builds `!(p == null)`, and `BoolNode.EQ.idealize` rewrites the inner comparison
+to `!p`. `ScopeNode._addGuards` records nonzero `!!p` and zero `!p` on the
+true arm, but does not descend again to record non-null `p`. Meanwhile,
+`CProjNode.idealize` strips the negations from the control test. The resulting
+graph therefore tests `p` directly without refining the pointer used by `p.x`.
+
+With default seed 126, the original's `LoadNode#150` uses nullable
+`ReadOnlyNode#126` directly after Parse, Iter, and Opto. Changing only the
+condition to `p` produces `GuardNode#132` on that same pointer, consumed by
+`LoadNode#135`, and returns `-1` and `42` as expected. Explicit `!!p` fails;
+`if (p == null) return -1; return p.x;` passes with both expected results.
+The original failure and successful `if (p)` control were also checked with
+seeds 1, 42, and 123, with assertions enabled.
+
+The local correction recurses into a Not operand when it is another Not or a
+short-circuit Phi, flipping the proven truth. Existing single-negation handling
+remains in place; its guards need not be duplicated. Each recursive call still
+checks dominance/schedulability, and the predicate is kept alive while recursive
+peepholes run. Boolean expression values retain their original meaning.
+
+`Chapter10Test.testNullGuards` covers both comparison orders,
+nested negations, early returns, a reused Boolean value, and negated
+short-circuit logic. Each case checks both runtime outcomes and scheduling with
+seeds 1, 42, and 126. `testNullGuardErrors` verifies rejection of
+unguarded access, a check on a different pointer, and a guard used beyond its
+branch. The positive regression failed before the compiler change.
+
+Final `make tests` passed all 427 tests with assertions enabled: 362 raw0,
+32 raw1, 7 standalone, 18 system, 1 fixed-seed fuzzer wrapper, and 7 remaining
+tests. The build regenerated `sys.o`. Log: `chapter25/build/issue246-tests.log`.
+A broader intermediate version that duplicated single-negation handling exposed
+a Dijkstra SCCP assertion; it was not retained. The original compiler passed
+Dijkstra in a comparison run with freshly rebuilt `sys.o`, and the final
+correction passes it too. No assertions or existing test expectations changed.
+
+Separate pre-existing diagnostic concern: using the same array setup with
+`if (p == null) return p.x; return -1;` was accepted before the correction,
+despite dereferencing null on the taken arm. This remains outside B13's fix.
+
+### B13: earliest chapter and regression placement
+
+Null-check refinement begins in Chapter 10, alongside nullable struct pointers;
+it is documented in `chapter10/README.md` and implemented by
+`ScopeNode.upcast`. Chapter 9 has no corresponding pointer guards. The reduced
+failure already reproduces in Chapter 10:
+
+```text
+struct Point { int x; };
+Point point = new Point;
+point.x = 42;
+Point? p = null;
+if (arg) p = point;
+if (p != null) return p.x;
+return -1;
+```
+
+The same program with `if (p)` passes. The array version reproduces in Chapter
+15, where arrays first appear; its `if (p)` control also passes.
+
+Substantial guard implementation differences before the backport:
+
+| Chapters | Representation and behavior |
+|---|---|
+| 10-16 | `ScopeNode.upcast` refines only values present directly in local scope inputs. A non-null pointer uses `CastNode(TypeMemPtr.VOIDPTR, ...)`; a zero/null fact replaces a local with a constant. There is no expression-guard table and no general nonzero integer refinement. |
+| 13 onward | The parser also refines the false arm without an explicit `else`, allowing a fact to survive an early return. Chapters 10-12 require an explicit `else` for that test. |
+| 17 | `addGuards`/`removeGuards` introduce a scoped predicate/cast table and `upcastGuard` applies facts to later matching expressions, including loads. Refinements use `nonZero`/`makeZero` and type-bearing CastNodes. |
+| 18-24 | The same scoped Cast architecture; `_addGuard` factors the two cases and rejects high joins. Chapter 23 adds short-circuit syntax, but these scopes do not recursively decompose its Phis as Chapter 25 does. |
+| 25 | GuardNode stores zero/nonzero intent and derives the value family from its input, supporting incomplete types. Scope guard discovery checks dominance, decomposes short-circuit Phis, and now follows nested negations. |
+
+The tests have been moved from Chapter 25's `Chapter25Test.java` to
+`Chapter10Test.java` and forward-ported into that same test class in every
+snapshot from 10 through 25. Chapters 10-14 use the reduced pointer setup;
+15-24 use arrays with the allocation syntax available there; 25 retains the
+constructor-based array setup, seed rotation, and scheduling checks. Boolean
+temporaries use `int` in earlier snapshots, early-return coverage uses an
+explicit `else` before Chapter 13, and short-circuit coverage starts at 23.
+The tests are active and assert successful guarded access, not the old error.
+
+The compiler backport now retains each chapter's existing guard representation
+and adds negation traversal there, as detailed below. Importing Chapter 25's
+GuardNode/incomplete-type architecture remains a separate design change.
+
+Before adding the tests, `make -k tests` passed in all snapshots 10-24.
+Log: `chapter25/build/issue246-review/baseline.log`.
+
+After moving/porting the tests, ran `make -k tests` across 10-25:
+
+- Chapters 10-23 each fail only `Chapter10Test.testNullGuards`, with
+  `Might be null accessing 'x'`. The new rejection test and all existing tests
+  in those suites pass. These were the exposed regressions before the compiler
+  backport recorded below.
+- Chapter 24's isolated `Chapter10Test` runs 26 tests with only the same B13
+  failure. Its full suite also exposes a type-equality failure, starting at
+  `Chapter16Test.testSquare`: `TypeFloat.eq` receives a plain `Type` through
+  `Type.cycle_eq`, causing a ClassCastException and later cascading failures
+  (197 failures total, including B13). A forced `make -B tests CHAPTERS=chapter24`
+  reproduces this, so a rebuild does not resolve it. No type implementation
+  was changed. This additional failure must be reviewed separately.
+- Chapter 25 passes all 427 tests after relocation: 364 raw0, 32 raw1,
+  7 standalone, 16 system, 1 fuzzer wrapper, and 7 remaining tests.
+
+Logs: `chapter25/build/issue246-review/ported-tests.log`,
+`chapter25/build/issue246-review/chapter24-focused.log`, and
+`chapter25/build/issue246-review/chapter24-rebuilt.log`.
+
+### B13: completed compiler backport
+
+Applied locally on 2026-09-19 to every snapshot from Chapter 10 through 25;
+nothing pushed. The same paired-negation traversal starts in Chapter 10 and
+continues forward, with representation-specific adaptations:
+
+- 10-16 recurse within `upcast`, retaining local-binding casts and constants.
+  No dominance machinery, expression-guard table, or modern type architecture
+  is introduced.
+- 17-22 separate the public guard-set marker from recursive `_addGuards`.
+  Nested negations add facts within the existing scoped CastNode table;
+  recursive calls do not add extra scope markers.
+- 23-24 additionally use Chapter 25's short-circuit Phi decomposition, since
+  short-circuit syntax starts in 23. A small `availableAt` helper walks data
+  inputs and the existing CFG immediate-dominator chain to reject RHS-only
+  values unavailable at the proposed guard. Phi availability is checked at
+  its Region. This avoids importing 25's folding-aware `earlyCFG` machinery.
+- 25 retains its existing GuardNode, dominance, and short-circuit mechanisms;
+  only the nested-negation discovery is added.
+
+All versions retain the original single-negation handling and keep the
+predicate alive during recursive peepholes. The added guards do not change
+the Boolean expression's value.
+
+The reused-Boolean regression also exposed `Eval2` casting a pointer to Long
+when evaluating Not in 18-24. Those test evaluators now share Chapter 25's
+null/integer/float-aware Not helper. Compiler type implementations are unchanged.
+
+`Chapter10Test` remains the home of the forward-ported regression in every
+snapshot 10-25. In 23-25, `testShortCircuitGuardScheduling` additionally checks
+an RHS-only call result through local scheduling and evaluates both branch
+outcomes and a zero result. Chapter 25's pointer cases retain seed rotation.
+
+Each full `make tests` target passes after the correction:
+
+| Chapters | Ordinary tests per chapter | Additional suites |
+|---|---|---|
+| 10-17 | 149, 164, 166, 181, 201, 212, 230, 282 | Each complete chapter target passed. |
+| 18-22 | 309, 350, 359, 374, 386 | Each also passed its fixed-seed fuzzer wrapper. |
+| 23-24 | 403, 425 | Each also passed its fixed-seed fuzzer wrapper. |
+| 25 | 428 total | 365 raw0, 32 raw1, 7 standalone, 16 system, 1 fuzzer wrapper, 7 remaining. |
+
+Commands were `make tests CHAPTERS=chapter10`, then `make -k tests` with
+`CHAPTERS='chapter11 ... chapter22'`, `CHAPTERS='chapter18 ... chapter24'`,
+and finally `CHAPTERS='chapter23 chapter24 chapter25'` (the ellipses here
+abbreviate explicitly enumerated chapter names). Logs under
+`chapter25/build/issue246-review/`: `chapter10-fixed.log`,
+`chapters11-22-fixed.log` (11-17 passed; 18-22 exposed the evaluator issue),
+`chapters18-24-fixed.log`, and `chapters23-25-final.log`.
+
+The separate cyclic-equality failure is not fixed by B13's green suites.
+It has a deterministic standalone reproducer in 23 and 24 and is queued as
+B14. The probe interns `Probe { x: BOTTOM }`, `Probe { x: 1.0 }`, and
+`Probe { x: 2.0 }`, then meets the latter two. It throws in `TypeFloat.eq`
+through `Type.cycle_eq`; Chapter 25's equivalent probe succeeds. This records
+the previously order-sensitive full-suite failure without expanding the guard
+fix into a lattice change.
+
 ### Initial setup baseline
 
 On 2026-09-17, using Windows/Cygwin and OpenJDK 22.0.2: