-
Notifications
You must be signed in to change notification settings - Fork 833
Unify let, let!, use, use!
LetOrUse
AST representation.
#18825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Ensure that regular let/use bindings (isComputed=false) are matched before let!/use! bindings (isComputed=true) to prevent computation expressions from being treated as regular bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this changes the public API, I think this deserves some form of impact analysis for this change, and some guidance around the change.
https://github.com/search?q=LetOrUseBang+language%3AF%23&type=code&l=F%23
8ddf826
to
596202b
Compare
596202b
to
b1ed90b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider writing a footnote on migration towards AST users.
Can be a footnote inside the release-notes file, pointed to from the main release notes entry.
# Conflicts: # tests/ILVerify/ilverify_FSharp.Compiler.Service_Debug_netstandard2.0.bsl # tests/ILVerify/ilverify_FSharp.Compiler.Service_Release_netstandard2.0.bsl
Head branch was pushed to by a user without write access
Done :) |
let isUse = ($1 = "use") | ||
|
||
mkLetExpression(true, mKeyword, None, m, $8, None, Some(pat, $4, $7, mEquals, isUse)) } | ||
mkLetExpression(true, None, m, $8, None, Some(pat, $4, $7, mKeyword, mEquals, isUse)) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgarfgp It seems the separate rules should not be needed anymore for the computed variants of the bindings too. Could you try unifying them with the the normal ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!!!
Description
This PR unifies the AST representation for binding expressions by consolidating
SynExpr.LetOrUse
andSynExpr.LetOrUseBang
into a singleSynExpr.LetOrUse
node. This change simplifies the AST structure and makes it more consistent for tools working with syntax trees.Key Changes
Removed:
SynExpr.LetOrUseBang
case has been completely removedExtended:
SynExpr.LetOrUse
now includes two additional boolean flags:isFromSource
: Indicates whether the binding originates from user-written code (true) vs compiler-generated (false)isComputed
: Distinguishes computation expression bindings (let!
/use!
= true) from regular bindings (let
/use
= false)Before and After AST Structure
Before (Two Separate Cases)
After (Unified Single Case)
Example 1: Computation Expression with
let!
andand!
Before AST:
After AST:
Example 2: Regular
let
BindingBefore AST:
After AST:
Example 3:
use!
in Computation ExpressionBefore AST:
After AST:
Complete AST Flags Mapping
(isRec, isUse, isFromSource, isComputed)
let x = 42
LetOrUse
(false, false, true, false)
let rec f x = ...
LetOrUse
(true, false, true, false)
use file = ...
LetOrUse
(false, true, true, false)
let! x = asyncOp()
LetOrUse
(false, false, true, true)
use! r = getResource()
LetOrUse
(false, true, true, true)
let! a = opA() and! b = opB()
LetOrUse
(false, false, true, true)
LetOrUse
(false, false, false, false)
LetOrUse
(false, false, false, true)
Understanding the Boolean Flags
isFromSource
true
: User-written code that appears in the source filefalse
: Compiler-generated during transformations (pattern match compilation, CE desugaring, etc.)isComputed
true
: Computation expression binding (let!
oruse!
)false
: Regular binding (let
oruse
)Migration Guidance for Ecosystem AST Users
1. Pattern Matching Updates
Before:
After:
2. Construction Updates
Before:
After:
3. Common Migration Patterns
Checking for computation expressions:
Extracting pattern and expression from let!:
Processing and! bindings:
Breaking Changes
This is a breaking change for any code that pattern matches on or constructs
SynExpr
values. All tools and libraries that work with the AST will need to be updated, including:Checklist