Skip to content

fix(codegen): #37/#321 catch-less try/finally must re-propagate exceptions (not swallow)#1901

Closed
proggeramlug wants to merge 1 commit into
mainfrom
fix-internalcall-finally-37
Closed

fix(codegen): #37/#321 catch-less try/finally must re-propagate exceptions (not swallow)#1901
proggeramlug wants to merge 1 commit into
mainfrom
fix-internalcall-finally-37

Conversation

@proggeramlug
Copy link
Copy Markdown
Contributor

Summary

Fixes a real Perry codegen correctness bug: a try { ... } finally { ... } with no catch clause silently swallows exceptions thrown in the try body, returning undefined instead of re-propagating. Per ECMAScript, the finally must run and then the original exception must propagate.

This is angle (A) (the root correctness fix) from internal task #37 under epic #321 (effect Context/Layer DI). It is a genuine, isolated correctness improvement with a standalone repro and zero regression in Perry's own exception/async/closure/generator test suite.

Important — read the "Effect Layer status" section below before merging. This fix alone does NOT unblock the effect Layer path and in fact regresses the effect survey by unmasking a separate, deeper pre-existing bug (effect's dual/arguments.length). The true #37/#321 blocker is that deeper gap, not try/finally.

Root cause (file:line)

crates/perry-codegen/src/stmt/try_stmt.rslower_try().

The try/finally CFG uses setjmp/longjmp. When the try body throws, control longjmps into the catch_idx block. For a try with a user catch, that block reads the exception and runs the handler. But when there is no catch clause (a try/finally), the block only did js_try_end() and fell through to the shared finally/merge, after which the function emitted the implicit ret undefined — eating the throw.

LLVM evidence (effect's forced arrow (body) => { try { return body() } finally {} }):

try.catch.2:
  call void @js_try_end()
  br label %try.finally.3
try.finally.3:
  ret double 0x7FFC000000000001   ; <- returns undefined, exception lost

Fix

In lower_try, the catch block's no-catch arm now:

  1. captures the pending exception (js_get_exception) before running finally,
  2. runs a dedicated copy of the finally body on the exception path,
  3. re-raises via js_throw (+ unreachable) — unless the finally itself completes abruptly (a return/throw in finally overrides the pending exception, per spec).

The normal-completion path still runs the merge finally and falls through unchanged. +38/-6 lines.

Repro

Standalone (was wrong, now correct):

function inner(): number { throw new Error("boom"); }
function wrap(): number { try { return inner(); } finally {} }
try { console.log("got:", wrap()); } catch (e) { console.log("caught:", (e as Error).message); }
// Node: caught: boom        Perry (before): got: 0        Perry (after): caught: boom

Gap test test-files/test_gap_try_finally_no_catch_rethrow.ts covers: empty finally + throw, non-empty finally + throw, normal completion, indirect call through a function-valued arrow (the effect shape), finally-return override, and nested catch-less try/finally. Byte-identical to node --experimental-strip-types.

How this surfaced (effect)

effect's Utils.ts internalCall picks one of two impls at module load based on whether new Error().stack contains the wrapper's function name (isNotOptimizedAway). On Perry the stack lacks function names, so effect picks the forced impl: (body) => { try { return body() } finally {} }. With the swallow bug, when body() threw, forced returned undefined to the caller (e.g. OP_WITH_RUNTIME in fiberRuntime.ts), presenting as (FiberFailure) Error: {}. Confirmed via instrumentation: body() produced a valid object, yet the caller received undefined, with the corresponding forced "returning" log missing because body() had thrown.

Effect Layer status (NEW state) — why this does not fully unblock #37 yet

With this fix, survey/nlay.ts and survey/real3_context_layer.ts advance past the internalCall swallow (no more Error: {}). They now hit a separate, deeper, pre-existing error: TypeError: value is not a function.

A symbolized backtrace localizes it precisely:

js_throw_type_error_not_a_function
  perry_fn_..._Function_ts__pipe          (effect pipe)
  ..._internal_metric_hook_ts__28
  MetricRegistryImpl__getHistogram        (registry.ts:44 pipe(this.map, MutableHashMap.get(...), Option.getOrUndefined))
  FiberRuntime__reportExitValue           (fiberRuntime.ts:825 fiberLifetimes.unsafeUpdate)
  FiberRuntime__setExitValue / evaluateEffect / start

reportExitValue is gated by runtimeFlags_.runtimeMetrics(flags) which is isEnabled(self, RuntimeMetrics) = (self & 4) !== 0. isEnabled/MutableHashMap.get are built with effect's dual(arity, body), whose returned wrapper branches on arguments.length (if (arguments.length >= 2) ...). Perry currently does not support arguments (compile-time warning: "unknown identifier 'arguments' — bare reads lower to 0"), so the dual wrappers mis-dispatch — runtimeMetrics returns a function instead of false, the metric path runs, and MutableHashMap.get/Option.getOrUndefined resolve to non-functions and throw.

The old swallow behavior was masking this throw (it happens at fiber exit, after the result is computed), so the effect survey "passed". This fix removes the mask, so the survey regresses (11/14 effect-runtime cases) — but the underlying bug was always there. The real #37/#321 Layer blocker is the arguments/dual gap, which is a large, separate feature. Recommend filing it as the next granular blocker; once it lands, this try/finally fix becomes regression-free against the survey.

Validation

  • Gap test test_gap_try_finally_no_catch_rethrow.ts: byte-identical to Node (6 scenarios).
  • Perry exception/try/finally/async sweep (15 files: test_try_catch, test_issue_536_*, test_issue_921/928, test_microtask_inv_06_finally_after_await, etc.): 14/15 byte-identical to Node; the 1 "diff" (test_issue_510_primitive_method_typeerror) is a pre-existing uncaught-error stack-format gap and is identical to baseline (not a regression).
  • Broader sweep (async/closure/generator gap tests): all byte-identical.
  • cargo test -p perry-codegen: pass.
  • cargo fmt --all -- --check: clean. scripts/check_file_size.sh: clean. Workspace build (--no-run, UI crates excluded): green.
  • Effect survey: 01_succeed/30_option/31_data unchanged; the 11 fiber-driven cases regress to the arguments/dual throw described above (root cause separate from this change; documented for the maintainer to gate the merge).

Refs #37, #321. Do not merge until the effect-survey gate is reconciled (see "Effect Layer status").

…tions

A `try { ... } finally { ... }` with no `catch` clause was swallowing
exceptions: the setjmp landing pad ran js_try_end(), fell through to the
shared finally/merge block, and the function returned `undefined` instead
of re-raising. Per ECMAScript the finally must run and then the original
exception must propagate.

Root cause: crates/perry-codegen/src/stmt/try_stmt.rs lower_try() — the
`else` (no user catch) arm of the catch block now captures the pending
exception, runs a dedicated copy of the finally body, and re-raises via
js_throw (unless the finally itself completes abruptly).

Surfaced under effect's internalCall "forced" path
(`try { return body() } finally {}`), which swallowed body()'s return when
body threw, presenting as `(FiberFailure) Error: {}`.

Adds test-files/test_gap_try_finally_no_catch_rethrow.ts (byte-identical
to node --experimental-strip-types).
@proggeramlug
Copy link
Copy Markdown
Contributor Author

Superseded by #1912, which carries this try/finally fix together with the namespace-member-dispatch fix (#39) it was coupled to — #1912 makes the effect survey recover (this fix alone regressed it).

proggeramlug added a commit that referenced this pull request May 27, 2026
…tch member, not fold to ArraySort (carries #1901 try/finally) (#1912)

* fix(codegen): #37/#321 catch-less try/finally must re-propagate exceptions

A `try { ... } finally { ... }` with no `catch` clause was swallowing
exceptions: the setjmp landing pad ran js_try_end(), fell through to the
shared finally/merge block, and the function returned `undefined` instead
of re-raising. Per ECMAScript the finally must run and then the original
exception must propagate.

Root cause: crates/perry-codegen/src/stmt/try_stmt.rs lower_try() — the
`else` (no user catch) arm of the catch block now captures the pending
exception, runs a dedicated copy of the finally body, and re-raises via
js_throw (unless the finally itself completes abruptly).

Surfaced under effect's internalCall "forced" path
(`try { return body() } finally {}`), which swallowed body()'s return when
body threw, presenting as `(FiberFailure) Error: {}`.

Adds test-files/test_gap_try_finally_no_catch_rethrow.ts (byte-identical
to node --experimental-strip-types).

* fix(hir): #39/#321 data-last namespace export `sort`/`reverse` must dispatch member, not fold to ArraySort

A module-namespace export named `sort` (effect's `Array.sort = dual(2, body)`,
a data-last/pipeable function) was mis-lowered to the `js_array_sort` intrinsic
when called as `NS.sort(cmp)`. `sort` (and the other non-overlapping mutators
`reverse`/`entries`/`keys`/`values`/`splice`/`fill`/...) are NOT in the
array-method `is_overlapping` set, so the existing imported-array namespace
guard didn't cover them: the call fell through to `try_array_only_methods`,
whose `"sort" if !args.is_empty()` arm folded `NS.sort(cmp)` into
`Expr::ArraySort { array: NS, comparator: cmp }`. The compiled binary then ran
`js_array_sort(NS, cmp)`, which returned the namespace object itself (typeof
"object") instead of the curried `(self) => sorted` closure, so the downstream
`pipe(bounds, NS.sort(cmp), ...)` threw "value is not a function".

This surfaced in effect's metric histogram path
(`hook.histogram` -> `pipe(bounds, Arr.sort(number.Order), Arr.map(...))`)
reached from `MetricRegistryImpl.getHistogram` at fiber exit (default
`runtimeMetrics` flag is on), breaking every `Effect.runSync(...)` fiber case.

Fix: in `try_array_only_methods`, bail (treat the receiver as not-an-array) for
ALL method names when the receiver identifier is a module namespace import
(`namespace_import_locals`), so the call dispatches the namespace export and the
data-last curried form is returned correctly.

Also carries the held #1901 try/finally fix (catch-less try/finally must
re-propagate exceptions) — that fix unmasked this bug, and the two land as a
pair so #1901 can be closed in favor of this PR.

Refs #39, #321.
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