Unify `let, let!, use, use!` `LetOrUse` AST representation. by edgarfgp · Pull Request #18816 · dotnet/fsharp

edgarfgp · 2025-08-04T16:46:51Z

Description

Fixes # (issue, if applicable)

Checklist

Test cases added
Release notes entry updated

…x-attr-targets

github-actions · 2025-08-04T16:47:36Z

❗ Release notes required

@edgarfgp,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

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

Change path	Release notes path	Description
`src/Compiler`	docs/release-notes/.FSharp.Compiler.Service/10.0.100.md	No release notes found or release notes format is not correct

edgarfgp · 2025-08-04T16:53:52Z

@auduchinok Can you please have a look at the AST changes.? Once we are happy with these will double check the type checking files.

Copilot

Pull Request Overview

This PR unifies the AST representation for let, let!, use, and use! expressions by consolidating them into a single LetOrUse variant. Previously, F# had separate AST nodes for regular let/use expressions and bang-form let!/use! expressions (LetOrUseBang), which led to code duplication and complexity in handling these constructs.

Removes the SynExpr.LetOrUseBang AST variant entirely
Extends SynExpr.LetOrUse with additional flags (isFromSource and isComputed) to handle all cases
Updates all related parsing, checking, and analysis code to work with the unified representation

Reviewed Changes

