From 68498768a8fd099756c70dbb9c803b30f093e531 Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Thu, 23 Jul 2026 22:42:02 +0100 Subject: [PATCH] Improve error handling for non-constant index access and unsupported spread in object literals --- tslang/docs/not-implemented-audit.md | 259 +++++++++++++++++++++++++++ tslang/lib/TypeScript/MLIRGenImpl.h | 20 ++- 2 files changed, 274 insertions(+), 5 deletions(-) create mode 100644 tslang/docs/not-implemented-audit.md diff --git a/tslang/docs/not-implemented-audit.md b/tslang/docs/not-implemented-audit.md new file mode 100644 index 000000000..1f5a9a4e1 --- /dev/null +++ b/tslang/docs/not-implemented-audit.md @@ -0,0 +1,259 @@ +# `llvm_unreachable("not implemented")` audit + +Status: **2 confirmed crashes fixed (this pass), ~120 markers still uninvestigated** — +written as a roadmap for continuing this audit, not a claim that the sweep is +complete. Triggered by a user request to review every "not implemented" marker +in the codebase and see which ones can be implemented. + +## 1. Scope and method + +A repo-wide search for `not implemented`/`llvm_unreachable` across +`lib/TypeScript/*.cpp` and `include/TypeScript/{MLIRLogic,LowerToLLVM}/*.h` +turns up **~130 raw matches** (some are two-line sites: an `LLVM_DEBUG` print +immediately followed by the `llvm_unreachable`, counted here as one site). +Grep command used: + +``` +grep -rn "not implemented\|Not implemented\|NOT IMPLEMENTED\|not yet implemented\|NotImplemented" \ + --include=*.cpp --include=*.h lib include tslang +``` + +These markers span three very different situations that look identical in a +grep, and need to be told apart before deciding what "implement" even means +for each: + +1. **Legitimate diagnostics.** A handful are `emitError(...)` calls for + genuinely invalid source (e.g. `MLIRGenClasses.cpp:1919`, "Abstract method + 'X' is not implemented in 'Y'" — a normal missing-override error; + `MLIRGenVariables.cpp:285/324`, array binding pattern spread/type + mismatches). These already produce clean compiler errors. Not gaps. +2. **Generic exhaustiveness fallbacks.** The majority (~90+ of the raw + matches) are `.Default([&](auto type) { llvm_unreachable("not + implemented"); })` at the bottom of an MLIR `TypeSwitch` chain, mostly in + low-level LLVM-lowering, RTTI, and cast-helper code + (`LowerToLLVM.cpp`, `CastLogicHelper.h`, `LLVMRTTIHelperVC*.h`, + `MLIRRTTIHelperVC*.h`, `MLIRTypeHelper.h`'s `funcRef` family, etc.). These + fire only if a `mlir::Type` value reaches that specific conversion/lowering + stage with a kind the author never handled there. Some are real gaps; many + are defensive guards for states the type system already rules out earlier + in the pipeline (see §3 for a proven example) — **reachability is unknown + without testing each one**, and that is genuinely the expensive part. +3. **Named, specific gaps.** ~40 markers carry a message naming the exact + scenario ("SpreadAssignment not implemented for type: X", "TypeOf NOT + IMPLEMENTED for Type: X", "not implemented (index)", "not implemented + (ElementAccessExpression)", …). These are much cheaper to turn into a + reproduction: the message plus its surrounding `if`/`TypeSwitch` branches + usually tells you exactly what source-level construct is missing. + +Of these, only **6 sites were actually tested** this pass (2 named markers +confirmed reachable and fixed, 1 generic fallback confirmed dead, 3 more +named markers read but not yet reproduced). Everything else in §5 is an +inventory, not a verdict. + +## 2. Reproduction recipe used + +For each candidate: read the surrounding code to infer what TS source +pattern would make execution reach that branch, write a minimal `.ts` file +exercising it, and run it through the actual compiler: + +``` +test-runner.exe +``` + +(no `-jit`/`-shared` needed for a single-file reachability check). A crash +looks like: + +``` +not implemented +UNREACHABLE executed at I:\...\MLIRGenImpl.h:5797! +``` + +If it crashes, the marker is real and reachable. If it compiles/runs, either +the type is resolved away earlier in the pipeline (like §3), or the guess +about the trigger was wrong and needs another attempt. + +## 3. Confirmed dead code (proof-of-concept for triaging fallbacks) + +`LowerToLLVM.cpp:6267`: + +```cpp +converter.addConversion([&](mlir_ts::IntersectionType type) { + llvm_unreachable("type usage (IntersectionType) is not implemented"); + return mlir::Type(); +}); +``` + +Intersection types (`A & B`) are exercised by two existing, currently-passing +tests (`00intersection_type.ts`, `00intersection_type_generic.ts`, +`test-compile-00-intersection-type[-generic]` / `test-jit-...` in +`test/tester/CMakeLists.txt`), so an `IntersectionType` MLIR value clearly +*can* exist. It just never survives to LLVM type conversion — it must be +resolved into its concrete merged/flattened type earlier in MLIRGen. This +one marker is confirmed unreachable for any currently-expressible source +program. **This is the template for triaging the remaining ~90 generic +fallbacks**: find an existing test that plausibly produces the type in +question, confirm it passes, and if so the fallback is very likely dead for +today's feature set (not proof for all future features, but proof for now). + +## 4. Fixed this pass + +Both fixes are in `lib/TypeScript/MLIRGenImpl.h`, both converted a hard +`llvm_unreachable` crash into an `emitError` + graceful failure, matching the +pattern already established at `MLIRGenVariables.cpp:285` (Array Binding +Pattern spread) — nothing new was invented, this is the existing +"fail loud with a message, don't crash" convention applied to two spots that +hadn't gotten it yet. + +### 4.1 `obj[dynamicKey]` — non-constant index on a tuple/object-literal value + +`mlirGenElementAccessTuple` (`MLIRGenImpl.h`, was line 5797): + +```ts +function main() { + const obj = { a: 1, b: 2 }; + let key = "a"; + print(obj[key]); // key is a runtime variable, not a literal +} +``` + +crashed with `UNREACHABLE executed at MLIRGenImpl.h:5797`. Root cause: +tuples/object-literals in this compiler lower to a **fixed-layout struct** +(each field resolved to a specific byte offset at compile time), not a +dynamic hash map. When the index expression is a compile-time constant, the +existing code resolves it to a field the normal way; the crash was the `else` +branch, hit whenever the index is a genuine runtime value. This is not +missing code so much as **a real limitation of the current object +representation** — properly "implementing" `obj[runtimeKey]` in general would +need a dynamic property-bag runtime representation (a different data +structure entirely, not a small patch). Converted the crash to: + +```cpp +emitError(location) << "Element access with a non-constant index is not supported on this type; " + "only array types and constant keys (obj[\"literal\"]) can be indexed"; +return ValueOrLogicalResult(mlir::failure()); +``` + +### 4.2 Spreading a non-struct-like value into an object literal + +The `SpreadAssignment` `TypeSwitch` inside object-literal codegen +(`MLIRGenImpl.h`, was line 7863) only handles spreading a +`TupleType`/`ConstTupleType`/`InterfaceType`/`ClassType`/`ObjectType` into +`{...expr}`. Anything else hit the `Default` branch: + +```ts +function main() { + const arr = [1, 2, 3]; + const obj = { ...arr }; // crash +} +function main2() { + let x: {a: number} | number[] = { a: 1 }; + const obj = { ...x }; // crash, same site +} +``` + +Unlike §4.1, this genuinely **is** a missing feature (array spread would need +to synthesize numeric-string-keyed fields `"0"`, `"1"`, …; union spread would +need a runtime type-tag dispatch) rather than an architectural wall — it just +wasn't scoped/implemented for this pass. Converted to: + +```cpp +emitError(location) << "Spread in an object literal is not supported for type: " << to_print(type); +return mlir::failure(); +``` + +Both verified individually (clean diagnostic, no crash) and via the full +suite (`ctest -C Debug -j8`: 829/829, no regressions — these `Default` +branches were never reached by any existing passing test). + +## 5. Inventory of remaining markers (untested this pass) + +Grouped by file. "Shape" is a guess from reading the surrounding code, not a +verified verdict — see §2 for how to actually check one. + +### 5.1 Named/specific (cheapest to investigate next — read the message + local branch, write a 5-line repro) + +| Site | Message | Shape (unverified guess) | +|---|---|---| +| `MLIRGenAccessCall.cpp:1159` | not implemented (ElementAccessExpression) | `boxedObj[computedNonStringExpr]` — cousin of the fixed §4.1 site, one level up in the dispatch (only reached before deciding it's a tuple) | +| `MLIRGenAccessCall.cpp:1219` | not implemented (ElementAccessExpression) | `enumValue[computedExpr]` with a non-constant index | +| `MLIRGenAccessCall.cpp:1524` | not implemented | unread this pass | +| `MLIRGenCast.cpp:1321-1322` | TypeOf NOT IMPLEMENTED for Type | inside a generated `__unbox` 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) | +| `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 | +| `MLIRGenImpl.h:5330` | not implemented | unread | +| `MLIRGenImpl.h:6732` | not implemented | unread | +| `MLIRGenImpl.h:7314` | not implemented | unread | +| `MLIRGenImpl.h:7418` | not implemented | unread | +| `MLIRGenImpl.h:8164` | not implemented | unread | +| `MLIRGenImpl.h:8382` / `:8400` / `:8426` | not implemented | unread, three sites close together — likely related | +| `MLIRGenImpl.h:9342` | not implemented | unread | +| `MLIRGenInterfaces.cpp:475` | not implemented yet | unread | +| `MLIRGenInterfaces.cpp:932` / `:954` | not implemented | unread | +| `MLIRGenTypes.cpp:183` | not implemented type declaration | unread | +| `MLIRGenTypes.cpp:1474` / `:1567` / `:1876` / `:1910` / `:2001` / `:2204` / `:2210` / `:2661` / `:3401` | not implemented | unread, largest single-file cluster after MLIRGenImpl.h | +| `LLVMCodeHelper.h:452` | array literal is not implemented(1) | likely the LLVM-lowering-side twin of the (confirmed-dead) `MLIRGenImpl.h:7875` "object literal is not implemented(1)" `else` branch — check the same way (is there any other `SyntaxKind` an array-literal element list can produce?) | +| `UnaryBinLogicalOrHelper.h:42-43` | "Not implemented operator for type 1: 'X'" (`emitError`) then `llvm_unreachable` | **worth a quick look on its own**: this one already calls `emitError` (like §4's fix) but *still* falls through to `llvm_unreachable` right after — likely a copy-paste of the crash-then-message pattern that never got the `return` that would make the error actually graceful. If so, this is a one-line fix (drop the `llvm_unreachable`, return failure), no new investigation needed beyond confirming what emits it currently crashes instead of erroring cleanly. | + +### 5.2 Generic `TypeSwitch::Default` exhaustiveness fallbacks (likely mostly dead, per §3's precedent — verify by checking whether an existing passing test already produces the relevant `mlir::Type` at that pipeline stage) + +`MLIRGenClasses.cpp:603,635,1802,2269,2306` · `MLIRGenExpressions.cpp:530,552,988` · +`MLIRGenGenerics.cpp:424,542,911,1342` · `MLIRGenImpl.h:3203,3477,3803,4402,4518,6379,7080,7094` · +`MLIRGenInterfaces.cpp:656` · `CastLogicHelper.h:338,344,353,359,460,487,1002` (four of these say +"must be processed at MLIR pass" — suggests a *specific*, documented reason +they should be unreachable at the LLVM-lowering stage, worth reading before +assuming they're arbitrary) · `CodeLogicHelper.h:241` · `OptionalLogicHelper.h:143,213` · +`UndefLogicHelper.h:74,107` · `MLIRCodeLogic.h:1218,1660,1680,1722` · +`MLIRPrinter.h:302-303,534` (type-name printing — a `Default` here would show +up immediately as a printer test failure, so likely easy to check against +`unittests/MLIRGen/TypeToString.cpp`'s existing coverage) · `MLIRTypeIterator.h:403-404` · +`Win32ExceptionPass.cpp:584`. + +### 5.3 RTTI type-switch fallbacks (Windows/Linux variants — the Linux ones can't be exercised from this Windows dev box without a Linux/WSL build) + +`LLVMRTTIHelperVCWin32.h:141,156,169` · `LLVMRTTIHelperVCLinux.h:113,128,142` · +`MLIRRTTIHelperVC.h:108` · `MLIRRTTIHelperVCWin32.h:216,226,241` · +`MLIRRTTIHelperVCLinux.h:146,161,182,202,217,230,399`. + +### 5.4 `MLIRTypeHelper.h`'s `funcRef` family — interesting because directly unit-testable + +`getReturnTypeFromFuncRef` (:732-733), `getParamFromFuncRef` (:755-756), +`getFirstParamFromFuncRef` (:779-780), `getParamsFromFuncRef` (:805-806), +`getParamsTupleTypeFromFuncRef` (:840, `llvm_unreachable` already commented +out at :843 — someone deliberately silenced this one, worth understanding +why before re-enabling), `getVarArgFromFuncRef` (:863-864), plus :410, :420, +:899, :2108, :2256-2257, :2290, :2307, :2685, :2709. Unlike most of §5.2, +these are **pure functions taking an `mlir::Type` and returning a piece of +it** — exactly the shape `unittests/MLIRGen/TypeHelper.cpp` (added this +session, see `declaration-printer-unit-tests`-style memory entries) already +tests other `MLIRTypeHelper.h` functions with. Reachability here is testable +*without* writing a `.ts` repro at all: construct the "wrong" `mlir::Type` +input directly in a unit test and see what real callers expect the sensible +behavior to be, the same way the existing `canWideTypeWithoutDataLoss` tests +work. + +## 6. Suggested next steps, in cost order + +1. `UnaryBinLogicalOrHelper.h:42-43` — likely a one-line fix, already has the + error message, just needs the crash removed. +2. The rest of §5.1 (named/specific) — cheap repro-and-check, same recipe as + §4. +3. §5.4 (`funcRef` family) — extend `unittests/MLIRGen/TypeHelper.cpp` with + direct calls instead of writing `.ts` repros; fast to iterate. +4. §5.2 (generic fallbacks) — triage a handful against existing passing + tests using §3's method before assuming any individual one is live. +5. §5.3 (RTTI) — lowest priority from this (Windows) machine; the Linux + variants need a WSL/Linux build to exercise at all, and even the Windows + ones are deep in a code path (RTTI/exception typeinfo generation) that's + hard to reach without a specific class-hierarchy-plus-exception scenario. + +## 7. Non-goals / out of scope + +- This document does not claim every remaining marker is a "real bug" — + §3 demonstrates at least some meaningful fraction are dead code, and the + true ratio across all ~120 is unknown. +- Not attempting §4.2's actual missing feature (array/union spread into an + object literal) in this pass — only converting its crash into a clean + error. Implementing real array-spread semantics (numeric-string-keyed + field synthesis) is a separate, scoped follow-up if ever prioritized. +- Not attempting a dynamic-property-bag runtime representation for §4.1 — + that is a different object model entirely, out of scope for a + crash-to-error pass. diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index f264b4ad8..5b3982021 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -5792,9 +5792,14 @@ class MLIRGenImpl } else { - LLVM_DEBUG(llvm::dbgs() << "\n!! index value: " << argumentExpression - << ", check if tuple must be an array\n";); - llvm_unreachable("not implemented (index)"); + // tuples/object-literals here compile to a fixed-layout struct (their fields + // are resolved to specific offsets at compile time), not a dynamic hash map - + // a runtime-computed (non-constant) index genuinely can't resolve to a + // specific field, so this is a real language limitation, not a missing code + // path. Fail with a clear diagnostic instead of crashing. + emitError(location) << "Element access with a non-constant index is not supported on this type; " + "only array types and constant keys (obj[\"literal\"]) can be indexed"; + return ValueOrLogicalResult(mlir::failure()); } } @@ -7859,8 +7864,13 @@ class MLIRGenImpl return mlir::success(); }) .Default([&](auto type) { - LLVM_DEBUG(llvm::dbgs() << "\n!! SpreadAssignment not implemented for type: " << type << "\n";); - llvm_unreachable("not implemented"); + // spreading a tuple/const-tuple/interface/class/(boxed) object into an + // object literal is handled above; spreading anything else (an array, + // a union, a primitive...) is a real, unimplemented feature (e.g. array + // spread would need to synthesize numeric-string-keyed fields "0","1",...) + // rather than an unreachable state - fail with a clear diagnostic instead + // of crashing until it's implemented. + emitError(location) << "Spread in an object literal is not supported for type: " << to_print(type); return mlir::failure(); });