Skip to content

Commit a9d3ead

Browse files
authored
Bugfix :: Object expressions in struct types generating invalid IL with byref fields (#19070)
1 parent f9de6c7 commit a9d3ead

File tree

5 files changed

+298
-2
lines changed

5 files changed

+298
-2
lines changed

docs/release-notes/.FSharp.Compiler.Service/11.0.0.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
* Type relations cache: handle unsolved type variables ([Issue #19037](https://github.com/dotnet/fsharp/issues/19037)) ([PR #19040](https://github.com/dotnet/fsharp/pull/19040))
1515
* Fix insertion context for modules with multiline attributes. ([Issue #18671](https://github.com/dotnet/fsharp/issues/18671))
1616
* Fix `--typecheck-only` for scripts stopping after processing `#load`-ed script ([PR #19048](https://github.com/dotnet/fsharp/pull/19048))
17+
* Fix object expressions in struct types generating invalid IL with byref fields causing TypeLoadException at runtime. ([Issue #19068](https://github.com/dotnet/fsharp/issues/19068), [PR #19070](https://github.com/dotnet/fsharp/pull/19070))
1718

1819
### Added
1920

src/Compiler/Checking/Expressions/CheckExpressions.fs

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7236,6 +7236,13 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI
72367236
// Add the object type to the ungeneralizable items
72377237
let env = {env with eUngeneralizableItems = addFreeItemOfTy objTy env.eUngeneralizableItems }
72387238

7239+
// Save the enclosing struct context BEFORE EnterFamilyRegion overwrites env.eFamilyType.
7240+
// This is used later to detect struct instance captures that would generate illegal byref fields.
7241+
let enclosingStructTyconRefOpt =
7242+
match env.eFamilyType with
7243+
| Some tcref when tcref.IsStructOrEnumTycon -> Some tcref
7244+
| _ -> None
7245+
72397246
// Object expression members can access protected members of the implemented type
72407247
let env = EnterFamilyRegion tcref env
72417248
let ad = env.AccessRights
@@ -7344,8 +7351,20 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI
73447351
errorR (Error(FSComp.SR.tcInvalidObjectExpressionSyntaxForm (), mWholeExpr))
73457352

73467353
// 4. Build the implementation
7347-
let expr = mkObjExpr(objtyR, baseValOpt, ctorCall, overrides', extraImpls, mWholeExpr)
7348-
let expr = mkCoerceIfNeeded g realObjTy objtyR expr
7354+
// Check for struct instance captures that would generate illegal byref fields.
7355+
// See AnalyzeObjExprStructCaptures and TransformObjExprForStructByrefCaptures for details.
7356+
let shouldTransform, structCaptures, _ =
7357+
AnalyzeObjExprStructCaptures enclosingStructTyconRefOpt ctorCall overrides' extraImpls
7358+
7359+
let expr =
7360+
if not shouldTransform then
7361+
// No transformation needed - build the object expression directly
7362+
let expr = mkObjExpr(objtyR, baseValOpt, ctorCall, overrides', extraImpls, mWholeExpr)
7363+
mkCoerceIfNeeded g realObjTy objtyR expr
7364+
else
7365+
// Transform to avoid byref captures
7366+
TransformObjExprForStructByrefCaptures g mWholeExpr structCaptures objtyR baseValOpt ctorCall overrides' extraImpls realObjTy
7367+
73497368
expr, tpenv
73507369

73517370
//-------------------------------------------------------------------------

src/Compiler/Checking/Expressions/CheckExpressionsOps.fs

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
module internal FSharp.Compiler.CheckExpressionsOps
44

5+
open Internal.Utilities.Collections
56
open Internal.Utilities.Library
67
open Internal.Utilities.Library.Extras
78
open FSharp.Compiler.CheckBasics
@@ -389,3 +390,128 @@ let inline mkOptionalParamTyBasedOnAttribute (g: TcGlobals.TcGlobals) tyarg attr
389390
mkValueOptionTy g tyarg
390391
else
391392
mkOptionTy g tyarg
393+
394+
//-------------------------------------------------------------------------
395+
// Struct byref capture fix for object expressions
396+
//-------------------------------------------------------------------------
397+
398+
/// When a struct instance method creates an object expression that captures constructor
399+
/// parameters, those captures go through 'this' which is a byref. This would create
400+
/// illegal byref fields in the closure class. This function detects such captures and
401+
/// extracts them to local bindings before the object expression.
402+
///
403+
/// Returns: (shouldTransform, structCaptures, methodParamStamps)
404+
let AnalyzeObjExprStructCaptures
405+
(enclosingStructTyconRefOpt: TyconRef option)
406+
(ctorCall: Expr)
407+
(overrides: ObjExprMethod list)
408+
(extraImpls: (TType * ObjExprMethod list) list)
409+
: bool * Val list * Set<Stamp> =
410+
411+
// Collect free variables from an expression
412+
let collectFreeVars expr =
413+
(freeInExpr CollectLocals expr).FreeLocals |> Zset.elements
414+
415+
// Collect all method parameters (bound variables) from object expression methods
416+
// These should NOT be treated as struct instance captures
417+
let methodParams =
418+
[
419+
for TObjExprMethod(_, _, _, paramGroups, _, _) in overrides do
420+
for paramGroup in paramGroups do
421+
for v in paramGroup do
422+
yield v
423+
for (_, methods) in extraImpls do
424+
for TObjExprMethod(_, _, _, paramGroups, _, _) in methods do
425+
for paramGroup in paramGroups do
426+
for v in paramGroup do
427+
yield v
428+
]
429+
|> List.map (fun v -> v.Stamp)
430+
|> Set.ofList
431+
432+
let allFreeVars =
433+
[
434+
yield! collectFreeVars ctorCall
435+
for TObjExprMethod(_, _, _, _, body, _) in overrides do
436+
yield! collectFreeVars body
437+
for (_, methods) in extraImpls do
438+
for TObjExprMethod(_, _, _, _, body, _) in methods do
439+
yield! collectFreeVars body
440+
]
441+
|> List.distinctBy (fun v -> v.Stamp)
442+
443+
// Filter to struct instance captures:
444+
// - We're in a struct context (enclosingStructTyconRefOpt is Some)
445+
// - The value is NOT a method parameter of the object expression
446+
// - The value is NOT a module binding
447+
// - The value is NOT a member or module binding (excludes property getters, etc.)
448+
// - The value is NOT a constructor
449+
let structCaptures =
450+
match enclosingStructTyconRefOpt with
451+
| None -> []
452+
| Some _ ->
453+
allFreeVars
454+
|> List.filter (fun v ->
455+
not v.IsModuleBinding
456+
&& not v.IsMemberOrModuleBinding
457+
&& not (Set.contains v.Stamp methodParams)
458+
&& v.LogicalName <> ".ctor")
459+
460+
let shouldTransform = not (List.isEmpty structCaptures)
461+
(shouldTransform, structCaptures, methodParams)
462+
463+
/// Transform an object expression to avoid byref captures from struct instance state.
464+
/// Creates local bindings for captured values and remaps references in the object expression.
465+
let TransformObjExprForStructByrefCaptures
466+
(g: TcGlobals.TcGlobals)
467+
(mWholeExpr: Text.range)
468+
(structCaptures: Val list)
469+
(objtyR: TType)
470+
(baseValOpt: Val option)
471+
(ctorCall: Expr)
472+
(overrides: ObjExprMethod list)
473+
(extraImpls: (TType * ObjExprMethod list) list)
474+
(realObjTy: TType)
475+
: Expr =
476+
477+
// Create local bindings for each captured value to avoid byref captures
478+
let localBindings =
479+
structCaptures
480+
|> List.map (fun v ->
481+
let local, _localExpr =
482+
mkCompGenLocal mWholeExpr (v.LogicalName + "$captured") v.Type
483+
484+
let readExpr = exprForVal mWholeExpr v
485+
(v, local, readExpr))
486+
487+
// Build remap: original val -> local val
488+
let remap =
489+
localBindings
490+
|> List.fold
491+
(fun (r: Remap) (orig, local, _) ->
492+
{ r with
493+
valRemap = r.valRemap.Add orig (mkLocalValRef local)
494+
})
495+
Remap.Empty
496+
497+
// Helper to remap an object expression method
498+
let remapMethod (TObjExprMethod(slotSig, attrs, mtps, paramGroups, body, range)) =
499+
TObjExprMethod(slotSig, attrs, mtps, paramGroups, remapExpr g CloneAll remap body, range)
500+
501+
// Remap all parts of the object expression
502+
let ctorCall' = remapExpr g CloneAll remap ctorCall
503+
let overrides' = overrides |> List.map remapMethod
504+
505+
let extraImpls' =
506+
extraImpls |> List.map (fun (ty, ms) -> (ty, ms |> List.map remapMethod))
507+
508+
// Build the object expression with remapped references
509+
let objExpr =
510+
mkObjExpr (objtyR, baseValOpt, ctorCall', overrides', extraImpls', mWholeExpr)
511+
512+
let objExpr = mkCoerceIfNeeded g realObjTy objtyR objExpr
513+
514+
// Wrap with let bindings: let x$captured = x in ...
515+
localBindings
516+
|> List.foldBack (fun (_, local, valueExpr) body -> mkLet DebugPointAtBinding.NoneAtInvisible mWholeExpr local valueExpr body)
517+
<| objExpr
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
namespace FSharp.Compiler.ComponentTests.Conformance.Expressions
2+
3+
open Xunit
4+
open FSharp.Test.Compiler
5+
6+
module StructObjectExpression =
7+
8+
[<Fact>]
9+
let ``Object expression in struct should not generate byref field - simple case`` () =
10+
Fsx """
11+
type Class(test : obj) = class end
12+
13+
[<Struct; NoComparison>]
14+
type Struct(test : obj) =
15+
member _.Test() = {
16+
new Class(test) with
17+
member _.ToString() = ""
18+
}
19+
20+
let s = Struct(42)
21+
let obj = s.Test()
22+
"""
23+
|> withOptions [ "--nowarn:52" ] // Suppress struct copy warning
24+
|> compileExeAndRun
25+
|> shouldSucceed
26+
27+
[<Fact>]
28+
let ``Object expression in struct with multiple fields`` () =
29+
Fsx """
30+
type Base(x: int, y: string) = class end
31+
32+
[<Struct; NoComparison>]
33+
type MyStruct(x: int, y: string) =
34+
member _.CreateObj() = {
35+
new Base(x, y) with
36+
member _.ToString() = y + string x
37+
}
38+
39+
let s = MyStruct(42, "test")
40+
let obj = s.CreateObj()
41+
"""
42+
|> withOptions [ "--nowarn:52" ]
43+
|> compileExeAndRun
44+
|> shouldSucceed
45+
46+
[<Fact>]
47+
let ``Object expression in struct referencing field in override method`` () =
48+
Fsx """
49+
type IFoo =
50+
abstract member DoSomething : unit -> int
51+
52+
[<Struct; NoComparison>]
53+
type MyStruct(value: int) =
54+
member _.CreateFoo() = {
55+
new IFoo with
56+
member _.DoSomething() = value * 2
57+
}
58+
59+
let s = MyStruct(21)
60+
let foo = s.CreateFoo()
61+
let result = foo.DoSomething()
62+
"""
63+
|> withOptions [ "--nowarn:52" ]
64+
|> compileExeAndRun
65+
|> shouldSucceed
66+
67+
// Regression tests - these must continue to work
68+
69+
[<Fact>]
70+
let ``Static member in struct with object expression should compile - StructBox regression`` () =
71+
// This is the StructBox.Comparer pattern from FSharp.Core/seqcore.fs
72+
// Static members don't have 'this' so should NOT be transformed
73+
Fsx """
74+
open System.Collections.Generic
75+
76+
[<Struct; NoComparison; NoEquality>]
77+
type StructBox<'T when 'T: equality>(value: 'T) =
78+
member x.Value = value
79+
80+
static member Comparer =
81+
let gcomparer = HashIdentity.Structural<'T>
82+
{ new IEqualityComparer<StructBox<'T>> with
83+
member _.GetHashCode(v) = gcomparer.GetHashCode(v.Value)
84+
member _.Equals(v1, v2) = gcomparer.Equals(v1.Value, v2.Value) }
85+
86+
let comparer = StructBox<int>.Comparer
87+
let box1 = StructBox(42)
88+
let box2 = StructBox(42)
89+
let result = comparer.Equals(box1, box2)
90+
if not result then failwith "Expected equal"
91+
"""
92+
|> compileExeAndRun
93+
|> shouldSucceed
94+
95+
[<Fact>]
96+
let ``Module level object expression with struct parameter should compile`` () =
97+
// Module-level functions don't have instance context
98+
Fsx """
99+
[<Struct>]
100+
type MyStruct(value: int) =
101+
member x.Value = value
102+
103+
let createComparer () =
104+
{ new System.Object() with
105+
member _.ToString() = "comparer" }
106+
107+
let c = createComparer()
108+
if c.ToString() <> "comparer" then failwith "Failed"
109+
"""
110+
|> compileExeAndRun
111+
|> shouldSucceed
112+
113+
[<Fact>]
114+
let ``Object expression in struct not capturing anything should compile`` () =
115+
// Object expression that doesn't reference any struct state
116+
Fsx """
117+
[<Struct; NoComparison>]
118+
type MyStruct(value: int) =
119+
member _.CreateObj() = {
120+
new System.Object() with
121+
member _.ToString() = "constant"
122+
}
123+
124+
let s = MyStruct(42)
125+
let obj = s.CreateObj()
126+
if obj.ToString() <> "constant" then failwith "Failed"
127+
"""
128+
|> withOptions [ "--nowarn:52" ]
129+
|> compileExeAndRun
130+
|> shouldSucceed
131+
132+
[<Fact>]
133+
let ``Object expression in struct with method parameters should not confuse params with captures`` () =
134+
// Method parameters are not instance captures, should not trigger transformation
135+
Fsx """
136+
[<Struct; NoComparison>]
137+
type MyStruct(value: int) =
138+
member _.Transform(multiplier: int) = {
139+
new System.Object() with
140+
member _.ToString() = string (value * multiplier)
141+
}
142+
143+
let s = MyStruct(21)
144+
let obj = s.Transform(2)
145+
if obj.ToString() <> "42" then failwith "Expected 42"
146+
"""
147+
|> withOptions [ "--nowarn:52" ]
148+
|> compileExeAndRun
149+
|> shouldSucceed

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@
8787
<Compile Include="Conformance\Expressions\BindingExpressions\BindingExpressions.fs" />
8888
<Compile Include="Conformance\Expressions\ComputationExpressions\ComputationExpressions.fs" />
8989
<Compile Include="Conformance\Expressions\ObjectExpressions\ObjectExpressions.fs" />
90+
<Compile Include="Conformance\Expressions\ObjectExpressions\StructObjectExpression.fs" />
9091
<Compile Include="Conformance\Expressions\ControlFlowExpressions\PatternMatching\PatternMatching.fs" />
9192
<Compile Include="Conformance\Expressions\ControlFlowExpressions\SequenceIteration\SequenceIteration.fs" />
9293
<Compile Include="Conformance\Expressions\ControlFlowExpressions\Type-relatedExpressions\Type-relatedExpressions.fs" />

0 commit comments

Comments
 (0)