Fix FieldAccessException when optimizer relocates protected base-field access (#19963)#19964
Fix FieldAccessException when optimizer relocates protected base-field access (#19963)#19964T-Gro wants to merge 3 commits into
Conversation
…d access (#19963) The optimizer inlines/splits a member that reads a protected (family) base-class field into a method outside the field's family (e.g. a trivial member inlined into module/startup code under --optimize+), copying the ldfld/stfld there and producing illegal IL that throws FieldAccessException at runtime. Protected *method*/base calls were already pinned via FreeVars.UsesMethodLocalConstructs (set for TOp.ILCall isProtected) and the optimizer's relocation guards. Field loads were invisible to that machinery. Add a FreeVars.ContainsILFieldAccess flag set for any IL field instruction (TOp.ILAsm ldfld/stfld/...) and OR it into the five optimizer relocation guards. It is deliberately NOT read by the FS0405 escape check, so it can never reject otherwise-valid code; field accessibility is unavailable where free vars are summarised, so the flag is conservative over all IL fields (F# fields use TOp.ValFieldGet and are unaffected). Measured zero EmittedIL baseline churn. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
T-Gro
left a comment
There was a problem hiding this comment.
AI code review (expert-reviewer). The committed design fixes the crash but has an over-broad relocation guard that will block inlining of public IL field accesses - a regression that manifests as FS1118 when bootstrapping FSharp.Core.
|
|
||
| let acc = | ||
| if usesFieldAccess then | ||
| accContainsILFieldAccess acc |
There was a problem hiding this comment.
[Optimization Correctness - Critical] accContainsILFieldAccess fires here for any IL field instruction - public or protected. Because accFreeVarsInOp has no cenv, conservative tagging is unavoidable at this site. But all 5 optimizer guards consume this raw flag unconditionally, so any inline function whose body reads a public IL field (System.String.Empty, IntPtr.Zero, any C#-defined public field) also has its CurriedLambdaValue discarded and raises FS1118. FSharp.Core inline functions that read String.Empty will fail during bootstrapping. The fix: add isProtectedILFieldSpec cenv m fspec at each guard site via cenv.amap, so only truly protected (family) fields block relocation.
| (not (isNil boundVars) && List.exists (Zset.memberOf fvs.FreeLocals) boundVars) || | ||
| (not (isNil boundTyVars) && List.exists (Zset.memberOf fvs.FreeTyvars.FreeTypars) boundTyVars) || | ||
| fvs.UsesMethodLocalConstructs) -> | ||
| fvs.UsesMethodLocalConstructs || fvs.ContainsILFieldAccess) -> |
There was a problem hiding this comment.
[Optimization Correctness - Critical] This AbstractExprInfoByVars guard fires for any lambda with ContainsILFieldAccess = true, independent of whether that lambda captures the boundVars being scoped out. The existing free-locals check already handles the escape case. Adding || fvs.ContainsILFieldAccess creates a second orthogonal trimming path: a local inline function reading String.Empty inside any let binding gets trimmed to UnknownValue here even though it captures nothing going out of scope. For UsesMethodLocalConstructs the unconditional trim is justified (a base.Foo() body cannot be inlined outside the class). For ContainsILFieldAccess it is not - only protected fields warrant this. Requires adding cenv to AbstractExprInfoByVars (3 call sites) to call usesMethodLocalConstructsOrProtectedField cenv fvs expr.
| else | ||
| let fvs = freeInExpr CollectLocals body | ||
| if fvs.UsesMethodLocalConstructs then | ||
| if fvs.UsesMethodLocalConstructs || fvs.ContainsILFieldAccess then |
There was a problem hiding this comment.
[Optimization Correctness - Critical] OptimizeBinding.cut is the most direct FS1118 trigger: any CurriedLambdaValue with ContainsILFieldAccess = true (including inline functions reading String.Empty) is replaced with UnknownValue here. When TryOptimizeVal later encounters UnknownValue for an inline binding it cannot expand it and FS1118 is raised. The precise check usesMethodLocalConstructsOrProtectedField cenv fvs body (cheap flag as gate, exprReferencesProtectedILField cenv expr walk only when gate fires, isProtectedILFieldSpec cenv m fspec for the amap lookup) correctly limits the block to protected fields. cenv is already in scope at this site.
|
|
||
| // Companion to the above exercising a protected *static* field (the I_ldsfld branch of the | ||
| // optimizer relocation guard) for issue https://github.com/dotnet/fsharp/issues/19963. | ||
| [<Fact>] |
There was a problem hiding this comment.
[Test Coverage - High] The committed tests cover the original crash (protected field -> FieldAccessException) but there is no test for the collateral regression (public IL field -> FS1118). Without it the over-broad guard cannot be caught by the component test suite and would only surface during FSharp.Core bootstrapping. Add a companion test: compile let inline emptyOr (s: string) = if s.Length = 0 then System.String.Empty else s with --optimize+ --warnaserror+:1118 and assert success. The local working-tree draft already contains exactly this test.
| | CurriedLambdaValue (_, _, _, expr, _) when shouldInline || inlineIfLambda -> | ||
| let fvs = freeInExpr CollectLocals expr | ||
| if fvs.UsesMethodLocalConstructs then | ||
| if fvs.UsesMethodLocalConstructs || fvs.ContainsILFieldAccess then |
There was a problem hiding this comment.
[Design - High] The 5-guard + amap-resolution design in the local draft is the necessary and correct fix direction. Alternatives evaluated: (a) stamp protectedness on TOp.ILAsm at typecheck time - requires changing the typed tree discriminated union and TypedTreePickle.fs; too invasive. (b) drop the ContainsILFieldAccess gate and call exprReferencesProtectedILField directly - would run FoldExpr over every lambda body at all 5 sites even when there are no IL field accesses; expensive. (c) unify private-member and protected-field checks - they guard different things (FreeLocals accessibility vs. IL body protectedness); not practical. The 2-stage approach (O(1) flag gate, targeted FoldExpr walk, ILMemberAccess.Family lookup via amap) is the right balance.
…ly) fields only The #19963 fix flagged every IL field access as non-relocatable, so the optimizer refused to inline `inline` values reading public IL fields - e.g. FSharp.Core's `inline GetStringSlice` (which reads String.Empty) - failing the FSharp.Core bootstrap with FS1118 across all build legs. Refine the five optimizer relocation guards to pin only protected/family IL field access: keep the cheap FreeVars.ContainsILFieldAccess gate, then resolve accessibility via ImportILTypeRef/ILFieldDef.Access in the optimizer (where the import map is available) through a single usesMethodLocalConstructsOrProtectedField helper. Public IL field access is freely optimized again, matching the existing protected-method treatment. Adds an (|ILFieldInstr|_|) active pattern and a positive regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #19963.
Problem
Reading a
protected(family) base-class field from a derived F# member compiles fine, but the optimizer can relocate that field load — by inlining or method/lambda splitting — into a method outside the field's family. The relocatedldfld/stfldis then illegal IL and throwsSystem.FieldAccessExceptionat runtime. This happens under--optimize+, the default for Release builds, regardless of--realsig.--optimize-runs fine;--optimize+inlinesRunintomain(which is not inBase's family) and throws:Cause
Protected method/base calls are already pinned in-family: they set
FreeVars.UsesMethodLocalConstructs(viaTOp.ILCall isProtected), which the optimizer's relocation guards honour. Protected field loads were invisible to that machinery, so the optimizer freely relocated them. Contrast under--optimize+:Fix
Add a
FreeVars.ContainsILFieldAccessflag, set for any IL field instruction (TOp.ILAsmldfld/ldflda/stfld/ldsfld/ldsflda/stsfld), and OR it into the five optimizer relocation guards so such code is not inlined or split out of its declaring family.The flag is deliberately not read by the FS0405 escape check, so it can never reject otherwise-valid code (e.g. capturing a public field like
IntPtr.Zeroin a closure). Field accessibility is not available where free variables are summarised, so the flag is conservative over all IL field access; F#-defined fields useTOp.ValFieldGetand are unaffected.Validation
--optimize+(red before, green after).EmittedILsuite: 1307 tests, zero baseline churn.This is also a prerequisite for enabling protected-member access from closures (#5302): the same relocation otherwise reintroduces the runtime failure there.