Skip to content

WIP :: Bugfix :: Fix closure capture and let-rec initialization codegen #19339

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.

Sign up for GitHub

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

Draft
T-Gro wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP :: Bugfix :: Fix closure capture and let-rec initialization codegen #19339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/release-notes/.FSharp.Compiler.Service/10.0.300.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
* Fix FS3356 false positive for instance extension members with same name on different types, introduced by [#18821](https://github.com/dotnet/fsharp/pull/18821). ([PR #19260](https://github.com/dotnet/fsharp/pull/19260))
* Fix graph-based type checking incorrectly resolving dependencies when the same module name is defined across multiple files in the same namespace. ([PR #19280](https://github.com/dotnet/fsharp/pull/19280))
* F# Scripts: Fix default reference paths resolving when an SDK directory is specified. ([PR #19270](https://github.com/dotnet/fsharp/pull/19270))
* Fix object expressions in struct types no longer generate invalid IL with byref fields. (Issue [#19068](https://github.com/dotnet/fsharp/issues/19068), [PR #19339](https://github.com/dotnet/fsharp/pull/19339))
* Avoid duplicate parameter names in closure constructors. (Issue [#17692](https://github.com/dotnet/fsharp/issues/17692), [PR #19339](https://github.com/dotnet/fsharp/pull/19339))
* Improve let-rec codegen: reorder bindings to allocate lambda closures before non-lambda values that reference them. ([PR #19339](https://github.com/dotnet/fsharp/pull/19339))

### Added
* FSharpType: add ImportILType ([PR #19300](https://github.com/dotnet/fsharp/pull/19300))
Expand Down
21 changes: 19 additions & 2 deletions src/Compiler/CodeGen/EraseClosures.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,21 @@ let mkILFreeVarForParam (p: ILParameter) =

let mkILLocalForFreeVar (p: IlxClosureFreeVar) = mkILLocal p.fvType None

// Note: This is similar to ChooseFreeVarNames in IlxGen.fs but operates on
// IlxClosureFreeVar[] instead of string lists. Kept separate to avoid cross-file dependency.
Comment on lines +395 to +396
Copy link
Member

@auduchinok auduchinok Feb 23, 2026

Choose a reason for hiding this comment

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

Can this be extracted and unified somehow instead? If the logic is the same, we probably want to update it together in future, so producing duplicate code doesn't sound good?

let mkUniqueFreeVarName (baseName: string) (existingFields: IlxClosureFreeVar[]) =
let existingNames = existingFields |> Array.map (fun fv -> fv.fvName) |> Set.ofArray

let rec findUnique n =
let candidate = if n = 0 then baseName else baseName + string n

if Set.contains candidate existingNames then
findUnique (n + 1)
else
candidate

findUnique 0

let mkILCloFldSpecs _cenv flds =
flds |> Array.map (fun fv -> (fv.fvName, fv.fvType)) |> Array.toList

Expand Down Expand Up @@ -490,7 +505,8 @@ let rec convIlxClosureDef cenv encl (td: ILTypeDef) clo =
let laterGenericParams = td.GenericParams @ addedGenParams

let selfFreeVar =
mkILFreeVar (CompilerGeneratedName("self" + string nowFields.Length), true, nowCloSpec.ILType)
let baseName = CompilerGeneratedName("self" + string nowFields.Length)
mkILFreeVar (mkUniqueFreeVarName baseName nowFields, true, nowCloSpec.ILType)

let laterFields = Array.append nowFields [| selfFreeVar |]
let laterCloRef = IlxClosureRef(laterTypeRef, laterStruct, laterFields)
Expand Down Expand Up @@ -612,7 +628,8 @@ let rec convIlxClosureDef cenv encl (td: ILTypeDef) clo =
let laterGenericParams = td.GenericParams
// Number each argument left-to-right, adding one to account for the "this" pointer
let selfFreeVar =
mkILFreeVar (CompilerGeneratedName "self", true, nowCloSpec.ILType)
let baseName = CompilerGeneratedName "self"
mkILFreeVar (mkUniqueFreeVarName baseName nowFields, true, nowCloSpec.ILType)

let argToFreeVarMap =
(0, selfFreeVar)
Expand Down
57 changes: 46 additions & 11 deletions src/Compiler/CodeGen/IlxGen.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6450,7 +6450,7 @@ and GenObjectExpr cenv cgbuf eenvouter objExpr (baseType, baseValOpt, basecall,
GenWitnessArgsFromWitnessInfos cenv cgbuf eenvouter m cloinfo.cloWitnessInfos

for fv in cloinfo.cloFreeVars do
GenGetLocalVal cenv cgbuf eenvouter m fv None
GenGetFreeVarForClosure cenv cgbuf eenvouter m fv

CG.EmitInstr
cgbuf
Expand Down Expand Up @@ -6541,7 +6541,7 @@ and GenSequenceExpr
if stateVarsSet.Contains fv then
GenDefaultValue cenv cgbuf eenv (fv.Type, m)
else
GenGetLocalVal cenv cgbuf eenv m fv None
GenGetFreeVarForClosure cenv cgbuf eenv m fv

CG.EmitInstr
cgbuf
Expand Down Expand Up @@ -6653,7 +6653,7 @@ and GenSequenceExpr
if stateVarsSet.Contains fv then
GenDefaultValue cenv cgbuf eenvouter (fv.Type, m)
else
GenGetLocalVal cenv cgbuf eenvouter m fv None
GenGetFreeVarForClosure cenv cgbuf eenvouter m fv

CG.EmitInstr cgbuf (pop ilCloAllFreeVars.Length) (Push [ ilCloRetTyOuter ]) (I_newobj(ilxCloSpec.Constructor, None))
GenSequel cenv eenvouter.cloc cgbuf sequel
Expand Down Expand Up @@ -6886,7 +6886,9 @@ and GenClosureAlloc cenv (cgbuf: CodeGenBuffer) eenv (cloinfo, m) =
CG.EmitInstr cgbuf (pop 0) (Push [ EraseClosures.mkTyOfLambdas cenv.ilxPubCloEnv cloinfo.ilCloLambdas ]) (mkNormalLdsfld fspec)
else
GenWitnessArgsFromWitnessInfos cenv cgbuf eenv m cloinfo.cloWitnessInfos
GenGetLocalVals cenv cgbuf eenv m cloinfo.cloFreeVars

for fv in cloinfo.cloFreeVars do
GenGetFreeVarForClosure cenv cgbuf eenv m fv

CG.EmitInstr
cgbuf
Expand All @@ -6901,6 +6903,12 @@ and GenLambda cenv cgbuf eenv isLocalTypeFunc thisVars expr sequel =

and GenTypeOfVal cenv eenv (v: Val) = GenType cenv v.Range eenv.tyenv v.Type

and capturedTypeForFreeVar (g: TcGlobals) (fv: Val) =
if isByrefTy g fv.Type then
destByrefTy g fv.Type
else
fv.Type

and GenFreevar cenv m eenvouter tyenvinner (fv: Val) =
let g = cenv.g

Expand All @@ -6915,7 +6923,7 @@ and GenFreevar cenv m eenvouter tyenvinner (fv: Val) =
| Method _
| Null -> error (InternalError("GenFreevar: compiler error: unexpected unrealized value", fv.Range))
#endif
| _ -> GenType cenv m tyenvinner fv.Type
| _ -> GenType cenv m tyenvinner (capturedTypeForFreeVar g fv)

and GetIlxClosureFreeVars cenv m (thisVars: ValRef list) boxity eenv takenNames expr =
let g = cenv.g
Expand Down Expand Up @@ -7276,7 +7284,9 @@ and GenDelegateExpr cenv cgbuf eenvouter expr (TObjExprMethod(slotsig, _attribs,
IlxClosureSpec.Create(IlxClosureRef(ilDelegeeTypeRef, ilCloLambdas, ilCloAllFreeVars), ctxtGenericArgsForDelegee, false)

GenWitnessArgsFromWitnessInfos cenv cgbuf eenvouter m cloWitnessInfos
GenGetLocalVals cenv cgbuf eenvouter m cloFreeVars

for fv in cloFreeVars do
GenGetFreeVarForClosure cenv cgbuf eenvouter m fv

CG.EmitInstr
cgbuf
Expand Down Expand Up @@ -8206,6 +8216,17 @@ and GenLetRecFixup cenv cgbuf eenv (ilxCloSpec: IlxClosureSpec, e, ilField: ILFi
GenExpr cenv cgbuf eenv e2 Continue
CG.EmitInstr cgbuf (pop 2) Push0 (mkNormalStfld (mkILFieldSpec (ilField.FieldRef, ilxCloSpec.ILType)))

and isLambdaBinding (TBind(_, expr, _)) =
match stripDebugPoints expr with
| Expr.Lambda _
| Expr.TyLambda _
| Expr.Obj _ -> true
| _ -> false

and reorderBindingsLambdasFirst binds =
let lambdas, nonLambdas = binds |> List.partition isLambdaBinding
lambdas @ nonLambdas

/// Generate letrec bindings
and GenLetRecBindings cenv (cgbuf: CodeGenBuffer) eenv (allBinds: Bindings, m) (dict: Dictionary<Stamp, ILTypeRef> option) =

Expand Down Expand Up @@ -8299,8 +8320,11 @@ and GenLetRecBindings cenv (cgbuf: CodeGenBuffer) eenv (allBinds: Bindings, m) (
let recursiveVars =
Zset.addList (bindsPossiblyRequiringFixup |> List.map (fun v -> v.Var)) (Zset.empty valOrder)

let reorderedBindsPossiblyRequiringFixup =
reorderBindingsLambdasFirst bindsPossiblyRequiringFixup

let _ =
(recursiveVars, bindsPossiblyRequiringFixup)
(recursiveVars, reorderedBindsPossiblyRequiringFixup)
||> List.fold (fun forwardReferenceSet (bind: Binding) ->
// Compute fixups
bind.Expr
Expand Down Expand Up @@ -8344,6 +8368,8 @@ and GenLetRecBindings cenv (cgbuf: CodeGenBuffer) eenv (allBinds: Bindings, m) (
let _ =
(recursiveVars, groupBinds)
||> List.fold (fun forwardReferenceSet (binds: Binding list) ->
let binds = reorderBindingsLambdasFirst binds

match dict, cenv.g.realsig, binds with
| _, false, _
| None, _, _
Expand Down Expand Up @@ -8380,8 +8406,11 @@ and GenLetRecBindings cenv (cgbuf: CodeGenBuffer) eenv (allBinds: Bindings, m) (

and GenLetRec cenv cgbuf eenv (binds, body, m) sequel =
let _, endMark as scopeMarks = StartLocalScope "letrec" cgbuf
let eenv = AllocStorageForBinds cenv cgbuf scopeMarks eenv binds
GenLetRecBindings cenv cgbuf eenv (binds, m) None

let reorderedBinds = reorderBindingsLambdasFirst binds

let eenv = AllocStorageForBinds cenv cgbuf scopeMarks eenv reorderedBinds
GenLetRecBindings cenv cgbuf eenv (reorderedBinds, m) None
GenExpr cenv cgbuf eenv body (EndLocalScope(sequel, endMark))

//-------------------------------------------------------------------------
Expand Down Expand Up @@ -9850,8 +9879,14 @@ and GenGetStorageAndSequel (cenv: cenv) cgbuf eenv m (ty, ilTy) storage storeSeq
CG.EmitInstrs cgbuf (pop 0) (Push [ ilTy ]) [ mkLdarg0; mkNormalLdfld ilField ]
CommitGetStorageSequel cenv cgbuf eenv m ty localCloInfo storeSequel

and GenGetLocalVals cenv cgbuf eenvouter m fvs =
List.iter (fun v -> GenGetLocalVal cenv cgbuf eenvouter m v None) fvs
/// Load free variables for closure capture, dereferencing byrefs.
and GenGetFreeVarForClosure cenv cgbuf eenv m (fv: Val) =
let g = cenv.g
GenGetLocalVal cenv cgbuf eenv m fv None

if isByrefTy g fv.Type then
let ilUnderlyingTy = GenType cenv m eenv.tyenv (capturedTypeForFreeVar g fv)
CG.EmitInstr cgbuf (pop 1) (Push [ ilUnderlyingTy ]) (mkNormalLdobj ilUnderlyingTy)

and GenGetLocalVal cenv cgbuf eenv m (vspec: Val) storeSequel =
GenGetStorageAndSequel cenv cgbuf eenv m (vspec.Type, GenTypeOfVal cenv eenv vspec) (StorageForVal m vspec eenv) storeSequel
Expand Down
Loading
Loading