-
Notifications
You must be signed in to change notification settings - Fork 844
[WIP] Investigation: Object expressions in struct types generating invalid IL with byref fields #19070
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
Draft
Copilot
wants to merge
32
commits into
main
Choose a base branch
from
copilot/fix-invalid-il-byref-fields
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+661
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ef fields Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix invalid IL generation with byref fields in structs
Fix object expressions in struct types generating invalid IL with byref fields
Nov 11, 2025
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
…ementations Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
…ar 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>
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>
The transformation is causing false positives - it's being applied to object expressions in non-struct contexts, leading to byref type errors in files like ServiceParamInfoLocations.fs. Root cause: The check for `v.IsInstanceMember && isStructTyconRef v.DeclaringEntity` is too broad - it matches any struct instance member that appears as a free variable, even when the object expression is not inside a struct instance member method. Need to add context awareness to only transform when we're truly inside a struct instance member context, not just when we happen to reference any struct member. Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Copilot
AI
changed the title
Fix object expressions in struct types generating invalid IL with byref fields
[WIP] Fix object expressions in struct types generating invalid IL with byref fields
Dec 31, 2025
Following reviewer's suggestion, the transformation now uses env.eFamilyType to detect when we're actually inside a struct instance member method, and checks that baseValOpt's type matches the enclosing struct. This fixes false positives by ensuring we only transform when: 1. Not a pure interface implementation (not isInterfaceTy) 2. Inside a struct instance member (env.eFamilyType is a struct tyconRef) 3. baseVal is being captured (baseValOpt is Some) 4. baseVal's type matches the enclosing struct (tyconRefEq check) This prevents transformation in module functions or class methods that happen to reference struct members, which was causing build failures. Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
This commit completely reverts the struct object expression transformation that was causing a regression in FSharp.Core/seqcore.fs line 200 where a static member contains an object expression. The transformation was incorrectly triggering for static members, causing false positive byref field errors. Rather than attempting to fix the detection logic, we're reverting the entire transformation to get back to a clean state. Changes reverted: - Removed TryExtractStructMembersFromObjectExpr function from CheckExpressionsOps.fs - Removed enclosingStructTyconRefOpt capture logic from CheckExpressions.fs - Removed all transformation calls and method/constructor remapping - Removed Internal.Utilities.Collections import (only used by transformation) The test file StructObjectExpression.fs is kept but will need to be fixed or removed in a future PR when we implement a proper solution that correctly distinguishes instance vs static member context. Files changed: - src/Compiler/Checking/Expressions/CheckExpressions.fs (-42 lines) - src/Compiler/Checking/Expressions/CheckExpressionsOps.fs (-89 lines) Total: -131 lines
After extensive investigation (see HYPOTHESIS.md Hypothesis 10), discovered that the approach using env.eFamilyType is fundamentally flawed - it cannot distinguish between static and instance member contexts. The seqcore.fs regression persists even with transformation disabled because the bootstrap compiler has the buggy code. A complete rebuild and different approach is required. Removed: - TryExtractStructMembersFromObjectExpr function (89 lines) - All transformation logic from CheckExpressions.fs (42 lines) - Total: 131 lines removed Kept StructObjectExpression.fs test file for future reference when implementing a correct solution that properly detects instance member contexts. This provides a clean slate to start fresh with a better architectural approach. Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Copilot
AI
changed the title
Fix object expressions in struct types generating invalid IL with byref fields
[WIP] Investigation: Object expressions in struct types generating invalid IL with byref fields
Jan 7, 2026
Ran all 3 StructObjectExpression tests - ALL FAILING AS EXPECTED: Test 1: TypeLoadException - byref field in closure (runtime error) Test 2: Error 406 - byref variable captured (compile-time) Test 3: Error 406 - byref variable captured (compile-time) This confirms: ✅ Tests correctly detect the original bug ✅ The issue is real and reproducible ✅ Test suite ready for validating future fix ❌ Current approach fundamentally flawed Investigation complete. A correct implementation requires a different architectural approach that can properly distinguish between static and instance member contexts - env.eFamilyType alone is insufficient. Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Investigation: Object expressions in struct types generate invalid IL with byref fields
Problem
When object expressions inside struct member methods capture struct constructor parameters or fields, the compiler generates illegal
byref<Struct>fields in closure classes, causingTypeLoadExceptionat runtime.Current Status
INVESTIGATION COMPLETE - TRANSFORMATION CODE REVERTED
After extensive investigation testing 11 hypotheses (documented in HYPOTHESIS.md), confirmed:
env.eFamilyTypecannot distinguish static vs instance contextsTest Results (Baseline - No Fix Applied)
All 3 tests FAILING as expected - This confirms the bug exists and tests detect it:
Test 1: Simple case
TypeLoadException: A ByRef or ByRef-like type cannot be used as the type for an instance fieldTest 2: Multiple fields
Error 406: The byref-typed variable '_' is used in an invalid wayCreateObj()method)Test 3: Interface override
Error 406: The byref-typed variable '_' is used in an invalid wayCreateFoo()method)Investigation Summary (11 Hypotheses - See HYPOTHESIS.md)
Hypotheses 1-6: Various filter and context detection attempts
Hypothesis 7: Discovered
env.eFamilyTypeoverwritten byEnterFamilyRegionHypothesis 8: Simplified filter for constructor parameters
Hypothesis 9: Removed interface-only guard
Hypothesis 10: CRITICAL - env.eFamilyType cannot distinguish static vs instance contexts
Hypothesis 11: CONFIRMED - Tests properly fail without fix, validating test design
Why the Initial Approach Failed
env.eFamilyTyperepresents the TYPE being type-checked (the object expression's base type), NOT the execution context. Both static and instance members haveenv.eFamilyType = Some(structTcref)when inside a struct type definition, making it impossible to distinguish between:This caused false positives like the
StructBox.Comparerregression in seqcore.fs.Path Forward
A correct solution needs to:
thisvalue presence in scope, ORTcObjectExpr, ORbaseValOptmore intelligently to detect struct instance capturesInvestigation Artifacts
Conclusion
Investigation complete. The original bug is confirmed and well-tested. A different architectural approach is required for a correct fix.
Relates to #19068
Original prompt
Fix: Object expressions in struct types generate invalid IL with byref fields
Problem
When an object expression is created inside a struct member method and references values from the struct's constructor parameters or fields, the F# compiler generates invalid IL code. The generated closure class contains a
byref<Struct>field, which is illegal according to CLI rules. This results in aTypeLoadExceptionat runtime.Repro Code (Currently Crashes)
Current Broken Behavior
The compiler generates IL equivalent to:
This violates .NET CLI rules and crashes at runtime.
Root Cause
The compiler's object expression handling in
src/Compiler/Checking/Expressions/CheckExpressions.fstreats the enclosing struct instance (baseValOpt) as a captured variable. Instead of extracting the specific values needed from the struct, it tries to capture a reference to the entire struct, which becomes an illegal byref field in the generated closure class.Solution: Extract and Capture Values Instead of Struct Reference
At type-checking time (before closure conversion), detect when an object expression inside a struct would capture the struct instance, and instead:
this/baseValOptbaseValOptsince we no longer need the struct referenceThis preserves F# struct copy-by-value semantics and generates legal IL.
Expected Behavior After Fix
The compiler should generate IL equivalent to:
Implementation Location
Primary File:
src/Compiler/Checking/Expressions/CheckExpressions.fsTarget Function: The function that type-checks object expressions and constructs
Expr.Obj(likely namedTcObjectExpror similar, typically around lines 3000-4500)Implementation Steps
Step 1: Add Helper to Collect Captured Struct Members
Add a function that analyzes object expression methods to find which struct members are captured:
Step 2: Add Helper to Create Capture Bindings
Add a function that creates local bindings for captured struct members:
Step 3: Add Helper to Rewrite Method Bodies
Add a function that rewrites object expression methods to use captured locals: