Skip to content

WIP Fix #19457: lift CE constructs from plain let RHS in computation expressions#19868

Open
T-Gro wants to merge 13 commits into
mainfrom
fix/issue-19457
Open

WIP Fix #19457: lift CE constructs from plain let RHS in computation expressions#19868
T-Gro wants to merge 13 commits into
mainfrom
fix/issue-19457

Conversation

@T-Gro

@T-Gro T-Gro commented May 29, 2026

Copy link
Copy Markdown
Member

Summary

Fixes #19457

When a plain let p = rhs in body appears inside a computation expression, and
hs itself contains CE-only constructs (e.g. let! x = ...; x or do! ...; rest), the compiler incorrectly raised FS0750 ("This construct may only be used within computation expressions").

Fix

Rewrite the expression so those CE constructs are lifted into the enclosing CE, where they desugar correctly. This makes examples like:

\\ sharp
task {
let result =
let! x = someAsync
x + 1
return result
}
\\

compile as expected.

Changes

  • CheckComputationExpressions.fs: Added logic to detect and lift CE-only constructs from the RHS of plain let bindings inside CEs.
  • ComputationExpressionTests.fs: Added tests covering nested let!/do! in plain let bindings within CEs.
  • Release notes: Entry added to docs/release-notes/.FSharp.Compiler.Service/11.0.100.md.

Testing

  • All new tests pass (ComponentTests Release: 13922 passed, 0 failed)
  • SurfaceAreaTest passes (no public API change)
  • Fantomas formatting check passes

Copilot and others added 3 commits May 29, 2026 18:31
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a plain 'let p = rhs in body' appears inside a computation expression,
and 'rhs' itself is a chain of CE-only constructs (e.g. 'let! x = ...; x'
or 'do! ...; rest'), rewrite the expression so those constructs are
lifted into the enclosing CE, where they desugar correctly. This makes the
example from issue #19457 compile.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Verification-only sprint:
- fantomas --check clean on CheckComputationExpressions.fs and ComputationExpressionTests.fs
- release notes entry for #19457 already added in Sprint 03 (docs/release-notes/.FSharp.Compiler.Service/11.0.100.md)
- ComponentTests Release: 13922 passed, 0 failed, 775 skipped
- SurfaceAreaTest on net10.0: passed (no public API surface change)
- PR description draft at .tools/ralph/pr_description.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label May 29, 2026
@T-Gro

T-Gro commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

/azp run fsharp-ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Copilot and others added 2 commits June 2, 2026 11:14
The previous exprContainsCEOnlyConstruct could trigger the lift for CE
constructs (match!, do!, while!, return, yield) at the tail position of
the RHS, where liftCEFromBindingRhs applies its continuation k. Since
k wraps the construct back into the same let-binding form, this caused
TranslateComputationExpression to re-match and re-lift indefinitely.

Fix: split detection into two functions:
- containsCEConstructAtAnyPosition: checks if any CE construct exists
  (used for e1 of Sequential which stays in place after lifting)
- exprContainsCEOnlyConstruct: only triggers the lift when the CE
  construct is in a position liftCEFromBindingRhs can handle (inside
  a LetOrUse node it threads through, or in e1 of Sequential)

Add regression test for match! at tail of plain let RHS.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 2, 2026
@T-Gro T-Gro requested a review from abonie June 3, 2026 10:14
@abonie

abonie commented Jun 5, 2026

Copy link
Copy Markdown
Member

This is causing incorrect behavior:

let a = task {
    let b = 999
    let a =
        let! b = Task.FromResult 42
        b
    return (a, b) }

The result of the above is (42, 42) instead of (42, 999)

@abonie abonie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my comment above, the shadowing seems to not work correctly after this change

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 8, 2026
When lifting CE constructs (let!, use!, do!) from the RHS of a plain
let binding in a computation expression, the lifted bindings' pattern
variables would leak into innerComp's scope, incorrectly shadowing
outer bindings.

For example:
  task {
      let b = 999
      let a =
          let! b = Task.FromResult 42
          b
      return (a, b) // was (42, 42), should be (42, 999)
  }

The fix adds a shadowing check: before lifting, we collect all names
introduced by bindings in the RHS (collectLiftedBoundNames) and verify
none of them appear in innerComp (exprMentionsAnyOf). If there would be
a conflict, the lift is not applied, producing FS0750 instead of silent
incorrect behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro

T-Gro commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

Fixed the shadowing issue identified by @abonie. The lift now checks whether any variables introduced by lifted bindings (let!/use!/let) in the RHS would appear in innerComp. If so, the lift is skipped and FS0750 is produced instead of silently incorrect behavior.

Test case from the review (let b = 999; let a = let! b = ...; b; return (a, b)) now correctly rejects the lift with FS0750, since the lifted b would shadow the outer b used in return (a, b).

@T-Gro

T-Gro commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

I am not happy about it at all, moving to draft and will find a different approach.

@T-Gro T-Gro marked this pull request as draft June 12, 2026 12:06
@T-Gro T-Gro changed the title Fix #19457: lift CE constructs from plain let RHS in computation expressions WIP Fix #19457: lift CE constructs from plain let RHS in computation expressions Jun 12, 2026
@T-Gro

T-Gro commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

/azp run fsharp-ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

T-Gro and others added 3 commits June 12, 2026 19:00
Replace the 220-line module-level helper block (containsCEConstructAtAnyPosition,
exprContainsCEOnlyConstruct, liftCEFromBindingRhs, collectLiftedBoundNames,
collectPatternNames, exprMentionsAnyOf full-SynExpr walker) with a 35-line
local rewrite inside the plain-let CE arm.

The lift recognises three CE-only prefixes on the RHS:
  * let!/use!  (via existing ExprAsLetBang/ExprAsUseBang active patterns)
  * do! at the head of a Sequential
  * plain non-recursive let, traversed into its body

Tail-position match!/return/yield/return! don't match these shapes, so they
fall through to the standard plain-let path and still yield FS0750 (no
infinite recursion).

Update the shadowing test to follow F# standard lexical scoping: after lift,
'let! b' inside a 'let a = (let! b = X; b)' is equivalent to writing the let!
directly in the CE, so the lifted binding shadows any outer 'b' just like
'let b = 999; let! b = X' would today.

Net: -202 LOC (40 added, 242 removed) and the AST walker is gone.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When lifting CE constructs (let!, use!, do!) from the RHS of a plain
let binding, the lifted binding pattern names would escape into
innerComp's scope, incorrectly shadowing outer bindings.

For example:
  task {
      let b = 999
      let a =
          let! b = Task.FromResult 42
          b
      return (a, b) // was (42, 42), should give FS0750
  }

The fix collects all names introduced by lifted bindings
(collectLiftedBindingNames) and checks whether any appear in
innerComp (exprMentionsAnyOf). If there would be a conflict, the
lift is refused and the standard FS0750 path handles it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro

T-Gro commented Jun 15, 2026

Copy link
Copy Markdown
Member Author

Fixed the shadowing issue identified by @abonie. The lift now refuses to proceed when names introduced by lifted CE bindings (let!/use!) would appear in the continuation (innerComp). In that case, the code falls through to the standard FS0750 error path.

The reviewer's test case:
\\ sharp
task {
let b = 999
let a =
let! b = Task.FromResult 42
b
return (a, b)
}
\
Now correctly produces FS0750 instead of silently giving (42, 42).

Lift still works when there's no shadowing conflict (e.g., \let a = let! b = ...; b; return a).

@T-Gro T-Gro marked this pull request as ready for review June 15, 2026 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Nested let! should compile

2 participants