Copilot reviewed 85 out of 85 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/Compiler/SyntaxTree/SyntaxTree.fs	Removes LetOrUseBang variant and extends LetOrUse with additional parameters
src/Compiler/SyntaxTree/ParseHelpers.fs	Updates mkLetExpression to create unified LetOrUse nodes for all cases
src/Compiler/Checking/Expressions/CheckComputationExpressions.fs	Major refactoring to work with unified AST representation
src/Compiler/Service/*.fs	Updates various service modules to handle unified representation
tests/service/data/SyntaxTree/SynType/*.bsl	Updates baseline test files to reflect new AST structure
tests/FSharp.Compiler.Service.Tests/*.bsl	Updates surface area baselines for API changes

src/Compiler/SyntaxTree/ParseHelpers.fs

src/Compiler/Checking/Expressions/CheckComputationExpressions.fs

auduchinok

@edgarfgp This is really good work 🙂

I think we can remove a lot of computed let handling from the tree traversal and similar utilities. I'd expect the normal let logic to work as expected there.

auduchinok · 2025-08-05T12:12:55Z

src/Compiler/Service/ServiceInterfaceStubGenerator.fs

-                | SynExpr.LetOrUseBang(rhs = synExpr1; andBangs = synExprAndBangs; body = synExpr2) ->
-                    [
-                        yield synExpr1
-                        for SynBinding(expr = eAndBang) in synExprAndBangs do
-                            yield eAndBang
-                        yield synExpr2
-                    ]
-                    |> List.tryPick walkExpr
-


Not a suggestion in this particular case, but we could in theory match

| SynExpr.LetOrUse(isComputed = true; bindings = synBindingList; body = synExpr) ->

to minimize the diff in cases like this.

auduchinok · 2025-08-05T12:14:04Z

src/Compiler/Service/ServiceInterfaceStubGenerator.fs

+                    if isComputed then
+                        [
+                            for SynBinding(expr = bindingExpr) in synBindingList do
+                                yield bindingExpr
+                            yield synExpr
+                        ]
+                        |> List.tryPick walkExpr
+                    else
+                        Option.orElse (List.tryPick walkBinding synBindingList) (walkExpr synExpr)


I wonder if different logic is actually needed here. Would it work as expected if we just reuse the LetOrUse logic for the computed variants? It seems the tree traversal should be the same.

auduchinok · 2025-08-05T12:15:14Z

src/Compiler/Service/FSharpParseFileResults.fs

-                            for SynBinding(debugPoint = andBangSpBind; expr = eAndBang) in andBangs do
-                                yield! walkBindSeqPt andBangSpBind
-                                yield! walkExpr true eAndBang
+                        | SynExpr.LetOrUse(isComputed = true; bindings = bindings; body = bodyExpr) ->


Isn't it covered on line 700?

auduchinok · 2025-08-05T12:17:47Z

src/Compiler/Driver/GraphChecking/FileContentMapping.fs

            visit expr (fun exprNodes -> exprNodes @ List.collect visitSynType typeArgs |> continuation)
-        | SynExpr.LetOrUse(bindings = bindings; body = body) ->
-            visit body (fun nodes -> List.collect visitBinding bindings @ nodes |> continuation)
+        | SynExpr.LetOrUse(isComputed = isComputed; bindings = bindings; body = body) ->


Likewise, do we need to distinguish the computed variant here? I'd expect the logic from LetOrUse to be reused.

auduchinok · 2025-08-05T12:21:23Z

src/Compiler/SyntaxTree/ParseHelpers.fs

+                true, // isFromSource is true for user-written code
+                false, // isComputed is false for let/use


Please move these comments to SyntaxTree.fsi.

edgarfgp · 2025-08-05T18:46:50Z

@auduchinok Thanks for the review. I trying making this compile locally but as soon I removed the LetOrUseBang case from the SyntaxTree file and build it again using build.sh it just does not work. Not sure It seems like a compiler bootstrapping issue.

I would love to unify both constructs. but not luck so far.

Edit: This issue is when trying to build locally FsharpCore with the newly updated parser. @T-Gro have you seen this before by any chance ?

T-Gro · 2025-08-06T14:06:29Z

No, have not seen it.
Did you try with a clean folder state, deleting all artifacts from previous work?

edgarfgp · 2025-08-06T15:52:14Z

No, have not seen it. Did you try with a clean folder state, deleting all artifacts from previous work?

Yes. usual stuff , but something seems to be happening to FSharpCore and I can not figure it out.

majocha · 2025-08-06T17:15:18Z

I tried to build the branch from scratch and the first error I got was

E:\repos\fsharp\src\FSharp.Core\async.fs(1531,48): error FS0001: This expression was expected to have type    'CancellationToken'    but here has type    'Async<Cancellat
ionToken>' [E:\repos\fsharp\src\FSharp.Core\FSharp.Core.fsproj::TargetFramework=netstandard2.0]

It looks like it encountered a let! but treated it as let. Normal LetOrUse matched before the isComputed match?

edgarfgp · 2025-08-06T21:38:10Z

@majocha Thanks, You were on to something. It build now locally.

edgarfgp and others added 30 commits April 23, 2025 15:27

make attribute targets mismatches a warning and not an error.

1799aaf

release notes

55507e9

update tests

1738018

Merge branch 'main' into fix-attr-targets

65f5bb6

Merge branch 'main' into fix-attr-targets

0c97b9d

update baselines

6f2b706

Merge branch 'main' into fix-attr-targets

e8f1bb0

Update baselines

75d8f5e

Merge branch 'fix-attr-targets' of github.com:edgarfgp/fsharp into fi…

4f2e97e

…x-attr-targets

Merge branch 'main' into fix-attr-targets

63be5d5

Move attribute form logic to an AP

4248f2a

Merge branch 'main' into fix-attr-targets

cc96217

Merge branch 'main' into fix-attr-targets

e270b88

Merge branch 'main' into fix-attr-targets

e0cc65a

Merge branch 'main' into fix-attr-targets

1e29d58

Merge branch 'main' into fix-attr-targets

1470bf9

Merge branch 'main' into fix-attr-targets

8988215

Merge branch 'main' into fix-attr-targets

74712e8

Merge branch 'main' of github.com:edgarfgp/fsharp

967c4a9

Merge branch 'dotnet:main' into main

a30cef4

Merge branch 'dotnet:main' into main

5fa0480

Merge branch 'dotnet:main' into main

15e3d34

Merge branch 'dotnet:main' into main

b7ffcf8

Merge branch 'dotnet:main' into main

5bde641

Merge branch 'dotnet:main' into main

0f7c23c

Merge branch 'dotnet:main' into main

6a6843c

Merge branch 'dotnet:main' into main

d11dd4a

Remove LetOrUseBang

9d3c0f5

Update syntax tree tests

6cf1a9c

update baselines

a0005ec

Update type checking to use LetOrUse

f6c80c7

github-project-automation bot added this to F# Compiler and Tooling Aug 4, 2025

github-project-automation bot moved this to New in F# Compiler and Tooling Aug 4, 2025

Merge branch 'main' into unify-let-or-use

720eec5

Update CheckComputationExpressions.fs

d380ef0

edgarfgp force-pushed the unify-let-or-use branch from 9f41ed1 to d380ef0 Compare August 4, 2025 19:15

edgarfgp added 4 commits August 4, 2025 21:21

Update CheckExpressionsOps.fs

1b2d9e5

Update FileContentMapping.fs

05e88b7

Update FSharpParseFileResults.fs

ee9397a

Service updates

b3db637

edgarfgp force-pushed the unify-let-or-use branch from 8872d00 to b3db637 Compare August 5, 2025 05:36

edgarfgp requested a review from Copilot August 5, 2025 09:56

Copilot AI reviewed Aug 5, 2025

View reviewed changes

src/Compiler/SyntaxTree/ParseHelpers.fs Outdated Show resolved Hide resolved

src/Compiler/Checking/Expressions/CheckComputationExpressions.fs Outdated Show resolved Hide resolved

src/Compiler/Checking/Expressions/CheckComputationExpressions.fs Outdated Show resolved Hide resolved

Update CheckComputationExpressions.fs

f960746

edgarfgp force-pushed the unify-let-or-use branch from b8f7ad6 to f960746 Compare August 5, 2025 10:41

edgarfgp closed this Aug 5, 2025

github-project-automation bot moved this from New to Done in F# Compiler and Tooling Aug 5, 2025

auduchinok reviewed Aug 5, 2025

View reviewed changes

edgarfgp mentioned this pull request Aug 6, 2025

Unify let, let!, use, use! LetOrUse AST representation. #18825

Merged

2 tasks

		true, // isFromSource is true for user-written code
		false, // isComputed is false for let/use

Conversation

edgarfgp commented Aug 4, 2025

Description

Checklist

Uh oh!

github-actions bot commented Aug 4, 2025

❗ Release notes required

Uh oh!

edgarfgp commented Aug 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

auduchinok left a comment

Choose a reason for hiding this comment

Uh oh!

auduchinok Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

auduchinok Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

auduchinok Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

auduchinok Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

auduchinok Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

edgarfgp commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

T-Gro commented Aug 6, 2025

Uh oh!

edgarfgp commented Aug 6, 2025

Uh oh!

majocha commented Aug 6, 2025

Uh oh!

edgarfgp commented Aug 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edgarfgp commented Aug 5, 2025 •

edited

Loading