Skip to content

Narrowing in an else branch ends with the branch - #377

Merged
ASDAlexander77 merged 1 commit into
mainfrom
fix-else-narrowing-scope
Sep 27, 2026
Merged

ASDAlexander77 merged 1 commit into
mainfrom
fix-else-narrowing-scope

Conversation

@ASDAlexander77

Copy link
Copy Markdown
Owner

Narrowing a field in an else branch was not limited to that branch. Code after the if, and the next method compiled, then read a value from a region that no longer existed. Found in the BrowserLib sources attached to #231, where 7 of 14 files crashed. Event.ts alone reproduces it.

class N { v: number = 1; }
class L {
    head: N | null = null;
    a(): void { if (this.head === null) { print("x"); } else { print("y"); } }
    b(): boolean { return this.head === null; }   // crash, or "saving to constant object" at 1:1
}

Cause

Narrowing a property access registers {object type, "this.head"} → narrowed value in safeTypesMap. The true side of an if, a ?: and an && opens a SafeTypesMapScopeT, so its entry is removed when that side ends. The false side (the else narrowing from ElseSafeCase) opened only a SymbolTableScopeT. Its entry stayed until the module-level scope in MLIRGenModule.cpp, which caused two problems:

  • The rest of the function still saw this.head as the else value, even though that value is defined inside the else region.
  • Each method is generated twice, once for discovery and once for real. The second pass looked this.head up again and got a value from the discarded discovery body. What happened next depended on how the freed memory was reused: a crash, a bogus "saving to constant object", or nothing visible.

Fix

The false side of if (MLIRGenStatements.cpp), of ?: (MLIRGenExpressions.cpp) and of || (MLIRGenImpl.h) now opens a SafeTypesMapScopeT, the same as the true side.

|| does not narrow its right side yet, so that change only keeps the three places consistent.

Tests

  • New 00safe_cast_else_scope.ts (compile, jit, and the rc/none corpus). It covers narrowing in an if/else, in a ?:, a read after an if/else in the same method, and a later method. Each is checked with head null and not null. Before the fix it fails with 0xC0000005.
  • Full release suite: 2968/2968 passed.

The other #231 crash, a field of a union compared with null, is fixed in #376.

🤖 Generated with Claude Code

The then-branch of an if, a conditional and a logical operator opens a
SafeTypesMapScopeT, but the false side did not. A field narrowed there
(`this.head` after `if (this.head === null) ... else`) stayed in safeTypesMap
up to the module-level scope. Later reads of the field got a value from the
else region: in the rest of the function, and in the next method compiled,
whose discovery pass had already been discarded. The result was a crash or a
bogus "saving to constant object". The false side now opens its own scope.

Found in the BrowserLib sources attached to #231.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ASDAlexander77
ASDAlexander77 force-pushed the fix-else-narrowing-scope branch from e845917 to 2cdf9ff Compare September 27, 2026 21:51
@ASDAlexander77
ASDAlexander77 merged commit 4d8ed73 into main Sep 27, 2026
2 checks passed
@ASDAlexander77
ASDAlexander77 deleted the fix-else-narrowing-scope branch September 27, 2026 22:08
ASDAlexander77 added a commit that referenced this pull request Sep 28, 2026
#383)

`if (x === null) return;` now narrows x for the rest of the block, as an else
branch does: an if without else whose body always returns, throws, breaks or
continues applies its else narrowing after the if. Each block opens its own
narrowing scope, so a narrowing made in it ends with it.

A narrowed field (`this.head` after `this.head !== null`) was mapped to the
value the narrowing cast, and every later read reused that value. An
assignment to the field then failed ("saving to constant object"), a read
after it returned the old value, and a value used outside its region was the
use-after-free #377 fixed for else. safeTypesMap now keeps only the narrowed
type: a read loads the field again and casts it the way addSafeCastStatement
does (castToNarrowedType), and an assignment writes the field itself.

Found in the BrowserLib sources attached to #231 (Event.ts `remove`).

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant