fix(all): add Seq.enumerateTryWith for try/with in comprehensions#4719
fix(all): add Seq.enumerateTryWith for try/with in comprehensions#4719klofberg wants to merge 0 commit into
Conversation
|
The only red check is It's caused by a CI SDK bump: the job ran under .NET SDK 10.0.109 (via All other checks (JS, TS, Python 3.12–3.14, Rust, Dart, BEAM, analyzers) pass. Could a maintainer reformat |
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) |
Automated reviewWhat it does: Adds a new 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. ( 2. Rust's rethrow path uses 3. Python omits 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 |
|
Implemented review comments |
|
The catch-all cases in this PR work on every target. But a I used Claude Code to troubleshoot and it suggested the following:
What do you think? How should it be implemented? @dbrattli |
|
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. |
|
@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. |
|
@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? |
|
@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 propagatedAlso 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? |
|
@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". |
Yes disabling the test is fine thank you |
|
@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? |
4729dcd to
14f1db6
Compare
|
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 |
Yes, if that's the right way to go, you can send a PR against it, I'll merge it. |
Problem
Using
try ... withinside a list, array, or seq comprehension produces output that imports and callsenumerateTryWithfrom the runtimeSeqmodule — a function that does not exist. On JavaScript the emitted module fails to load at runtime:Compilation reports success; the breakage only surfaces when the emitted module is loaded.
Minimal repro
Root cause
The F# compiler lowers
try/withinside a sequence expression to a call to the intrinsicMicrosoft.FSharp.Core.CompilerServices.RuntimeHelpers.EnumerateTryWith. Fable'sseqModulereplacement automatically maps anyRuntimeHelpers.EnumerateXxxcall toSeq.<lowerFirst Xxx>, so it emitsSeq.enumerateTryWith.The sibling helpers are all implemented in the runtime
Seqmodule —enumerateThenFinally(try/finally),enumerateUsing(use),enumerateWhile(while) — butenumerateTryWithwas never added. Because every target shares the same lowering, all targets are affected, not just JavaScript.Fix
Add an
enumerateTryWithhelper to each runtimeSeq.fs, with the signature the lowering expects: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/withsemantics and the existingenumerate*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'snext : unit -> 'T optionenumerator API; uses a loop rather than a recursive closure, and follows this file's existingfailwith ex.Messageconvention on the rethrow path.src/fable-library-dart/Seq.fs— mirrors the shared implementation.No compiler change is required — the automatic
EnumerateXxx → enumerateXxxmapping is correct; the missing piece was purely the runtime helper.Tests
Added
try/withseq-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.fstests/Dart/src/SeqExpressionTests.fsRust is left function-only (no active test), matching its already-commented-out
try...finally in seqtest.Verification
[1];[ try raise … with _ -> 42 ]→[42]; a mid-sequence catch →[1; 3].Seq.pyexportsenumerate_try_with.