From 6a4733e721e9c74595250d22a10271f14120ab80 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 12:16:29 +0000 Subject: [PATCH 1/6] Initial plan From b8e8f2804638fbad055e80f6955154426ad7dfa7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 13:15:14 +0000 Subject: [PATCH 2/6] Fix object expressions in struct types generating invalid IL with byref fields Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> --- .../Checking/Expressions/CheckExpressions.fs | 70 ++++++++++++++++++- .../StructObjectExpression.fs | 62 ++++++++++++++++ .../FSharp.Compiler.ComponentTests.fsproj | 1 + 3 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ObjectExpressions/StructObjectExpression.fs diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index 65e6de7f094..1f7e04f5b08 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressions.fs @@ -7308,6 +7308,59 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI DispatchSlotChecking.CheckDispatchSlotsAreImplemented (env.DisplayEnv, cenv.infoReader, m, env.NameEnv, cenv.tcSink, isOverallTyAbstract, true, implTy, dispatchSlots, availPriorOverrides, overrideSpecs) |> ignore // 3. create the specs of overrides + + // Fix for struct object expressions: extract captured struct members to avoid byref fields + // When an object expression is created inside a struct member method and references struct fields, + // the generated closure would contain a byref field which is illegal IL. + // Solution: Extract the struct member values into local variables before creating the object expression, + // so the closure captures the values instead of a byref to the struct. + + let capturedStructMembers, methodBodyRemap = + // Collect all free variables from method bodies in all overrides + let allMethodBodies = + overridesAndVirts + |> List.collect (fun (_, _, _, _, _, overrides) -> + overrides |> List.map (fun (_, (_, _, _, _, bindingBody)) -> bindingBody)) + + if allMethodBodies.IsEmpty then + [], Remap.Empty + else + let freeVars = + allMethodBodies + |> List.fold (fun acc body -> + let bodyFreeVars = freeInExpr CollectTyparsAndLocals body + unionFreeVars acc bodyFreeVars) emptyFreeVars + + // Filter to only variables that are members of a struct type + let structMembers = + freeVars.FreeLocals + |> Zset.elements + |> List.filter (fun v -> + v.HasDeclaringEntity && isStructTyconRef v.DeclaringEntity) + + if structMembers.IsEmpty then + [], Remap.Empty + else + // Create local variables for each captured struct member + let bindings = + structMembers + |> List.map (fun memberVal -> + // Create a new local to hold the member's value + let localVal, _ = mkCompGenLocal mWholeExpr memberVal.DisplayName memberVal.Type + // The value expression is just a reference to the member + let valueExpr = exprForVal mWholeExpr memberVal + (memberVal, localVal, valueExpr)) + + // Build a remap from original member vals to new local vals + let remap = + bindings + |> List.fold (fun remap (origVal, localVal, _) -> + { remap with valRemap = remap.valRemap.Add origVal (mkLocalValRef localVal) }) Remap.Empty + + // Return the bindings to be added before the object expression + let bindPairs = bindings |> List.map (fun (_, localVal, valueExpr) -> (localVal, valueExpr)) + bindPairs, remap + let allTypeImpls = overridesAndVirts |> List.map (fun (m, implTy, _, dispatchSlotsKeyed, _, overrides) -> let overrides' = @@ -7331,7 +7384,14 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI | Some x -> x | None -> error(Error(FSComp.SR.tcAtLeastOneOverrideIsInvalid(), mObjTy)) - yield TObjExprMethod(overridden.GetSlotSig(cenv.amap, m), bindingAttribs, mtps, [thisVal] :: methodVars, bindingBody, id.idRange) ] + // Remap method body to use local copies of struct members + let bindingBody' = + if methodBodyRemap.valRemap.IsEmpty then + bindingBody + else + remapExpr g CloneAll methodBodyRemap bindingBody + + yield TObjExprMethod(overridden.GetSlotSig(cenv.amap, m), bindingAttribs, mtps, [thisVal] :: methodVars, bindingBody', id.idRange) ] (implTy, overrides')) let objtyR, overrides' = allTypeImpls.Head @@ -7345,6 +7405,14 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI // 4. Build the implementation let expr = mkObjExpr(objtyR, baseValOpt, ctorCall, overrides', extraImpls, mWholeExpr) let expr = mkCoerceIfNeeded g realObjTy objtyR expr + + // Wrap with bindings for captured struct members + let expr = + if capturedStructMembers.IsEmpty then + expr + else + List.foldBack (fun (v, e) body -> mkInvisibleLet mWholeExpr v e body) capturedStructMembers expr + expr, tpenv //------------------------------------------------------------------------- diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ObjectExpressions/StructObjectExpression.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ObjectExpressions/StructObjectExpression.fs new file mode 100644 index 00000000000..60c5a4d15bf --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ObjectExpressions/StructObjectExpression.fs @@ -0,0 +1,62 @@ +namespace FSharp.Compiler.ComponentTests.Conformance.Expressions + +open Xunit +open FSharp.Test.Compiler + +module StructObjectExpression = + + [] + let ``Object expression in struct should not generate byref field - simple case`` () = + FSharp """ +type Class(test : obj) = class end + +[] +type Struct(test : obj) = + member _.Test() = { + new Class(test) with + member _.ToString() = "" + } + +let s = Struct(42) +let obj = s.Test() + """ + |> compile + |> shouldSucceed + + [] + let ``Object expression in struct with multiple fields`` () = + FSharp """ +type Base(x: int, y: string) = class end + +[] +type MyStruct(x: int, y: string) = + member _.CreateObj() = { + new Base(x, y) with + member _.ToString() = y + string x + } + +let s = MyStruct(42, "test") +let obj = s.CreateObj() + """ + |> compile + |> shouldSucceed + + [] + let ``Object expression in struct referencing field in override method`` () = + FSharp """ +type IFoo = + abstract member DoSomething : unit -> int + +[] +type MyStruct(value: int) = + member _.CreateFoo() = { + new IFoo with + member _.DoSomething() = value * 2 + } + +let s = MyStruct(21) +let foo = s.CreateFoo() +let result = foo.DoSomething() + """ + |> compile + |> shouldSucceed diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 8f5ace7aade..b5359280c1f 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -87,6 +87,7 @@ + From 0616707f44b7d6afe04096fc81b5f501efe3cab1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 13:32:41 +0000 Subject: [PATCH 3/6] Add IsInstanceMember check to only transform instance members of structs Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> --- src/Compiler/Checking/Expressions/CheckExpressions.fs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index 1f7e04f5b08..989575dabc6 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressions.fs @@ -7331,12 +7331,14 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI let bodyFreeVars = freeInExpr CollectTyparsAndLocals body unionFreeVars acc bodyFreeVars) emptyFreeVars - // Filter to only variables that are members of a struct type + // Filter to only instance members of struct types + // This identifies the problematic case: when an object expression inside a struct + // captures instance members, which would require capturing 'this' as a byref let structMembers = freeVars.FreeLocals |> Zset.elements |> List.filter (fun v -> - v.HasDeclaringEntity && isStructTyconRef v.DeclaringEntity) + v.IsInstanceMember && v.HasDeclaringEntity && isStructTyconRef v.DeclaringEntity) if structMembers.IsEmpty then [], Remap.Empty From de89a91d7db6af6923dddb340b1bb78fc53ac4cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 14:04:01 +0000 Subject: [PATCH 4/6] Add interface check to prevent transformation for pure interface implementations Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> --- .../Checking/Expressions/CheckExpressions.fs | 87 ++++++++++--------- 1 file changed, 48 insertions(+), 39 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index 989575dabc6..d16a84dfc46 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressions.fs @@ -7314,54 +7314,63 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI // the generated closure would contain a byref field which is illegal IL. // Solution: Extract the struct member values into local variables before creating the object expression, // so the closure captures the values instead of a byref to the struct. + // + // This transformation only applies when: + // - The object expression derives from a base class (not just implementing interfaces) + // - The object expression captures instance members from an enclosing struct let capturedStructMembers, methodBodyRemap = - // Collect all free variables from method bodies in all overrides - let allMethodBodies = - overridesAndVirts - |> List.collect (fun (_, _, _, _, _, overrides) -> - overrides |> List.map (fun (_, (_, _, _, _, bindingBody)) -> bindingBody)) - - if allMethodBodies.IsEmpty then + // Only apply transformation for object expressions with base classes, not pure interface implementations + // Interface implementations don't pass struct members to base constructors, so they don't have the byref issue + if isInterfaceTy then [], Remap.Empty else - let freeVars = - allMethodBodies - |> List.fold (fun acc body -> - let bodyFreeVars = freeInExpr CollectTyparsAndLocals body - unionFreeVars acc bodyFreeVars) emptyFreeVars - - // Filter to only instance members of struct types - // This identifies the problematic case: when an object expression inside a struct - // captures instance members, which would require capturing 'this' as a byref - let structMembers = - freeVars.FreeLocals - |> Zset.elements - |> List.filter (fun v -> - v.IsInstanceMember && v.HasDeclaringEntity && isStructTyconRef v.DeclaringEntity) + // Collect all free variables from method bodies in all overrides + let allMethodBodies = + overridesAndVirts + |> List.collect (fun (_, _, _, _, _, overrides) -> + overrides |> List.map (fun (_, (_, _, _, _, bindingBody)) -> bindingBody)) - if structMembers.IsEmpty then + if allMethodBodies.IsEmpty then [], Remap.Empty else - // Create local variables for each captured struct member - let bindings = - structMembers - |> List.map (fun memberVal -> - // Create a new local to hold the member's value - let localVal, _ = mkCompGenLocal mWholeExpr memberVal.DisplayName memberVal.Type - // The value expression is just a reference to the member - let valueExpr = exprForVal mWholeExpr memberVal - (memberVal, localVal, valueExpr)) + let freeVars = + allMethodBodies + |> List.fold (fun acc body -> + let bodyFreeVars = freeInExpr CollectTyparsAndLocals body + unionFreeVars acc bodyFreeVars) emptyFreeVars - // Build a remap from original member vals to new local vals - let remap = - bindings - |> List.fold (fun remap (origVal, localVal, _) -> - { remap with valRemap = remap.valRemap.Add origVal (mkLocalValRef localVal) }) Remap.Empty + // Filter to only instance members of struct types + // This identifies the problematic case: when an object expression inside a struct + // captures instance members, which would require capturing 'this' as a byref + let structMembers = + freeVars.FreeLocals + |> Zset.elements + |> List.filter (fun v -> + v.IsInstanceMember && v.HasDeclaringEntity && isStructTyconRef v.DeclaringEntity) - // Return the bindings to be added before the object expression - let bindPairs = bindings |> List.map (fun (_, localVal, valueExpr) -> (localVal, valueExpr)) - bindPairs, remap + if structMembers.IsEmpty then + [], Remap.Empty + else + // Create local variables for each captured struct member + let bindings = + structMembers + |> List.map (fun memberVal -> + // Create a new local to hold the member's value + let localVal, _ = mkCompGenLocal mWholeExpr memberVal.DisplayName memberVal.Type + // The value expression is just a reference to the member + let valueExpr = exprForVal mWholeExpr memberVal + (memberVal, localVal, valueExpr)) + + // Build a remap from original member vals to new local vals + let remap = + bindings + |> List.fold (fun remap (origVal, localVal, _) -> + { remap with valRemap = remap.valRemap.Add origVal (mkLocalValRef localVal) }) Remap.Empty + + // Return the bindings to be added before the object expression + let bindPairs = bindings |> List.map (fun (_, localVal, valueExpr) -> (localVal, valueExpr)) + bindPairs, remap let allTypeImpls = overridesAndVirts |> List.map (fun (m, implTy, _, dispatchSlotsKeyed, _, overrides) -> From 086f23f098c923478da878219e20c383c076033d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 15:30:13 +0000 Subject: [PATCH 5/6] Refactor struct object expression fix to CheckExpressionsOps with clear early guards Moved transformation logic from CheckExpressions.fs to CheckExpressionsOps.fs as requested. Added clear early guard conditions and comprehensive documentation. The transformation now has explicit early exits for: - Interface-only implementations (isInterfaceTy check) - Empty method bodies - No struct members captured This makes the code architecture clearer about when the new code executes. Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> --- .../Checking/Expressions/CheckExpressions.fs | 65 ++-------------- .../Expressions/CheckExpressionsOps.fs | 74 +++++++++++++++++++ 2 files changed, 79 insertions(+), 60 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index d16a84dfc46..7b4e2290168 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressions.fs @@ -7310,67 +7310,12 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI // 3. create the specs of overrides // Fix for struct object expressions: extract captured struct members to avoid byref fields - // When an object expression is created inside a struct member method and references struct fields, - // the generated closure would contain a byref field which is illegal IL. - // Solution: Extract the struct member values into local variables before creating the object expression, - // so the closure captures the values instead of a byref to the struct. - // - // This transformation only applies when: - // - The object expression derives from a base class (not just implementing interfaces) - // - The object expression captures instance members from an enclosing struct - + // This transformation is only applied when ALL of the following conditions are met: + // 1. The object expression derives from a base class (not just implementing an interface) + // 2. The object expression captures instance members from an enclosing struct + // See CheckExpressionsOps.TryExtractStructMembersFromObjectExpr for implementation details let capturedStructMembers, methodBodyRemap = - // Only apply transformation for object expressions with base classes, not pure interface implementations - // Interface implementations don't pass struct members to base constructors, so they don't have the byref issue - if isInterfaceTy then - [], Remap.Empty - else - // Collect all free variables from method bodies in all overrides - let allMethodBodies = - overridesAndVirts - |> List.collect (fun (_, _, _, _, _, overrides) -> - overrides |> List.map (fun (_, (_, _, _, _, bindingBody)) -> bindingBody)) - - if allMethodBodies.IsEmpty then - [], Remap.Empty - else - let freeVars = - allMethodBodies - |> List.fold (fun acc body -> - let bodyFreeVars = freeInExpr CollectTyparsAndLocals body - unionFreeVars acc bodyFreeVars) emptyFreeVars - - // Filter to only instance members of struct types - // This identifies the problematic case: when an object expression inside a struct - // captures instance members, which would require capturing 'this' as a byref - let structMembers = - freeVars.FreeLocals - |> Zset.elements - |> List.filter (fun v -> - v.IsInstanceMember && v.HasDeclaringEntity && isStructTyconRef v.DeclaringEntity) - - if structMembers.IsEmpty then - [], Remap.Empty - else - // Create local variables for each captured struct member - let bindings = - structMembers - |> List.map (fun memberVal -> - // Create a new local to hold the member's value - let localVal, _ = mkCompGenLocal mWholeExpr memberVal.DisplayName memberVal.Type - // The value expression is just a reference to the member - let valueExpr = exprForVal mWholeExpr memberVal - (memberVal, localVal, valueExpr)) - - // Build a remap from original member vals to new local vals - let remap = - bindings - |> List.fold (fun remap (origVal, localVal, _) -> - { remap with valRemap = remap.valRemap.Add origVal (mkLocalValRef localVal) }) Remap.Empty - - // Return the bindings to be added before the object expression - let bindPairs = bindings |> List.map (fun (_, localVal, valueExpr) -> (localVal, valueExpr)) - bindPairs, remap + CheckExpressionsOps.TryExtractStructMembersFromObjectExpr isInterfaceTy overridesAndVirts mWholeExpr let allTypeImpls = overridesAndVirts |> List.map (fun (m, implTy, _, dispatchSlotsKeyed, _, overrides) -> diff --git a/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs b/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs index 6c12485c4ed..f16d69e29b8 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs @@ -4,6 +4,7 @@ module internal FSharp.Compiler.CheckExpressionsOps open Internal.Utilities.Library open Internal.Utilities.Library.Extras +open Internal.Utilities.Collections open FSharp.Compiler.CheckBasics open FSharp.Compiler.ConstraintSolver open FSharp.Compiler.DiagnosticsLogger @@ -389,3 +390,76 @@ let inline mkOptionalParamTyBasedOnAttribute (g: TcGlobals.TcGlobals) tyarg attr mkValueOptionTy g tyarg else mkOptionTy g tyarg + +/// Extract captured struct instance members from object expressions to avoid illegal byref fields in closures. +/// When an object expression inside a struct instance member method captures struct fields, the generated +/// closure would contain a byref field which violates CLI rules. This function extracts those struct +/// member values into local variables and rewrites the object expression methods to use the locals instead. +/// +/// Returns: (capturedMemberBindings, methodBodyRemap) where: +/// - capturedMemberBindings: list of (localVar, valueExpr) pairs to prepend before the object expression +/// - methodBodyRemap: Remap to apply to object expression method bodies to use the captured locals +let TryExtractStructMembersFromObjectExpr + (isInterfaceTy: bool) + (overridesAndVirts: ('a * 'b * 'c * 'd * 'e * ('f * (range * Val * Val list list * Attribs * Expr)) list) list) + (mWholeExpr: range) : (Val * Expr) list * Remap = + + // Early guard: Only apply for object expressions deriving from base classes, not pure interface implementations + // Interface implementations don't pass struct members to base constructors, so they don't have the byref issue + if isInterfaceTy then + [], Remap.Empty + else + // Collect all method bodies from the object expression overrides + let allMethodBodies = + overridesAndVirts + |> List.collect (fun (_, _, _, _, _, overrides) -> + overrides |> List.map (fun (_, (_, _, _, _, bindingBody)) -> bindingBody)) + + // Early exit if no methods to analyze + if allMethodBodies.IsEmpty then + [], Remap.Empty + else + // Find all free variables in the method bodies + let freeVars = + allMethodBodies + |> List.fold (fun acc body -> + let bodyFreeVars = freeInExpr CollectTyparsAndLocals body + unionFreeVars acc bodyFreeVars) emptyFreeVars + + // Filter to only instance members of struct types + // This identifies the problematic case: when an object expression inside a struct + // captures instance members, which would require capturing 'this' as a byref + let structMembers = + freeVars.FreeLocals + |> Zset.elements + |> List.filter (fun (v: Val) -> + // Must be an instance member (not static) + v.IsInstanceMember && + // Must have a declaring entity + v.HasDeclaringEntity && + // The declaring entity must be a struct type + isStructTyconRef v.DeclaringEntity) + + // Early exit if no struct members captured + if structMembers.IsEmpty then + [], Remap.Empty + else + // Create local variables for each captured struct member + let bindings = + structMembers + |> List.map (fun (memberVal: Val) -> + // Create a new local to hold the member's value + let localVal, _ = mkCompGenLocal mWholeExpr memberVal.DisplayName memberVal.Type + // The value expression is just a reference to the member + let valueExpr = exprForVal mWholeExpr memberVal + (memberVal, localVal, valueExpr)) + + // Build a remap from original member vals to new local vals + let remap = + bindings + |> List.fold (fun (remap: Remap) (origVal, localVal, _) -> + { remap with valRemap = remap.valRemap.Add origVal (mkLocalValRef localVal) }) Remap.Empty + + // Return the bindings to be added before the object expression + let bindPairs = bindings |> List.map (fun (_, localVal, valueExpr) -> (localVal, valueExpr)) + bindPairs, remap From de69a3ce6c733097f47dd75e1a3c0048826ab531 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Nov 2025 15:55:07 +0000 Subject: [PATCH 6/6] Fix type inference issue by removing explicit generic type annotation Removed the overly complex generic type annotation for overridesAndVirts parameter that was causing type mismatch errors. F# can now properly infer the type from usage. Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> --- src/Compiler/Checking/Expressions/CheckExpressionsOps.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs b/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs index f16d69e29b8..0f149996df6 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressionsOps.fs @@ -401,7 +401,7 @@ let inline mkOptionalParamTyBasedOnAttribute (g: TcGlobals.TcGlobals) tyarg attr /// - methodBodyRemap: Remap to apply to object expression method bodies to use the captured locals let TryExtractStructMembersFromObjectExpr (isInterfaceTy: bool) - (overridesAndVirts: ('a * 'b * 'c * 'd * 'e * ('f * (range * Val * Val list list * Attribs * Expr)) list) list) + overridesAndVirts (mWholeExpr: range) : (Val * Expr) list * Remap = // Early guard: Only apply for object expressions deriving from base classes, not pure interface implementations