Skip to content

[Repo Assist] Fix #323: Promote mutable variables captured by closures to ref cells in QuotationSimplifier#459

Merged
dsyme merged 2 commits intomasterfrom
repo-assist/fix-issue-323-promote-mutables-to-refs-994b9e2a3efb1f28
Feb 26, 2026
Merged

[Repo Assist] Fix #323: Promote mutable variables captured by closures to ref cells in QuotationSimplifier#459
dsyme merged 2 commits intomasterfrom
repo-assist/fix-issue-323-promote-mutables-to-refs-994b9e2a3efb1f28

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant. Fixes #323.

Problem

When a let mutable v = ... variable is captured by a lambda in a quotation used by a generated type provider, the TPSDK IL emitter copies the variable's value into a closure field at lambda-creation time. Mutations inside the closure do not propagate back to the outer scope (and vice versa), causing incorrect runtime behaviour:

let mutable x = 0
let f a =
    x <- x + a       // modifies x, but changes are lost at lambda boundary
    fun b ->
        x <- x + b
        fun c ->
            x <- x + c
let g = f 1
let x0 = x           // was 0, should be 1
let h = g 2
let x1 = x           // was 1, should be 3
h 3
x0, x1, x            // was (0, 1, 3), should be (1, 3, 6)
```

This is the issue identified in #323, which also included a `[(Fact(Skip = "Need to replace captured mutables with refs"))]` test that was never passing.

## Root Cause

The `QuotationSimplifier` transforms source quotations into a form that the TPSDK IL emitter can handle. It had no special handling for `let mutable v` bindings whose variable `v` is a free variable of an inner lambda. The generic `ShapeCombinationUnchecked` fallthrough passed these through unchanged, relying on the downstream emitter to handle mutable capture but `emitLambda` copies values by value into closure fields and has no mechanism to share mutable state.

## Fix

Added two helper functions to `QuotationSimplifier`:

1. **`isCapturedByLambda (v: Var) (expr: Expr)  bool`**  walks `expr` looking for any `ShapeLambda` where `v` appears as a free variable (via `GetFreeVars()`).

2. **`promoteMutableToRef (v: Var) (initExpr: Expr) (body: Expr)  Expr`**  rewrites `let mutable v = initExpr in body` as:
   ```
   let v_ref = ref initExpr in body'
   ```
   where `body'` replaces every `Var(v)` (read) with `!v_ref` and every `VarSet(v, e)` (write) with `v_ref := e'`. This matches how the F# compiler itself handles mutable capture by closures.

A new `Let` case is added in `simplifyExpr` that fires when `v.IsMutable && isCapturedByLambda v bexpr`, triggering the promotion before falling through to the generic cases.

## Trade-offs

- The transformation adds a ref-cell indirection for every mutable variable that is captured by a closure. This is a one-time cost at TP design-time (quotation simplification), not at user runtime.
- Mutable variables that are **not** captured by lambdas are unaffected — they continue to use `VarSet` / direct `stloc` as before.
- The fix is applied unconditionally (not gated on `isGenerated`). For erased TPs the F# compiler handles mutable capture itself, but the transformed quotation (using ref cells) is semantically equivalent, so no regression.

## Test Status

The previously-skipped test `TPSDK.GeneratedCodeTests.lambdas - failing` now passes. Full test suite: **103 passed, 0 failed** on net8.0.

```
Test Run Successful.
Total tests: 103
     Passed: 103
 Total time: 8.77 Seconds

Generated by Repo Assist

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@69c1ee19e39e6aaba35519aafefb5cbf314de323

… in QuotationSimplifier

When a 'let mutable v = ...' variable is used inside a lambda in a quotation
used by a generated type provider, the TPSDK emits IL that copies the variable
value into a closure field at the time the lambda is created. Mutations inside
the lambda (or visible to it via VarSet) are not reflected back to the outer
scope, causing incorrect behavior.

This fix implements the approach suggested in the issue: detect when a mutable
let-bound variable is captured by any lambda in its scope (via isCapturedByLambda),
and promote it to a ref cell (via promoteMutableToRef). The transformation:
  let mutable v = init in body
  =>

The previously-skipped test 'lambdas - failing' in GeneratedCodeTests.fs now
passes, confirming that nested closures correctly share mutable state.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dsyme dsyme marked this pull request as ready for review February 26, 2026 13:22
@dsyme dsyme merged commit 4e5f08b into master Feb 26, 2026
2 checks passed
dsyme pushed a commit that referenced this pull request Mar 7, 2026
🤖 *This PR was created by Repo Assist, an automated AI assistant.*

Prepares the RELEASE_NOTES.md for version **8.3.0** — a minor release
capturing significant improvements merged since 8.2.0 (February 24).

## Changes since 8.2.0

| Type | PR | Description |
|------|----|-------------|
| 🐛 Bug fix | #432 | Fix custom attributes on nested erased types |
| 🐛 Bug fix | #458 | Fix `GetNestedType` on
`TypeSymbol`/`ProvidedTypeSymbol` for generic provided types |
| 🐛 Bug fix | #459 | Fix mutable variable captures in
`QuotationSimplifier` — promote to ref cells |
| ⚡ Performance | #443 | Memoize `transType` in `AssemblyCompiler` to
reduce redundant type translation |
| ⚡ Performance | #457 | Cache `transTypeRef` and `transMethRef` in
assembly compiler |
| ✨ Feature | #428 | New warning when all static parameters in a type
provider are optional |
| 📚 Docs | #455 | Documentation guide overhaul |
| 🧪 Tests | #442 | Add coverage tests and Coverage build target |
| 🔧 Toolchain | #431 | Update to .NET 8 SDK and toolchain |

This is a minor version bump (8.2.0 → 8.3.0) due to the new feature
(#428) and significant improvements. If preferred, a patch release
(8.2.1) is also reasonable given the emphasis on bug fixes.

## Test Status

This PR only modifies RELEASE_NOTES.md. The build/test status for the
underlying changes is tracked in the individual PRs listed above (all
passed CI before merging).

---

*To release: merge this PR, then tag `v8.3.0` and publish the NuGet
package via the existing build pipeline.*




> Generated by [Repo
Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/22448247879)
>
> To install this [agentic
workflow](https://github.com/githubnext/agentics/tree/afb00b92a9514fee9a14c583f059a03d05738f70/workflows/repo-assist.md),
run
> ```
> gh aw add
githubnext/agentics@afb00b9
> ```

<!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, id:
22448247879, workflow_id: repo-assist, run:
https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/22448247879
-->

<!-- gh-aw-workflow-id: repo-assist -->

---------

Co-authored-by: Repo Assist <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Promote mutables to refs when captured by closures

1 participant