Skip to content

fix(all): add Seq.enumerateTryWith for try/with in comprehensions#4719

Closed
klofberg wants to merge 0 commit into
fable-compiler:mainfrom
klofberg:fix/seq-enumerate-try-with
Closed

fix(all): add Seq.enumerateTryWith for try/with in comprehensions#4719
klofberg wants to merge 0 commit into
fable-compiler:mainfrom
klofberg:fix/seq-enumerate-try-with

Conversation

@klofberg

@klofberg klofberg commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Problem

Using try ... with inside a list, array, or seq comprehension produces output that imports and calls enumerateTryWith from the runtime Seq module — a function that does not exist. On JavaScript the emitted module fails to load at runtime:

SyntaxError: The requested module './fable_modules/fable-library-js.5.5.0/Seq.js'
does not provide an export named 'enumerateTryWith'

Compilation reports success; the breakage only surfaces when the emitted module is loaded.

Minimal repro

let x = [
    try 1
    with _ -> 0
]
printfn "%A" x   // expected: [1]

Root cause

The F# compiler lowers try/with inside a sequence expression to a call to the intrinsic Microsoft.FSharp.Core.CompilerServices.RuntimeHelpers.EnumerateTryWith. Fable's seqModule replacement automatically maps any RuntimeHelpers.EnumerateXxx call to Seq.<lowerFirst Xxx>, so it emits Seq.enumerateTryWith.

The sibling helpers are all implemented in the runtime Seq module — enumerateThenFinally (try/finally), enumerateUsing (use), enumerateWhile (while) — but enumerateTryWith was never added. Because every target shares the same lowering, all targets are affected, not just JavaScript.

Fix

Add an enumerateTryWith helper to each runtime Seq.fs, with the signature the lowering expects:

enumerateTryWith : seq<'T> -> (exn -> int) -> (exn -> seq<'T>) -> seq<'T>

It enumerates the source; when an exception propagates it runs the filter (catchFilter): non-zero switches to the handler sequence and continues, zero re-raises. This mirrors F# try/with semantics and the existing enumerate* helpers.

  • src/fable-library-ts/Seq.fs — covers JavaScript, TypeScript, and BEAM (BEAM links this file).
  • src/fable-library-py/fable_library/Seq.fs — adds a new function enumerateTryWith (emitted as enumerate_try_with in the generated Python) without altering any existing function, so it's backward-compatible under the PyPI within-minor-version policy.
  • src/fable-library-rust/src/Seq.fs — adapted to Rust's next : unit -> 'T option enumerator API; uses a loop rather than a recursive closure, and follows this file's existing failwith ex.Message convention on the rethrow path.
  • src/fable-library-dart/Seq.fs — mirrors the shared implementation.

No compiler change is required — the automatic EnumerateXxx → enumerateXxx mapping is correct; the missing piece was purely the runtime helper.

Tests

Added try/with seq-expression cases (no-exception, exception-caught, array form, mid-sequence catch preserving already-yielded elements, and unmatched-exception rethrow) to:

  • tests/Js/Main/SeqExpressionTests.fs (JavaScript + TypeScript)
  • tests/Python/TestSeqExpression.fs
  • tests/Dart/src/SeqExpressionTests.fs

Rust is left function-only (no active test), matching its already-commented-out try...finally in seq test.

Verification

  • JavaScript quicktest on the exact repro now prints [1]; [ try raise … with _ -> 42 ][42]; a mid-sequence catch → [1; 3].
  • JavaScript full test suite passes (no new failures).
  • Runtime libraries build cleanly for all six targets (JS, TS, Python, Rust incl. cargo, Dart, BEAM); the generated Python Seq.py exports enumerate_try_with.

@klofberg

klofberg commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

verify-linting failure is unrelated to this PR

The only red check is verify-linting, on src/fable-library-dart/Map.fs — a file this PR doesn't touch (byte-identical to main, last changed 2025-08-13).

It's caused by a CI SDK bump: the job ran under .NET SDK 10.0.109 (via global.json rollForward: latestPatch), and Fantomas 7.0.5 reformats Map.fs under it. Locally the file is clean with the same Fantomas on SDK 10.0.108/10.0.203. main's next push will hit the same failure.

All other checks (JS, TS, Python 3.12–3.14, Rust, Dart, BEAM, analyzers) pass. Could a maintainer reformat Map.fs on main under the current SDK, or pin the SDK patch? Happy to fold it into this PR if preferred.

@dbrattli

dbrattli commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Could a maintainer reformat Map.fs

It's an intermittent bug in Fantomas (1 in 10'ish runs). It sometimes removes comments after an expression. Hard to reproduce and submit a bug report, so the easiest way for now until we figure this out is just to retrigger the failing job in CI. FYI: @nojaf

This is what Fantomas wants to do in 1 out of 10'ish runs (locally, almost always fails in CI)

-        else if t1h > t2h + tolerance then // left is heavier than right
+        else if t1h > t2h + tolerance then 

@dbrattli

dbrattli commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Automated review

What it does: Adds a new enumerateTryWith runtime helper to the four Seq.fs files (ts/dart/py/rust) so try/with inside list/array/seq comprehensions works. The core mechanism checks out: the EnumerateTryWith → Seq.enumerateTryWith mapping, the (source, catchFilter, catchHandler) argument order, and the filter polarity (<> 0 matches FSharp.Core's = 1, since the compiler-generated filter only ever returns 0 or 1) are all correct, and the JS/TS/Dart enumerator logic is sound. The findings below are edge/quality issues, not happy-path breaks.

Findings (most severe first)

1. The "unmatched-exception rethrow" test the PR body claims was added does not exist — leaving the only per-target-divergent branch untested. (tests/Js/Main/SeqExpressionTests.fs + Python/Dart equivalents)
All four added test cases use a catch-all with _, so catchFilter always returns non-zero and the reraise() / failwith branch (filter = 0) is never exercised on any target. The PR description lists "unmatched-exception rethrow" among the added cases, but no such test is in the diff. A regression that swallowed an unmatched exception instead of propagating it would ship undetected. A filtered handler such as try raise (exn "boom") with :? System.InvalidOperationException -> 0 (expecting the exn to propagate) would cover it.

2. Rust's rethrow path uses failwith ex.Message instead of reraise(), diverging from ts/dart/py and losing the exception type/stack. (src/fable-library-rust/src/Seq.fs)
For F# code that catches by type downstream (with :? SpecificException as e -> …), an unmatched exception propagating out of a Rust comprehension arrives as a generic message-only exception, so a subsequent :? OriginalType match fails where it would succeed on other targets. This is consistent with the file's existing finallyEnumerable convention (which does the same), and Rust has no test for the function — but combined with #1 it means the diverging branch is both unverified and observably different. Worth a comment acknowledging the known limitation, or a Rust test.

3. Python omits state.Source <- None before reassigning the handler enumerator, unlike ts/dart/rust, allowing a double-dispose if the handler's GetEnumerator() throws. (src/fable-library-py/fable_library/Seq.fs)
If (catchHandler ex).GetEnumerator() raises, the assignment never lands, so state.Source still references the already-disposed source; generateWhileSome's reraise path then disposes it a second time. Low severity (a seq expression's GetEnumerator essentially never throws, and most Fable enumerators dispose idempotently), but it's a gratuitous cross-target inconsistency with a trivial one-line fix that matches the other three implementations.

Nothing on the primary success paths (no-exception, matched-catch, mid-sequence catch, disposal on normal completion) survived verification — those are correct and well-covered.

🤖 Generated with Claude Code

@klofberg

klofberg commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Implemented review comments

@klofberg

klofberg commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

The catch-all cases in this PR work on every target. But a try/with in a comprehension that catches a specific exception type instead of catch-all fails static type-checking on TypeScript and Dart:

error TS2322: Type 'number' is not assignable to type 'Iterable<number>'.
dart: Compilation failed

I used Claude Code to troubleshoot and it suggested the following:

  1. Fix it in FCS — switch the handler to Rethrow in CheckSequenceExpressions.fs, matching the normal try/with path. This is the root-cause fix. It likely needs a companion Fable change though: the resulting reraise() lives in a standalone handler lambda (exn -> seq<'T>) that isn't lexically a try/with handler, so ctx.CaughtException isn't set and the reraise replacement in Replacements.fs would addError. We'd need to set CaughtException to the handler's exn parameter when transforming the EnumerateTryWith handler.

  2. Fix it in Fable only — at the EnumerateTryWith call site, rewrite the mistyped dead fallback into a bottom-typed makeThrow (mirroring the existing RaisingMatchFailureExprmakeThrow handling in FSharp2Fable.fs). A throw is never/Never/!-typed, so it type-checks on all targets including Rust, and it stays dead code. No FCS change required.

What do you think? How should it be implemented? @dbrattli

@MangelMaxime

Copy link
Copy Markdown
Member

I think it depends on where the issue really is.

If this is a bug in FCS, we should probably report an issue to FCS repository. Even if we are using a custom fork of FCS in Fable, in general we try to not modify it too much as we are not doing an hard-fork. We modify it so we can make it compile and runtime via Fable.

Plus perhaps some other minors change @ncave would know better than me.

@ncave

ncave commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

@klofberg Thank you for your contribution!

I agree, if this is a bug in FCS, we should log the issue in the F# repository, but it may take time to get it approved. So if we can fix it here for Fable, that's probably the quickest way to go.

@klofberg But if you prefer to go the FCS route, if it's really an FCS issue that will have to be resolved anyway, we do have the option of speeding things a bit by incorporating the fix in our FCS fork while waiting on the approval to merge it upstream. You can make a PR against this FCS fork branch, if you think this is easier/better.

@klofberg

klofberg commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@ncave I made a temporary fix to make the tests pass. I planning on logging an issue in the FCS repo and I might help out with a PR in the Fable FCS fork (If I feel that I understand what I'm doing :-) ). Is it OK to merge this PR with the temporary fix?

@MangelMaxime

Copy link
Copy Markdown
Member

@klofberg Can you please add tests for Beam and Rust ?

Note that this test will fails because of a bug in Beam but that bug can be fixed later.

[<Fact>]
let ``test try...with in seq expressions rethrows unmatched exceptions`` () =
    let mutable propagated = false
    try
        seq {
            try
                yield 1
                raise (System.InvalidOperationException "boom")
                yield 2
            with :? System.ArgumentException ->
                yield 0
        }
        |> Seq.toList
        |> ignore
    with :? System.InvalidOperationException ->
        propagated <- true
    equal true propagated

Also could you please check the box "Allow edits from maintainers" this would allows us (maintainers) to contribute to this PR instead of closing / opening a new one?

@klofberg

klofberg commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@MangelMaxime Will the PR be able to be completed with a failing test or am I misunderstanding what you mean? Should I add a disabled test? Also I checked "Allow edits from maintainers".

@MangelMaxime

Copy link
Copy Markdown
Member

Should I add a disabled test?

Yes disabling the test is fine thank you

@klofberg

klofberg commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@ncave I've created an issue for FCS dotnet/fsharp#20040 . Should I implement the fix in https://github.com/ncave/fsharp/tree/service_slim or is it OK to use my workaround (8fef787) for the time being?

@MangelMaxime MangelMaxime force-pushed the fix/seq-enumerate-try-with branch from 4729dcd to 14f1db6 Compare July 8, 2026 17:44
@MangelMaxime

Copy link
Copy Markdown
Member

I don't know why I have trouble pushing to your branch/fork, and while trying to fix it it auto closed the PR.

I am re-opening it at #4750 sorry

@ncave

ncave commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

@klofberg

Should I implement the fix in https://github.com/ncave/fsharp/tree/service_slim

Yes, if that's the right way to go, you can send a PR against it, I'll merge it.
That branch is a bit older than upstream edge, but we can rebase it later.

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.

4 participants