From c535c48a70a2cf256d441673b01f88ef5033daf6 Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Fri, 24 Jul 2026 09:33:38 +0100 Subject: [PATCH] Fix 3 more crashes: import-equals (require/enum targets) and interface methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - mlirGenModuleReference: `import fs = require("fs")` crashed - the function only handled QualifiedName/Identifier module references, never ExternalModuleReference (the parser does support this syntax). tslang compiles as one flat program with no Node.js-style require() at runtime, so this is converted to a clean "not supported" error rather than crash. - mlirGen(ImportEqualsDeclaration...): `import X = SomeEnum` (or any target that isn't a namespace/class/interface) crashed the same way. Fixing the require() crash above surfaced a second, latent bug in this same function: the caller unwrapped mlirGenModuleReference's result with V(result) without ever checking result.failed() first, since the callee previously could only crash or succeed. Added the missing EXIT_IF_FAILED(result) guard. - addInterfaceMethod's empty-name guard: already had llvm_unreachable immediately followed by a dead `return mlir::failure()` - the same "crash-then-already-written-graceful-return" shape as the (confirmed dead) UnaryBinLogicalOrHelper.h case earlier in this audit. Fixed defensively (trivial one-line swap, location already in scope); reachability not verified with a repro this time. Continues the MLIRGenImpl.h/MLIRGenInterfaces.cpp/MLIRGenTypes.cpp cluster sweep (docs/not-implemented-audit.md §4.11). 829/829 ctest, no regressions. --- tslang/docs/not-implemented-audit.md | 86 +++++++++++++++++++++++++--- tslang/lib/TypeScript/MLIRGenImpl.h | 14 ++++- 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/tslang/docs/not-implemented-audit.md b/tslang/docs/not-implemented-audit.md index 5e7836d56..e2ecec392 100644 --- a/tslang/docs/not-implemented-audit.md +++ b/tslang/docs/not-implemented-audit.md @@ -1,6 +1,6 @@ # `llvm_unreachable("not implemented")` audit -Status: **10 confirmed crashes fixed across five passes, ~106 markers still +Status: **12 confirmed crashes fixed across six passes, ~103 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 @@ -10,7 +10,9 @@ priority list while waiting on the first pass's PR to merge. Third pass PR merged. Fourth pass (§4.9) closed out `MLIRGenCast.cpp`'s two `TypeOf` sites, the last item left from the original §5.1 named/specific list. Fifth pass (§4.10) started the `MLIRGenImpl.h` cluster (§5.1's last remaining -block), closing 4 of its sites. +block), closing 4 of its sites. Sixth pass (§4.11) closed 3 more sites in +the same cluster, including two real, easily-reachable `import X = ...` +crashes. ## 1. Scope and method @@ -474,6 +476,69 @@ error even for an unfinished feature. All four verified (repro for the real crash, full-suite for regressions): `ctest -C Debug -j8` → 829/829, no regressions. +### 4.11 `MLIRGenImpl.h` cluster, 3 more sites — 2 real `import X = ...` crashes, 1 defensive fix + +Continues §4.10's sweep of the same cluster (was: 8164, 8382/8400/8426, +9342 remaining after the previous pass; this pass covers 3 of those 4 +entries). + +**`mlirGenModuleReference` (was :8432): a real, easily-reachable crash.** +`import X = require("module")` - the classic CommonJS/Node-interop form of +TypeScript's import-equals syntax, still common in real-world legacy TS - +crashed, because the function only handled `SyntaxKind::QualifiedName` +(`import X = A.B.C`) and `SyntaxKind::Identifier` (`import X = Foo`), never +`SyntaxKind::ExternalModuleReference` (confirmed via the parser: `ts-new- +parser` does have a dedicated `ExternalModuleReference` node and +`isExternalModuleReference` check, so this genuinely parses). Since tslang +compiles as one flat program with no Node.js-style dynamic `require()` at +runtime, there's nothing sensible to resolve this to - converted to a clean +`emitError` instead ("'import X = require(...)' is not supported"). + +**`mlirGen(ImportEqualsDeclaration...)` (was :8458): also a real, +easily-reachable crash**, once the target resolves to anything other than +a namespace, class, or interface - e.g. `import X = SomeEnum` or `import X += someFunction`. Confirmed via repro (`enum Color {...}; import C = +Color;`). Converted to a clean `emitError`. + +Fixing the first crash uncovered a **second, latent bug** in the exact same +function: after making `mlirGenModuleReference` return a graceful +`mlir::failure()` instead of crashing, the `require()` repro immediately +hit a *different* crash - `Assertion failed: detail::isPresent(Val) && +"dyn_cast on a non-existent value"` - because the caller +(`mlirGen(ImportEqualsDeclaration...)`) unwraps the callee's result with +`V(result)` without ever checking `result.failed()` first. That check was +never needed before, because the callee previously only ever *crashed* or +*succeeded* - a graceful failure return was a code path this caller had +never had to handle. Added the missing `EXIT_IF_FAILED(result)` guard +before the `V(result)` unwrap. **Lesson for the rest of this audit**: +converting a `llvm_unreachable` into `return mlir::failure()` can surface a +second bug one level up, in a caller that assumed its callee could never +fail gracefully - always re-run the specific repro after the fix, not just +the full test suite, to catch this. + +**`addInterfaceMethod` (was :9342, the `methodName.empty()` guard): fixed, +but unlike every other fix in this document, reachability was *not* +verified with a repro this time** - this one already had `llvm_unreachable` +immediately followed by a dead `return mlir::failure();` (the exact +"crash-then-already-written-graceful-return" shape flagged early in this +audit for `UnaryBinLogicalOrHelper.h:42-43`, which turned out to be fully +dead code). `location` was already in scope, so the fix was a trivial +one-line swap (`emitError(location, "interface method name cannot be +empty"); return mlir::failure();`), cheap enough to apply defensively +without spending time on a repro attempt (an interface method with a +computed name is one plausible trigger, not investigated). + +Still open from this cluster: `MLIRGenImpl.h:8164` (`processTypeParameter`'s +empty-name branch) and one of the "three close together" sites - the +`TypeAliasDeclaration` empty-name `else` (was :8414, the fourth of the +original 8382/8400/8426 trio once read in full) - both look parser- +grammar-guaranteed unreachable (a type alias or type parameter without a +name shouldn't parse at all) but neither was actually verified against the +parser; left as genuinely unread rather than assumed dead. + +Verified via repro (both real crashes) and the full suite: `ctest -C Debug +-j8` → 829/829, no regressions. + ## 5. Inventory of remaining markers (untested this pass) Grouped by file. "Shape" is a guess from reading the surrounding code, not a @@ -488,13 +553,16 @@ dead (guarded), one a real crash (union with a tuple-shaped member). **Fixed this pass** (§4.10): `MLIRGenImpl.h`'s first four sites (was 5330/6732/7314/7418) — 1 real crash, 2 dead, 1 untested-feature-path. `UnaryBinLogicalOrHelper.h:42-43` row removed - already resolved back in §3 -(dead code, `UnaryOp<>` has zero call sites; see §6 item 1). +(dead code, `UnaryOp<>` has zero call sites; see §6 item 1). **Fixed a +previous pass** (§4.11): `MLIRGenImpl.h`'s next three sites (was +8400/8426/9342 - `import X = require(...)`, `import X = `, and `addInterfaceMethod`'s empty-name guard) — 2 real +crashes, 1 defensive/unverified. | Site | Message | Shape (unverified guess) | | --- | --- | --- | -| `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 | +| `MLIRGenImpl.h:8164` | not implemented | unread — `processTypeParameter`'s empty-name branch, likely parser-grammar-unreachable but not verified | +| `MLIRGenImpl.h:8382` | not implemented | unread — `TypeAliasDeclaration`'s empty-name `else`, likely parser-grammar-unreachable but not verified (the other two of the original "three close together" trio, 8400/8426, were fixed in §4.11) | | `MLIRGenInterfaces.cpp:475` | not implemented yet | unread | | `MLIRGenInterfaces.cpp:932` / `:954` | not implemented | unread | | `MLIRGenTypes.cpp:183` | not implemented type declaration | unread | @@ -573,8 +641,10 @@ bug (the built-in utility types); tracing real callers is what worked. 5. **In progress**: the `MLIRGenImpl.h`/`MLIRGenInterfaces.cpp`/ `MLIRGenTypes.cpp` cluster (§5.1's last remaining block) — read-and-repro each per §2's recipe. §4.10 closed the first 4 `MLIRGenImpl.h` sites - (5330/6732/7314/7418); still open: `MLIRGenImpl.h:8164/8382/8400/8426/9342`, - all of `MLIRGenInterfaces.cpp` (475/932/954), all of `MLIRGenTypes.cpp` + (5330/6732/7314/7418); §4.11 closed 3 more (8400/8426/9342, 2 of them + real `import X = ...` crashes); still open: `MLIRGenImpl.h:8164/8382` + (both look parser-grammar-unreachable but unverified), all of + `MLIRGenInterfaces.cpp` (475/932/954), all of `MLIRGenTypes.cpp` (183/1474/1567/1876/1910/2001/2204/2210/2661/3401), and `LLVMCodeHelper.h:452`. 6. §5.2 (generic fallbacks) — triage a handful against existing passing diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index c122d5270..b142e5293 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -8429,7 +8429,11 @@ class MLIRGenImpl return mlirGen(moduleReference.as(), genContext); } - llvm_unreachable("not implemented"); + // e.g. `import fs = require("fs")` (SyntaxKind::ExternalModuleReference) - + // tslang compiles as one flat program with no Node.js-style dynamic + // require() at all, so there is nothing sensible to resolve this to. + emitError(loc(moduleReference), "'import X = require(...)' is not supported"); + return mlir::failure(); } mlir::LogicalResult mlirGen(ImportEqualsDeclaration importEqualsDeclarationAST, const GenContext &genContext) @@ -8438,6 +8442,7 @@ class MLIRGenImpl if (!name.empty()) { auto result = mlirGenModuleReference(importEqualsDeclarationAST->moduleReference, genContext); + EXIT_IF_FAILED(result) auto value = V(result); if (auto namespaceOp = value.getDefiningOp()) { @@ -8455,7 +8460,10 @@ class MLIRGenImpl return mlir::success(); } - llvm_unreachable("not implemented"); + // e.g. `import X = SomeEnum` or `import X = someFunction` - only + // namespace/class/interface targets are supported for import-equals. + emitError(loc(importEqualsDeclarationAST), "this import-equals target is not supported"); + return mlir::failure(); } return mlir::failure(); @@ -9371,7 +9379,7 @@ class MLIRGenImpl { if (methodName.empty()) { - llvm_unreachable("not implemented"); + emitError(location, "interface method name cannot be empty"); return mlir::failure(); }