Skip to content

Commit dfdcf6f

Browse files
Fix 2 more crashes: typeof-dispatch cast helpers in MLIRGenCast.cpp (#299)
castPrimitiveTypeFromAny (the __unbox<T> helper, generic type-param unboxing from any) was dead code - its one call site already only passes types the TypeSwitch handles - but fixed anyway (crash -> emitError) for consistency, since location was already in scope. castFromUnion is a real, easily-reachable crash: any union type with a tuple/object-literal-shaped member (e.g. `number | {a: number}`) crashes the moment it needs a runtime cast, since the typeof-dispatch TypeSwitch never got a TupleType/ConstTupleType case. The function's own forward-decl already carries a TODO acknowledging typeof-based dispatch can't handle this properly (can't even distinguish two different tuple shapes from each other) - a real redesign, out of scope here. Converted the crash to a clean error instead. Closes out MLIRGenCast.cpp's two TypeOf sites, the last item from the original not-implemented-audit's §5.1 named/specific list. 829/829 ctest, no regressions.
1 parent 47c6d8b commit dfdcf6f

2 files changed

Lines changed: 106 additions & 18 deletions

File tree

tslang/docs/not-implemented-audit.md

Lines changed: 82 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
# `llvm_unreachable("not implemented")` audit
22

3-
Status: **8 confirmed crashes fixed across three passes, ~112 markers still
3+
Status: **9 confirmed crashes fixed across four passes, ~110 markers still
44
uninvestigated** — written as a roadmap for continuing this audit, not a
55
claim that the sweep is complete. Triggered by a user request to review
66
every "not implemented" marker in the codebase and see which ones can be
77
implemented. Second pass (§4.3-4.5) worked through this document's own §6
88
priority list while waiting on the first pass's PR to merge. Third pass
99
(§4.6-4.8) closed out §5.4's `MLIRTypeHelper.h` `funcRef` family after that
10-
PR merged.
10+
PR merged. Fourth pass (§4.9) closed out `MLIRGenCast.cpp`'s two `TypeOf`
11+
sites, the last item left from the original §5.1 named/specific list.
1112

1213
## 1. Scope and method
1314

@@ -335,6 +336,69 @@ can't be found" instead of crashing) and via the full suite (`ctest -C Debug
335336
existing tests exercising these utility types with a real function argument
336337
(`test/tester/tests/00types_utility.ts`, `01types_utility.ts`) still pass.
337338

339+
### 4.9 `MLIRGenCast.cpp`'s two `TypeOf` sites — one dead, one a real, easily-reachable crash
340+
341+
Both are `.Default` branches of a `TypeSwitch` that builds up a synthetic
342+
`typeof t == '...'` dispatch function as TS source text, then parses and
343+
calls it - the compiler's mechanism for runtime type discrimination when
344+
casting away from a type whose concrete shape isn't known until runtime
345+
(`any`, or a union that can't be merged into one storage type).
346+
347+
**`castPrimitiveTypeFromAny` (was :1320-1322, the `__unbox<T>` helper for
348+
generic type-parameter unboxing from `any`, as guessed in the original
349+
§5.1 entry): dead code.** Its one call site (`MLIRGenCast.cpp:1140`, inside
350+
`castFromSourceSpecialCases`-family cast dispatch) only invokes it when
351+
`type` (the cast destination) is one of `{NumberType, BooleanType,
352+
StringType, BigIntType, IntegerType, FloatType, ClassType}` - a strict
353+
subset of what the `TypeSwitch` inside already handles (`{Boolean,
354+
TypePredicate, Number, String, Char, Integer, Float, Index, BigInt,
355+
Function×4, Class, Interface, Null, Undefined}`). Same "guarded, therefore
356+
dead" shape as §3's `IntersectionType` and §4.6-4.8's `funcRef` family.
357+
Fixed anyway (crash → set a flag, `emitError` + `return failure()` after
358+
the switch) since `location` was already in scope here and leaving a live
359+
`llvm_unreachable` behind is a landmine for the next caller.
360+
361+
**`castFromUnion` (was :1498-1499): a real, easily-reachable crash.** Called
362+
from `castFromSourceSpecialCases` whenever casting *from* a union-typed
363+
value whose members can't be merged into one storage representation
364+
(`mth.isUnionTypeNeedsTag`) to anything other than `any`. It loops over
365+
each union member type building the same kind of `typeof`-dispatch
366+
function, and the `TypeSwitch` per member is missing `TupleType`/
367+
`ConstTupleType` entirely - i.e. **any union with an object-literal-shaped
368+
member hits this the moment it needs a runtime cast**, which is a very
369+
ordinary shape (not an obscure corner case like §4.4's enum reverse-mapping
370+
or §4.5's `super()` edge case):
371+
372+
```ts
373+
function main() {
374+
let x: number | { a: number };
375+
x = 5;
376+
let y = <number>x; // crash: UNREACHABLE at MLIRGenCast.cpp:1499
377+
}
378+
```
379+
380+
The function's own forward declaration in `MLIRGenImpl.h` already carries a
381+
`// TODO: remove using typeof for Union types as it can't handle types such
382+
as 2 tuples in union etc` - confirming this is a known, **genuinely missing
383+
feature** (like §4.2), not just an unreached architectural corner: even two
384+
*different* tuple-shaped union members couldn't be told apart by `typeof`
385+
alone (both report `"object"`), so a real fix needs a structural redesign
386+
(a runtime shape tag, not `typeof` string dispatch), out of scope here. A
387+
partial start at this is visible in the code - a `tupleTypes`
388+
`SmallVector` and `TYPE_TUPLE_ALIAS` templating exist and are wired up at
389+
the end of the function, but nothing ever pushes into `tupleTypes` because
390+
no `.Case<mlir_ts::TupleType>` was ever added to populate it; that
391+
half-finished thread was left as-is rather than completed, since finishing
392+
it properly means solving the "2 tuples in union" ambiguity the TODO
393+
already flags, not just adding one more `.Case`. Converted the crash to
394+
`emitError(location) << "Cast from " << to_print(value.getType()) << " to "
395+
<< to_print(type) << " is not supported"; return mlir::failure();` after the
396+
member loop, gated by the same kind of flag used for `castPrimitiveTypeFromAny`
397+
(a per-subtype lambda can't `return` the enclosing function directly).
398+
399+
Verified individually (clean diagnostic, no crash) and via the full suite
400+
(`ctest -C Debug -j8`: 829/829, no regressions).
401+
338402
## 5. Inventory of remaining markers (untested this pass)
339403

340404
Grouped by file. "Shape" is a guess from reading the surrounding code, not a
@@ -343,12 +407,12 @@ verified verdict — see §2 for how to actually check one.
343407
### 5.1 Named/specific (cheapest to investigate next — read the message + local branch, write a 5-line repro)
344408

345409
**Fixed this pass**: `MLIRGenAccessCall.cpp`'s three sites (was lines
346-
1159/1219/1535) — see §4.3-4.5.
410+
1159/1219/1535) — see §4.3-4.5. **Fixed a previous pass** (§4.9):
411+
`MLIRGenCast.cpp`'s two `TypeOf` sites (was lines 1321-1322/1498-1499) — one
412+
dead (guarded), one a real crash (union with a tuple-shaped member).
347413

348414
| Site | Message | Shape (unverified guess) |
349415
| --- | --- | --- |
350-
| `MLIRGenCast.cpp:1321-1322` | TypeOf NOT IMPLEMENTED for Type | inside a generated `__unbox<T>` helper (generic type-parameter unboxing from `any`); `.Default` for a type kind not in its explicit list (Tuple/Array/Enum/Union/Optional are plausible candidates) |
351-
| `MLIRGenCast.cpp:1498-1499` | TypeOf NOT IMPLEMENTED for Type | second, near-identical site — check if it's reachable via a different call path than 1321 |
352416
| `MLIRGenImpl.h:5330` | not implemented | unread |
353417
| `MLIRGenImpl.h:6732` | not implemented | unread |
354418
| `MLIRGenImpl.h:7314` | not implemented | unread |
@@ -424,17 +488,23 @@ bug (the built-in utility types); tracing real callers is what worked.
424488
(faster than the originally-planned unit-test approach), found and fixed
425489
3 more live crashes (§4.6-4.8); the other 3 functions in the family were
426490
fixed too even though proven dead, for consistency within the family.
427-
4. §5.2 (generic fallbacks) — triage a handful against existing passing
491+
4. ~~`MLIRGenCast.cpp`'s two `TypeOf` sites~~ — done this pass (§4.9): one
492+
dead (guarded), one a real crash (union with a tuple-shaped member,
493+
`<number>x` where `x: number | {a: number}`) — fixed. That was the last
494+
item from the original §5.1 named/specific list; only the large
495+
`MLIRGenImpl.h`/`MLIRGenInterfaces.cpp`/`MLIRGenTypes.cpp` cluster remains
496+
from §5.1, plus the stray
497+
`MLIRTypeHelper.h:410/420/2108/2256-2257/2290/2307/2685/2709` sites
498+
(confirmed *not* part of the `funcRef` family, see §5.4).
499+
5. The `MLIRGenImpl.h`/`MLIRGenInterfaces.cpp`/`MLIRGenTypes.cpp` cluster
500+
(§5.1's last remaining block) — read-and-repro each per §2's recipe, same
501+
as every other §5.1 item so far.
502+
6. §5.2 (generic fallbacks) — triage a handful against existing passing
428503
tests using §3's method before assuming any individual one is live.
429-
5. §5.3 (RTTI) — lowest priority from this (Windows) machine; the Linux
504+
7. §5.3 (RTTI) — lowest priority from this (Windows) machine; the Linux
430505
variants need a WSL/Linux build to exercise at all, and even the Windows
431506
ones are deep in a code path (RTTI/exception typeinfo generation) that's
432507
hard to reach without a specific class-hierarchy-plus-exception scenario.
433-
6. `MLIRGenCast.cpp`'s two `TypeOf` sites, the large `MLIRGenImpl.h`/
434-
`MLIRGenInterfaces.cpp`/`MLIRGenTypes.cpp` cluster, and the stray
435-
`MLIRTypeHelper.h:410/420/2108/2256-2257/2290/2307/2685/2709` sites
436-
(confirmed *not* part of the `funcRef` family, see §5.4) all remain
437-
unread from the original §5.1 list.
438508

439509
## 7. Non-goals / out of scope
440510

tslang/lib/TypeScript/MLIRGenCast.cpp

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,6 +1293,7 @@ namespace mlirgen
12931293
SmallVector<mlir::Type> classInstances;
12941294
ss << S("function __unbox<T>(a: any) : T {\n");
12951295
auto subType = type;
1296+
auto hasUnsupportedType = false;
12961297
mlir::TypeSwitch<mlir::Type>(subType)
12971298
.Case<mlir_ts::BooleanType>([&](auto _) { typeOfs["boolean"] = true; })
12981299
.Case<mlir_ts::TypePredicateType>([&](auto _) { typeOfs["boolean"] = true; })
@@ -1317,10 +1318,16 @@ namespace mlirgen
13171318
// review code to use null in "TypeGuard"
13181319
.Case<mlir_ts::NullType>([&](auto _) { /* TODO: uncomment when finish with TypeGuard and null */ /*typeOfs["null"] = true;*/ })
13191320
.Case<mlir_ts::UndefinedType>([&](auto _) { /* TODO: I don't think we need any code here */ /*typeOfs["undefined"] = true;*/ })
1320-
.Default([&](auto type) {
1321+
.Default([&](auto type) {
13211322
LLVM_DEBUG(llvm::dbgs() << "\n\t TypeOf NOT IMPLEMENTED for Type: " << type << "\n";);
1322-
llvm_unreachable("not implemented yet");
1323-
});
1323+
hasUnsupportedType = true;
1324+
});
1325+
1326+
if (hasUnsupportedType)
1327+
{
1328+
emitError(location) << "Cast from 'any' to " << to_print(type) << " is not supported";
1329+
return mlir::failure();
1330+
}
13241331

13251332
auto next = false;
13261333
for (auto& pair : typeOfs)
@@ -1457,6 +1464,7 @@ namespace mlirgen
14571464
StringMap<boolean> typeOfs;
14581465
SmallVector<mlir::Type> classInstances;
14591466
SmallVector<mlir::Type> tupleTypes;
1467+
auto hasUnsupportedType = false;
14601468
ss << S("function __cast<T, U>(t: T) : U {\n");
14611469
for (auto subType : normalizedUnion.getTypes())
14621470
{
@@ -1494,10 +1502,20 @@ namespace mlirgen
14941502
.Case<mlir_ts::ObjectType>([&](auto _) { typeOfs["object"] = true; })
14951503
.Case<mlir_ts::NullType>([&](auto _) { typeOfs["null"] = true; })
14961504
.Case<mlir_ts::UndefinedType>([&](auto _) { typeOfs["undefined"] = false; })
1497-
.Default([&](auto type) {
1505+
.Default([&](auto type) {
14981506
LLVM_DEBUG(llvm::dbgs() << "\n\t TypeOf NOT IMPLEMENTED for Type: " << type << "\n";);
1499-
llvm_unreachable("not implemented yet");
1500-
});
1507+
hasUnsupportedType = true;
1508+
});
1509+
}
1510+
1511+
if (hasUnsupportedType)
1512+
{
1513+
// e.g. a tuple/object-literal-shaped member of the union - see the
1514+
// "must be improved"/"can't handle types such as 2 tuples in union"
1515+
// TODO on castFromUnion's declaration; typeof-based dispatch can't
1516+
// distinguish these today, that's a separate, larger redesign.
1517+
emitError(location) << "Cast from " << to_print(value.getType()) << " to " << to_print(type) << " is not supported";
1518+
return mlir::failure();
15011519
}
15021520

15031521
if (isNullDest)

0 commit comments

Comments
 (0)