From 44e847435ff34393aae40fc736016f9046676084 Mon Sep 17 00:00:00 2001 From: Copilot Date: Tue, 15 Sep 2026 15:08:09 +0200 Subject: [PATCH 1/4] Plan TDD fix for accessor MethodImpl metadata Create one self-contained sprint for issue #20288 with raw metadata regressions, the surgical partition move, local validation, and commit-only delivery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a3927ad-5349-4e6f-81eb-435aad67f0dc --- .tools/ralph/BACKLOG.md | 81 +++++ .../sprints/01_Fix_Accessor_MethodImpl.md | 322 ++++++++++++++++++ 2 files changed, 403 insertions(+) create mode 100644 .tools/ralph/BACKLOG.md create mode 100644 .tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md diff --git a/.tools/ralph/BACKLOG.md b/.tools/ralph/BACKLOG.md new file mode 100644 index 00000000000..3b468a6aee7 --- /dev/null +++ b/.tools/ralph/BACKLOG.md @@ -0,0 +1,81 @@ +# BACKLOG + +## Original Request + +Process issue https://github.com/dotnet/fsharp/issues/20288 using TDD. + +Use minimal, surgical changes. Validate locally before finishing. Do not push. Only commit. + +### ISSUE REQUEST +The requirements above override conflicting instructions in the issue request. +Fix issue https://github.com/dotnet/fsharp/issues/20288. + +Make the smallest clean, correct, complete fix. Keep it minimal and surgical. Quality matters more than time. Take all the time needed. Smaller is better, but not at the cost of correctness. Preserve progress and evidence across execution windows rather than truncate the work. + +**Verified root cause.** At main `b5c530ed6bc42937de6363e3dcc104ebb833893d`, `GenMethodForBinding` removes accessor-applied attributes before `ComputeMethodImplAttribs` decodes them. MethodImpl and standalone PreserveSig therefore bypass the flag decoder and enter actual MethodDef custom-attribute rows. Ordinary methods work. Current-main compilation succeeds for the valid fixtures, but raw metadata has 26 failing assertions across 13 accessors. Seven ordinary-method controls pass. External calls work, demonstrating why runtime success alone is not an adequate oracle. + +**Surgical candidate.** Move the existing accessor partition in `src/Compiler/CodeGen/IlxGen.fs:10002-10003` below `ComputeMethodImplAttribs` at `10011-10012`. Decode the full current-method list once, then partition the remaining ordinary attributes. Keep DllImport, CompiledName, property routing, security attributes, and method emission unchanged. Do not concatenate stale partitions, duplicate decoding, blacklist attributes globally, or modify the IL writer. This is a plan, not a tested patch. + +The decoder at `IlxGen.fs:9765-9800` currently implements NoInlining, AggressiveInlining, Synchronized, and PreserveSig, including combinations and standalone PreserveSigAttribute. Preserve that contract. Do not add ignored CLR option bits, MethodCodeType, or int16-constructor decoding. Do not normalize conflicting inlining bits. Property-level misuse currently emits warning FS0842 unless promoted; do not change that policy. Active dotnet/fsharp#20235 touches the decoder's surroundings but is not an equivalent accessor fix. Keep its unrelated feature out of this change. + +**RED-first test plan.** Use `tests/FSharp.Compiler.ComponentTests/EmittedIL/MethodImplAttribute/MethodImplAttribute.fs` and the existing `withMetadataReader` helper. Compile valid sources successfully, then inspect raw MethodDef implementation flags and actual CustomAttribute handles. Require expected method rows to exist. Do not use reflection-synthesized pseudo attributes or a retained call as the oracle. + +| Scenario | Required assertion | +|---|---| +| 1. Exact issue: static getter with AggressiveInlining, static setter with NoInlining, ordinary method control | Getter flags `0x100`, setter `0x8`, no actual MethodImpl custom attributes. Method control remains `0x100`. | +| 2. Instance property with different getter/setter flags and distinct property/getter/setter marker attributes | Getter exactly `0x100`, setter exactly `0xA8`. Preserve each marker's target. Do not leak flags or markers between accessors. | +| 3. Standalone PreserveSig plus MethodImpl(NoInlining) on a getter | Flags `0x88`, neither pseudo attribute stored as a real custom attribute. Ordinary equivalent remains unchanged. | +| 4. Explicit interface implementation | Inspect concrete accessor MethodDefs, not abstract slots. Getter `0x8`, setter `0x20`, no real MethodImpl attributes. | +| 5. Extension property | Static emitted accessor `0x108`, no real MethodImpl, ordinary getter marker retained. Cover the second accessor emission path. | +| 6. Attributed concrete accessors behind a neutral `.fsi` | Getter `0x8`, setter `0xA0`, no real pseudo attributes in the library. Reuse existing paired-source helpers. | +| 7. Ordinary-method and unannotated controls | Existing eight option baselines stay unchanged. Use compact parity checks for all supported bits, ignored bits, ignored MethodCodeType, int16 overload, and an unannotated accessor. Do not turn ignored options into new features. | +| 8. Property-level target misuse | Preserve FS0842 and current default severity. Promote it explicitly only when the test intends rejection. | +| 9. Separate compilation consuming the library | Normal, explicit-interface, and signature-constrained accessor calls retain their values and behavior. This is a control, not the flags oracle. | + +Rows 1-6 are the primary issue and five meaningful RED variants. Assert exact flags and pseudo-attribute absence together. Passing controls must not conceal a missing or skipped regression. Keep fixtures compact, share metadata checks, and parameterize only genuine variations. Reuse the ordinary-option baseline coverage instead of expanding a cross-product of flags, platforms, and compiler switches. + +First record current-main RED failures caused by wrong metadata, not invalid syntax or typechecking. Make the surgical change and get those tests GREEN without changing their expectations. Do not regenerate baselines or weaken absence assertions to obtain GREEN. Run the focused MethodImpl tests and the existing `MethodImplNoInline02_fs` optimization/realsig cases. Their starting baseline passed nine and four cases respectively. + +Format only changed F# files. Invoke `fsharp-diagnostics` after compiler edits and invoke the expert-review skill on the final work. Remove noise and deduplicate production/test code through existing helpers. Leave a clean, compact test suite and concise release note. No new API, diagnostic, attribute framework, or unrelated cleanup is needed. + +Sources: [issue](https://github.com/dotnet/fsharp/issues/20288), [early partition](https://github.com/dotnet/fsharp/blob/b5c530ed6bc42937de6363e3dcc104ebb833893d/src/Compiler/CodeGen/IlxGen.fs#L9993-L10012), [existing decoder](https://github.com/dotnet/fsharp/blob/b5c530ed6bc42937de6363e3dcc104ebb833893d/src/Compiler/CodeGen/IlxGen.fs#L9765-L9800), [attribute emission and flags](https://github.com/dotnet/fsharp/blob/b5c530ed6bc42937de6363e3dcc104ebb833893d/src/Compiler/CodeGen/IlxGen.fs#L10233-L10303). + +## Analysis + +The current task is architecture, not implementation. Produce self-contained sprint instructions and leave the compiler unchanged. + +The initial worktree is clean. HEAD matches the supplied baseline: `b5c530ed6bc42937de6363e3dcc104ebb833893d`. + +The requested output directory is ignored by Git. Preserve the requested planning files explicitly when committing. + +The shared issue queue database is absent at both configured Windows paths. This backlog preserves the implementation request locally. + +Repository inspection confirms the proposed ordering defect. `ComputeMethodImplAttribs` filters both pseudo attributes and decodes only the four supplied bits. The normal and extension emission paths both consume the accessor partition later. + +The issue is open with no comments. PR #20235 is open and implements runtime async. Its larger feature is explicitly out of scope. + +The test module contains eight option baselines and one inline-keyword warning case. `MethodImplNoInline02_fs` in `EmittedIL\Misc\Misc.fs` declares the four optimization/realsig combinations. + +`withMetadataReader` owns PE-reader setup in `tests\FSharp.Test.Utilities\Compiler.fs`. Public `Fsi`, `FsSource`, `withAdditionalSourceFile`, and `withReferences` helpers cover paired libraries and separate consumers. Do not depend on later-compiled signature-test modules. + +The required .NET 11 SDK is missing locally. `dotnet --version` reports the missing SDK, and only SDKs 8, 9, and 10 are installed. No `.dotnet` or `artifacts` directory exists. The sprint includes repository-wrapper provisioning before RED tests. Compiler builds and regression execution are implementation work, not completed planning evidence. + +The repository variable `VNEXT` is `11.0.100`. The release-note sink is `docs\release-notes\.FSharp.Compiler.Service\11.0.100.md`. + +## Approach + +Use one atomic sprint for the compiler change and its tests. Keep the RED evidence, GREEN verification, compatibility controls, review, and release note in that sprint. + +The sprint includes all nine requested scenarios, exact flags, handle-resolution requirements, example syntax, named helpers, concrete commands, and independent completion criteria. It distinguishes supplied investigation counts from local evidence. + +Run a structural and coverage check of both output files. Verify the original implementation request is preserved, paths resolve, and no production files changed. Commit only the requested planning files, even though `.tools` is ignored. Do not push. + +Local plan validation passed: one sprint, nine scenarios, 20 completion criteria, required frontmatter and headings, no checkboxes, and 11 existing reference paths. Git confirmed no production or staged changes before staging the plan. The native PowerShell check replaced an unavailable Python command. No compiler build or regression result is claimed. + +Final implementation verification must inspect the sprint's RED/GREEN logs, exact metadata expectations, unchanged baselines, local review result, release note, and commit. Successful runtime calls alone are insufficient. + +## Sprint Overview + +| # | Name | Purpose | +|---|---|---| +| 1 | `01_Fix_Accessor_MethodImpl.md` | Reproduce incorrect accessor metadata, move the partition, and verify the complete fix locally. | diff --git a/.tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md b/.tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md new file mode 100644 index 00000000000..eb9ee7c2b6b --- /dev/null +++ b/.tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md @@ -0,0 +1,322 @@ +--- +--- +# Sprint: Fix MethodImpl metadata on property accessors + +## Context - WHY this sprint exists + +Implement https://github.com/dotnet/fsharp/issues/20288 in `Q:\fsharp-worktrees\issue-877` using TDD. This file contains the complete work unit. No other sprint or backlog is required. + +Make the smallest clean, correct, complete fix. Validate locally, then commit. Do not push, open a PR, or post GitHub comments. + +At baseline `b5c530ed6bc42937de6363e3dcc104ebb833893d`, property accessor attributes bypass the existing implementation-flag decoder. `MethodImplAttribute` and standalone `PreserveSigAttribute` can enter real CustomAttribute rows instead. Ordinary methods work. + +In `src\Compiler\CodeGen\IlxGen.fs`, `GenMethodForBinding` partitions `attrsAppliedToGetterOrSetter` before calling `ComputeMethodImplAttribs`. The decoder therefore cannot see those attributes. + +The user reports 26 wrong-metadata assertions across 13 accessors, seven passing ordinary-method controls, and successful external calls. These are supplied investigation results, not tests executed by this planner. Runtime success alone cannot prove this fix. + +The issue description calls property-level misuse an error. The verified requirement takes precedence: preserve warning FS0842 at default severity. Explicit promotion can make it an error. + +PR #20235, "Enable runtime async via compiler intrinsics", also changes `IlxGen.fs`. It is not this accessor fix. Do not import its feature. + +### Starting state and prerequisites + +Planning inspected a clean worktree at the baseline above. Planning commits can now precede this sprint. Preserve existing changes and do not reset the branch. + +`global.json` requires SDK `11.0.100-rc.1.26420.103` and selects Microsoft.Testing.Platform. During planning, `dotnet --version` failed because that SDK was missing. Installed SDKs were 8, 9, and 10. No local `.dotnet` directory or `artifacts` directory existed. + +Provision the required SDK with the repository wrapper if it is still missing. Do not edit `global.json` or dependency versions to avoid setup. + +```powershell +Set-Location 'Q:\fsharp-worktrees\issue-877' +$env:BUILDING_USING_DOTNET = 'true' +dotnet --version +# Only when the required SDK is missing: +& .\eng\common\dotnet.ps1 --version +``` + +After provisioning, use the resolved SDK consistently. The commands below use `dotnet`. Substitute `& .\.dotnet\dotnet.exe` if the local SDK is not selected. Set `BUILDING_USING_DOTNET=true` in each new shell. + +Read `.github\copilot-instructions.md`, `.github\instructions\CodeGen.instructions.md`, `.github\instructions\ExpertReview.instructions.md`, `.github\instructions\ComponentTests.instructions.md`, `.github\instructions\NoBloat.instructions.md`, and `docs\representations.md` before editing. + +Files under `eng\common` are Arcade-owned. Run their setup wrapper but do not modify them. + +## Description - WHAT to implement with DETAILED guidance + +### Files to modify + +| Path relative to the repository | Change | +|---|---| +| `src\Compiler\CodeGen\IlxGen.fs` | Move the existing accessor partition below `ComputeMethodImplAttribs` in `GenMethodForBinding`. | +| `tests\FSharp.Compiler.ComponentTests\EmittedIL\MethodImplAttribute\MethodImplAttribute.fs` | Add compact raw-metadata regressions and compatibility controls. Preserve existing tests and baselines. | +| `docs\release-notes\.FSharp.Compiler.Service\11.0.100.md` | Add one concise entry under `### Fixed`, after confirming the release target. | + +No project-file change is needed when tests remain in the existing module. No public API, new diagnostic, attribute framework, or IL-writer change is needed. + +### Existing code and reusable patterns + +`ComputeMethodImplAttribs` is near lines 9765-9800 of `IlxGen.fs`. It decodes `NoInlining` (`0x8`), `Synchronized` (`0x20`), `PreserveSig` (`0x80`), and `AggressiveInlining` (`0x100`). It also consumes standalone `PreserveSigAttribute`. + +It removes both pseudo attributes from its returned ordinary-attribute list. Preserve its supported bits, combinations, ignored options, ignored `MethodCodeType`, and ignored `int16` constructor behavior. + +`GenMethodForBinding` currently has this order near lines 9993-10012: + +```fsharp +let attrs = // existing DllImport and CompiledName filtering + ... +let attrsAppliedToGetterOrSetter, attrs = + List.partition (fun (Attrib(_, _, _, _, isAppliedToGetterOrSetter, _, _)) -> isAppliedToGetterOrSetter) attrs +let sourceNameAttribs, compiledName = + ... +let hasPreserveSigImplFlag, hasSynchronizedImplFlag, hasNoInliningFlag, hasAggressiveInliningImplFlag, attrs = + ComputeMethodImplAttribs cenv v attrs +let securityAttributes, attrs = + ... +``` + +The ellipses above indicate existing code, not code to insert. Relocate only the two-line accessor partition. Place it immediately after the decoder call and before the security partition. + +This decodes the full current-method list once, after DllImport/CompiledName filtering. Then it partitions the remaining ordinary attributes. + +Leave `sourceNameAttribs`, `compiledName`, security processing, property routing, and method construction unchanged. Leave the normal accessor path near lines 10233-10265 and extension path near lines 10270-10285 unchanged. Leave `.WithPreserveSig`, `.WithSynchronized`, `.WithNoInlining`, and `.WithAggressiveInlining` near lines 10297-10303 unchanged. + +Use these existing test APIs from `FSharp.Test.Compiler` in `tests\FSharp.Test.Utilities\Compiler.fs`: + +```fsharp +FSharp source +|> asLibrary +|> compile +|> shouldSucceed +|> withMetadataReader (fun reader -> (* inspect raw metadata here *)) +``` + +`withMetadataReader`, near line 1000, already owns the PE-reader lifetime. Do not add another output-path/PE-reader wrapper. + +For paired sources, use the existing `Fsi` and `FsSource` helpers. They produce matching `test.fsi` and `test.fs` names: + +```fsharp +let library = + Fsi signatureSource + |> withAdditionalSourceFile (FsSource implementationSource) + |> asLibrary + |> withName "AccessorLibrary" +``` + +Examples exist in `tests\FSharp.Compiler.ComponentTests\Signatures\TestHelpers.fs` and `tests\FSharp.Compiler.ComponentTests\Conformance\Signatures\SignatureEnforcedAttributes.fs`. Reuse the public source helpers, not those modules' private helpers. The signature helpers module compiles after this test module. + +For a separate consumer, use `withReferences [ library ]`, `asExe`, an explicit entry point, and `compileExeAndRun |> shouldSucceed`. Pass a `CompilationUnit` to `withReferences`, not a `CompilationResult`. Keep consumer source out of the library's source list. + +### Step 1: Establish the baseline and preserve evidence + +Create an ignored evidence directory at `.tools\ralph\evidence\20288`. Save logs there, not among committed source files. Preserve the baseline hash, commands, SDK version, exit codes, test counts, and RED/GREEN diagnostics. + +Use `.tools\ralph\evidence\20288\progress.txt` for a short resume record. Include completed scenarios and the next command. Never mark a phase complete without its evidence. + +Before production edits, run the existing focused MethodImpl tests and `MethodImplNoInline02_fs`. The supplied starting results were nine and four passing cases respectively. Re-establish those counts locally. + +Build/test commands are given below. Resolve setup failures before diagnosing the regression. Missing SDKs, invalid fixture syntax, and typechecking failures do not count as RED. + +### Step 2: Add raw-metadata tests and obtain RED + +Keep additions in the existing `EmittedIL.MethodImplAttribute` module. Retain its eight option-baseline tests and existing inline-keyword warning test without changes. + +Use one shared metadata-checking path for new fixtures. Parameterize actual variations, rather than copying complete test bodies. Keep each of scenarios 1-6 independently observable in test output. A failure in scenario 1 must not prevent scenarios 2-6 from running. + +Select each expected declaring type and method by exact metadata identity. Require exactly one match, or fail with its expected identity. Do not filter discovered methods and then pass on an empty sequence. + +Read `MethodDefinition.ImplAttributes` as an integer and assert the complete expected value, not just selected bits. Read `MethodDefinition.GetCustomAttributes()` and resolve actual constructor handles through `MetadataReader.GetCustomAttribute`. + +Use `System.Reflection.Metadata`. External attribute constructors usually use `HandleKind.MemberReference` with a `TypeReference` parent. Locally declared markers can use `HandleKind.MethodDefinition` and `GetDeclaringType()`. Resolve both correctly. Handle any encountered `TypeDefinition` parent. Fail explicitly on unexpected shapes rather than returning an empty name or silently skipping a row. + +Compare the combined observation of exact flags and pseudo-attribute absence for each accessor. Include full attribute names: + +```text +System.Runtime.CompilerServices.MethodImplAttribute +System.Runtime.InteropServices.PreserveSigAttribute +``` + +No real row for either pseudo attribute is allowed on the tested concrete methods. Do not reject unrelated compiler-generated attributes. For marker checks, compare the marker subset precisely. + +Do not use reflection attribute enumeration. Reflection can synthesize pseudo attributes from implementation flags. Do not use an IL call instruction or successful execution as the flags oracle. + +#### Required scenario matrix + +| # | Fixture and construction guidance | Assertions | +|---|---|---| +| 1 | Exact issue source shown below: static getter, static setter, and ordinary static method. | `A.get_P1 = 0x100`, `B.set_P2 = 0x8`, `C.M1 = 0x100`. Both accessors and the method lack real pseudo attributes. | +| 2 | Instance property with getter `AggressiveInlining` and setter `NoInlining ||| Synchronized ||| PreserveSig`. Define three small marker attribute types, valid respectively on property, getter method, and setter method. | Getter `0x100`, setter `0xA8`, no real pseudo attributes. Read the PropertyDef and both MethodDefs. Each marker appears exactly once on its intended row and on neither other row. | +| 3 | Getter with standalone `[]` plus `[]`. Include an ordinary-method equivalent. | Both methods have `0x88`. Neither pseudo attribute has a real row. | +| 4 | Interface with `abstract P: int with get, set`, implemented explicitly by a concrete class. Attribute the concrete getter with `NoInlining`, and setter with `Synchronized`. | Concrete getter `0x8`, concrete setter `0x20`, no real pseudo attributes. Select the implementing type's MethodDefs, not interface slots. Require non-abstract methods with bodies. | +| 5 | Extrinsic extension property, such as `type System.String with ...` inside a module. Apply `NoInlining ||| AggressiveInlining` and an ordinary getter marker. | Emitted extension accessor is static, has `0x108`, retains its marker, and lacks real pseudo attributes. Do not normalize conflicting inlining flags. This must exercise the extension emission path. | +| 6 | Neutral `.fsi` declares a class constructor and `member P: int with get, set`. Matching `.fs` puts `NoInlining` on the getter and `Synchronized ||| PreserveSig` on the setter. | Concrete library getter `0x8`, setter `0xA0`, no real pseudo attributes. No attributes are needed in the signature. | +| 7 | Compact ordinary-method/accessor parity controls, described below. | Preserve current ordinary-method behavior and ensure an unannotated accessor remains `0x0`. Keep all eight existing option baselines unchanged. | +| 8 | Place `[]` before a property member rather than on its accessor. | Default diagnostic is warning FS0842. A separate, explicitly promoted case rejects it as error FS0842. | +| 9 | Separately compile an executable referencing normal, explicit-interface, and signature-constrained library fixtures. Reuse fixture source definitions. | Getters return expected values. Setters update backing values and subsequent getters observe the updates. Assert interface-dispatched calls and signature-constrained calls. This is a control only. | + +Exact issue source: + +```fsharp +module P +open System.Runtime.CompilerServices + +[] +type A = + static member P1 with [] get () = 1 + +[] +type B = + static member P2 with [] set (v: int) = ignore v + +[] +type C = + [] + static member M1() = 1 +``` + +For instance and explicit-interface fixtures, use a mutable integer backing field with a simple get/set body. Place accessor attributes after `with` or `and`, immediately before `get` or `set`. Ensure setters accept `int`. + +For scenario 5, extend an external type in a module to guarantee an extrinsic extension. A declaration such as `type System.String with member s.P with [<...>] get () = s.Length` is the intended shape. Discover the emitted qualified method name, then assert its exact identity and static flag. Do not assume an ordinary PropertyDef exists for this path. + +Use the paired-source helper pattern for scenario 6. The neutral signature can have this shape: + +```fsharp +module SignatureLibrary +type C = + new: unit -> C + member P: int with get, set +``` + +For scenario 7, use a compact data table, not a flags/platform/optimization/realsig cross-product. These constructor expressions and results describe the existing decoder: + +| Attribute argument or form | Expected implementation flags | +|---|---| +| `MethodImplOptions.NoInlining` | `0x8` | +| `MethodImplOptions.Synchronized` | `0x20` | +| `MethodImplOptions.PreserveSig` | `0x80` | +| `MethodImplOptions.AggressiveInlining` | `0x100` | +| Standalone `PreserveSigAttribute` | `0x80` | +| `MethodImplOptions.NoInlining ||| MethodImplOptions.AggressiveInlining` | `0x108` | +| Each of `ForwardRef`, `InternalCall`, `NoOptimization`, `Unmanaged`, and `AggressiveOptimization` | `0x0` | +| `MethodImplOptions.NoInlining, MethodCodeType = MethodCodeType.Native` | `0x8`, with the method still implemented as IL | +| `MethodImpl(8s)` using the `int16` constructor | `0x0` | +| No attribute | `0x0` | + +Reuse existing ordinary-option baseline coverage instead of adding equivalent text baselines. Add only the small raw checks needed for accessor parity and uncovered ordinary forms. Apply the pseudo-attribute absence check to annotated controls too. Record ordinary-control results before the compiler edit. + +For scenario 8, use `typecheck`, not compilation, because only diagnostic policy matters. Assert the exact diagnostic code and severity. Use `ignoreWarnings` only to let the harness accept the expected warning, then assert it with `withSingleDiagnostic` or equivalent exact diagnostics. + +`ignoreWarnings` changes harness acceptance; it is not an instruction to suppress FS0842. For the rejection case, use `withOptions [ "--warnaserror:842" ]` and assert an error. Do not add broad warning suppression to the valid metadata fixtures. + +Run all new scenarios against the unchanged compiler. Save metadata failures for each of scenarios 1-6. Each source must compile successfully before its metadata assertion fails. Capture actual flags and actual pseudo attributes, not merely a nonzero test exit code. + +Do not edit production code until these RED results exist. The supplied 26-assertion count describes an earlier investigation, not a requirement to duplicate its assertion structure. All six primary scenarios must be demonstrated locally. + +### Step 3: Apply the surgical change and obtain GREEN + +Move the existing partition as described above. Do not concatenate saved attribute partitions, decode twice, blacklist attributes globally, or modify `ComputeMethodImplAttribs`. + +Invoke the `fsharp-diagnostics` skill immediately after editing `IlxGen.fs`. Follow its parse-first, typecheck-second workflow: + +```powershell +& .\.github\skills\fsharp-diagnostics\scripts\get-fsharp-errors.ps1 -ParseOnly src\Compiler\CodeGen\IlxGen.fs +& .\.github\skills\fsharp-diagnostics\scripts\get-fsharp-errors.ps1 src\Compiler\CodeGen\IlxGen.fs +``` + +These checks do not replace a build. Rebuild the compiler and tests, then rerun the unchanged regression expectations. All scenarios must become GREEN. + +Keep DllImport handling, CompiledName handling, ordinary attributes, security attributes, property rows, and both accessor emission paths unchanged except for the corrected pseudo-attribute consumption. + +### Local validation commands + +Use Release for EmittedIL tests and optimization cases. `BUILDING_USING_DOTNET=true` selects the current .NET target instead of also building desktop tests on Windows. + +```powershell +$env:BUILDING_USING_DOTNET = 'true' +dotnet msbuild tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -getProperty:TargetFrameworks +dotnet build tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -c Release -v minimal +``` + +Stop on a failed build. Use `--no-build` only after a successful build of the current source. Confirm the test project uses the rebuilt local compiler, not an installed SDK compiler or stale output. + +Run these focused selections before adding tests, for RED, and after the fix as appropriate: + +```powershell +$env:BUILDING_USING_DOTNET = 'true' +dotnet test --project tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -c Release --no-build -- --filter-class "*EmittedIL.MethodImplAttribute*" +dotnet test --project tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -c Release --no-build -- --filter-method "*MethodImplNoInline02_fs*" +``` + +The existing `MethodImplNoInline02_fs` test is in `tests\FSharp.Compiler.ComponentTests\EmittedIL\Misc\Misc.fs`, near line 200. Its `FileInlineData` sets both `Realsig` and `Optimize` to `BooleanOptions.Both`, yielding four cases. Preserve that matrix. + +Verify selection and nonzero test counts. If the installed runner rejects a selector, inspect its help and use the equivalent xUnit v3 filter. Do not report an empty run as success. If necessary, execute the built test DLL with the same filters after discovering its output path. + +Keep logs and exit codes for each command. Escalate to related tests only when focused results require it. A full component run is not a substitute for the specific regression evidence. + +On an actual build failure, invoke `binlog-analysis`, collect a binary log, and fix the cause. If stale bootstrap output is suspected, preserve logs first. Inspect `git clean -ndx -- artifacts` before cleaning only that generated directory with `git clean -xfd -- artifacts`, then rebuild. Do not delete source, `.tools\ralph`, or another worktree. + +### Step 4: Format, review, document, and commit + +Format only changed F# files. The explicit request overrides repository-wide formatting: + +```powershell +dotnet fantomas src\Compiler\CodeGen\IlxGen.fs tests\FSharp.Compiler.ComponentTests\EmittedIL\MethodImplAttribute\MethodImplAttribute.fs +``` + +If Fantomas is missing, restore the existing local tools with `dotnet tool restore`, then rerun. Do not install a new formatter or change tool versions. + +Inspect the formatting diff and remove unrelated churn. Re-run compiler diagnostics if formatting changes compiler source. Rebuild and rerun focused tests after the final source changes. + +Invoke the `reviewing-compiler-prs` skill and its `expert-reviewer` agent on the final implementation diff. This is the available expert-review workflow. Give it the issue contract, baseline hash, exact changed files, and RED/GREEN evidence. Review locally only. Do not let a reviewer post, push, or open a PR. + +Require checks of exact flags, actual pseudo-attribute absence, non-vacuous method selection, both emission paths, marker routing, and compatibility controls. Resolve concrete findings and rerun affected checks. Invoke `code-compaction` if the test diff becomes repetitive, overengineered, or exceeds its size trigger. Do not add test-only frameworks or unrelated cleanup. + +Invoke the `release-notes` skill. `VNEXT` was `11.0.100` during planning. Confirm with `gh api repos/dotnet/fsharp/actions/variables/VNEXT --jq .value`. Use the compiler-service sink, not FSharp.Core or a new language-feature note. + +Use the insertion helper rather than prepending: + +```powershell +dotnet fsi .github\skills\release-notes\pick-insert-line.fsx --file docs\release-notes\.FSharp.Compiler.Service\11.0.100.md --section Fixed +``` + +One suitable entry is: + +```markdown +* Fix `MethodImpl` and `PreserveSig` attributes on property accessors to emit method implementation flags instead of real custom attributes. ([Issue #20288](https://github.com/dotnet/fsharp/issues/20288)) +``` + +Use the issue link because this is commit-only work. Do not fabricate a PR number or open a PR to obtain one. + +Finish with `git diff --check` and inspect the complete diff. Existing `.bsl` and `.il.bsl` files must be unchanged. Do not set `TEST_UPDATE_BSL` or regenerate baselines to obtain GREEN. + +Stage only the implementation, regression tests, and release note. Do not commit evidence logs, generated binaries, temporary source files, or changes owned by another task. Existing planning files can remain tracked but must not enter the production diff as new implementation changes. + +Commit with a descriptive message, for example `Fix MethodImpl flags on property accessors`. Include these trailers, substituting the implementing agent's actual session ID: + +```text +Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> +Copilot-Session: +``` + +Record the resulting commit hash and final verification outcomes in the ignored progress record. Leave no uncommitted changes from this sprint. Do not push. + +## Definition of Done - independently verifiable criteria + +- The existing focused suite passes nine cases before new tests, and the existing optimization/realsig selection passes four cases. +- Before production edits, scenarios 1-6 compile successfully and each exposes incorrect raw metadata in preserved RED logs. +- Every expected type and concrete accessor is required to exist exactly once; no regression can pass through an empty selection. +- Scenarios 1-6 assert the exact flags from the matrix and absence of actual MethodImpl/PreserveSig custom attributes together. +- Scenario 2 verifies property/getter/setter marker ownership, including absence from the other two targets. +- Scenario 4 checks concrete explicit-interface methods with bodies, not abstract slots. +- Scenario 5 checks the static extension accessor, retains its marker, and preserves the combined `0x108` flags. +- Scenario 6 verifies the emitted library behind a neutral signature using paired-source helpers. +- Scenario 7 preserves supported bits, standalone PreserveSig, ignored bits, ignored MethodCodeType, ignored int16 decoding, and unannotated-accessor behavior. +- Scenario 8 preserves warning FS0842 by default and rejects only the explicitly promoted test case. +- Scenario 9 separately compiles and executes normal, explicit-interface, and signature-constrained getter/setter calls with value assertions. +- The production diff only relocates the existing accessor partition after the decoder; unrelated routing and emission logic remain unchanged. +- Compiler diagnostics and the Release build succeed with no new warnings after the final source edit. +- All focused MethodImpl tests and all four `MethodImplNoInline02_fs` cases pass locally with unchanged expectations and no skipped regressions. +- Existing option baselines are unchanged, and no baseline regeneration was used. +- Only changed F# files are formatted, and `git diff --check` succeeds. +- Local expert review is complete, concrete findings are resolved, and affected checks were rerun. +- A concise compiler-service release note links to issue #20288. +- Evidence and resume state persist under `.tools\ralph\evidence\20288`, while temporary implementation artifacts are removed. +- The implementation, tests, and release note are committed with the required trailers; no sprint-owned changes remain uncommitted, and nothing was pushed. From e670bad97dfdba69ed6edf5ff46b430eaa64915d Mon Sep 17 00:00:00 2001 From: Copilot Date: Tue, 15 Sep 2026 15:42:49 +0200 Subject: [PATCH 2/4] Fix MethodImpl flags on property accessors Decode implementation flags before separating accessor attributes, preserving ordinary attribute routing and existing decoder behavior. Add raw-metadata regressions, compatibility controls, diagnostic checks and external-call coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ffa8facb-33f4-430a-8b15-ff25b03841bc --- .../.FSharp.Compiler.Service/11.0.100.md | 1 + src/Compiler/CodeGen/IlxGen.fs | 6 +- .../MethodImplAttribute.fs | 380 ++++++++++++++++++ 3 files changed, 384 insertions(+), 3 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 4df01e7ceb0..ea10192f980 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -106,6 +106,7 @@ * Fix internal error `FS0192: encodeCustomAttrElemType` when using arrays of user-defined types as custom attribute arguments. Empty arrays (e.g. `[]`) now compile successfully; non-empty arrays of unencodable types report a proper diagnostic (FS3887) instead of an internal error. ([Issue #12796](https://github.com/dotnet/fsharp/issues/12796), [PR #19472](https://github.com/dotnet/fsharp/pull/19472)) * Cleanup in IL base-call checking: skip `resolveILMethodRefWithRescope` when the method is not declared on the immediate IL type, instead of relying on the `try/with` around a `failwith`. `FS1201` still applies when the abstract member is declared on the immediate IL base. ([Issue #20264](https://github.com/dotnet/fsharp/issues/20264), [PR #20272](https://github.com/dotnet/fsharp/pull/20272)) * Fix internal compiler error in `use` bindings when a C#-style `Dispose` extension method is in scope alongside `IDisposable.Dispose`. ([Issue #19552](https://github.com/dotnet/fsharp/issues/19552), [PR #19568](https://github.com/dotnet/fsharp/pull/19568)) +* Fix `MethodImpl` and `PreserveSig` attributes on property accessors to emit method implementation flags instead of real custom attributes. ([Issue #20288](https://github.com/dotnet/fsharp/issues/20288)) * Fix signature generation: single-case struct DU gets spurious bar causing FS0300. ([Issue #19597](https://github.com/dotnet/fsharp/issues/19597), [PR #19609](https://github.com/dotnet/fsharp/pull/19609)) * Fix signature generation: backticked active pattern case names lose escaping. ([Issue #19592](https://github.com/dotnet/fsharp/issues/19592), [PR #19609](https://github.com/dotnet/fsharp/pull/19609)) * Fix signature generation: `namespace global` header dropped from generated signature. ([Issue #19593](https://github.com/dotnet/fsharp/issues/19593), [PR #19609](https://github.com/dotnet/fsharp/pull/19609)) diff --git a/src/Compiler/CodeGen/IlxGen.fs b/src/Compiler/CodeGen/IlxGen.fs index ae028f41cd4..f18d5b22adc 100644 --- a/src/Compiler/CodeGen/IlxGen.fs +++ b/src/Compiler/CodeGen/IlxGen.fs @@ -9999,9 +9999,6 @@ and GenMethodForBinding (WellKnownValAttributes.DllImportAttribute ||| WellKnownValAttributes.CompiledNameAttribute) - let attrsAppliedToGetterOrSetter, attrs = - List.partition (fun (Attrib(_, _, _, _, isAppliedToGetterOrSetter, _, _)) -> isAppliedToGetterOrSetter) attrs - let sourceNameAttribs, compiledName = match tryFindValAttribByFlag g WellKnownValAttributes.CompiledNameAttribute v.Attribs with | Some(Attrib(_, _, [ AttribStringArg b ], _, _, _, _)) -> [ mkCompilationSourceNameAttr g v.LogicalName ], Some b @@ -10011,6 +10008,9 @@ and GenMethodForBinding let hasPreserveSigImplFlag, hasSynchronizedImplFlag, hasNoInliningFlag, hasAggressiveInliningImplFlag, attrs = ComputeMethodImplAttribs cenv v attrs + let attrsAppliedToGetterOrSetter, attrs = + List.partition (fun (Attrib(_, _, _, _, isAppliedToGetterOrSetter, _, _)) -> isAppliedToGetterOrSetter) attrs + let securityAttributes, attrs = attrs |> List.partition (fun a -> IsSecurityAttribute g cenv.amap cenv.casApplied a m) diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/MethodImplAttribute/MethodImplAttribute.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/MethodImplAttribute/MethodImplAttribute.fs index 0c4e437999b..cd99c0bc5d8 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/MethodImplAttribute/MethodImplAttribute.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/MethodImplAttribute/MethodImplAttribute.fs @@ -1,5 +1,7 @@ namespace EmittedIL +open System.Reflection +open System.Reflection.Metadata open Xunit open FSharp.Test open FSharp.Test.Compiler @@ -80,3 +82,381 @@ module MethodImplAttribute = compilation |> getCompilation |> verifyCompilation + + let private verifyMetadata expectedMethods expectedProperties compilation = + compilation + |> asLibrary + |> compile + |> shouldSucceed + |> withMetadataReader (fun reader -> + let qualify ns name = + if ns = "" then name else ns + "." + name + + let rec typeName (handle: EntityHandle) = + match handle.Kind with + | HandleKind.TypeDefinition -> + let def = reader.GetTypeDefinition(TypeDefinitionHandle.op_Explicit handle) + let name = reader.GetString def.Name + let parent = def.GetDeclaringType() + + if parent.IsNil then + qualify (reader.GetString def.Namespace) name + else + typeName (TypeDefinitionHandle.op_Implicit parent) + "+" + name + | HandleKind.TypeReference -> + let reference = reader.GetTypeReference(TypeReferenceHandle.op_Explicit handle) + qualify (reader.GetString reference.Namespace) (reader.GetString reference.Name) + | kind -> failwithf "Unexpected attribute/type handle: %A" kind + + let attributes handles = + [ + for handle in handles do + let attribute = reader.GetCustomAttribute handle + + let parent = + match attribute.Constructor.Kind with + | HandleKind.MemberReference -> + reader.GetMemberReference(MemberReferenceHandle.op_Explicit attribute.Constructor).Parent + | HandleKind.MethodDefinition -> + let ctor = + reader.GetMethodDefinition(MethodDefinitionHandle.op_Explicit attribute.Constructor) + + TypeDefinitionHandle.op_Implicit (ctor.GetDeclaringType()) + | kind -> failwithf "Unexpected attribute constructor: %A" kind + + yield typeName parent + ] + + let exactlyOne identity items = + match Seq.toList items with + | [ item ] -> item + | items -> failwithf "Expected exactly one %s; found %d" identity items.Length + + let findType name = + reader.TypeDefinitions + |> Seq.filter (fun handle -> typeName (TypeDefinitionHandle.op_Implicit handle) = name) + |> exactlyOne name + |> reader.GetTypeDefinition + + let markers = + [ + "Markers.PropertyMarkerAttribute" + "Markers.GetterMarkerAttribute" + "Markers.SetterMarkerAttribute" + ] + + let markerAttributes = + List.filter (fun name -> List.contains name markers) >> List.sort + + let pseudoAttributes = + List.filter (fun name -> + name = "System.Runtime.CompilerServices.MethodImplAttribute" + || name = "System.Runtime.InteropServices.PreserveSigAttribute") + >> List.sort + + let actualMethods = + [ + for declaringType, name, _, isStatic, _ in expectedMethods do + let identity = declaringType + "." + name + + let methods = + (findType declaringType).GetMethods() + |> Seq.map reader.GetMethodDefinition + |> Seq.toList + + let methodDef = + methods + |> List.filter (fun def -> reader.GetString def.Name = name) + |> exactlyOne ( + sprintf "%s (available: %A)" identity (methods |> List.map (fun def -> reader.GetString def.Name)) + ) + + Assert.False(methodDef.Attributes.HasFlag MethodAttributes.Abstract, identity) + Assert.True(methodDef.RelativeVirtualAddress <> 0, identity + " must have a body") + Assert.True(methodDef.Attributes.HasFlag MethodAttributes.Static = isStatic, identity + " static flag") + let attrs = attributes (methodDef.GetCustomAttributes()) + yield identity, int methodDef.ImplAttributes, pseudoAttributes attrs, markerAttributes attrs + ] + + let expectedMethods = + [ + for declaringType, name, flags, _, markers in expectedMethods do + yield declaringType + "." + name, flags, [], List.sort markers + ] + + Assert.True( + (expectedMethods = actualMethods), + sprintf "Expected metadata:\n%A\nActual metadata:\n%A" expectedMethods actualMethods + ) + + for declaringType, name, expectedMarkers in expectedProperties do + let identity = declaringType + "." + name + + let property = + (findType declaringType).GetProperties() + |> Seq.map reader.GetPropertyDefinition + |> Seq.filter (fun def -> reader.GetString def.Name = name) + |> exactlyOne identity + + let actualMarkers = attributes (property.GetCustomAttributes()) |> markerAttributes + Assert.True(List.sort expectedMarkers = actualMarkers, sprintf "%s markers: %A" identity actualMarkers)) + + let private markerSource = + """ +namespace Markers +open System +[] +type PropertyMarkerAttribute() = inherit Attribute() +[] +type GetterMarkerAttribute() = inherit Attribute() +[] +type SetterMarkerAttribute() = inherit Attribute() +""" + + let private instanceLibrary = + FSharp( + markerSource + + """ +namespace InstanceLibrary +open Markers +open System.Runtime.CompilerServices +type C() = + let mutable value = 1 + [] + member _.P + with [] get () = value + and [] set (v: int) = value <- v + """ + ) + |> asLibrary + |> withName "InstanceLibrary" + + let private interfaceLibrary = + FSharp + """ +namespace InterfaceLibrary +open System.Runtime.CompilerServices +type I = + abstract P: int with get, set +type C() = + let mutable value = 2 + interface I with + member _.P + with [] get () = value + and [] set (v: int) = value <- v +""" + |> asLibrary + |> withName "InterfaceLibrary" + + let private signatureLibrary = + Fsi + """ +module SignatureLibrary +type C = + new: unit -> C + member P: int with get, set +""" + |> withAdditionalSourceFile ( + FsSource + """ +module SignatureLibrary +open System.Runtime.CompilerServices +type C() = + let mutable value = 3 + member _.P + with [] get () = value + and [] set (v: int) = value <- v + """ + ) + |> asLibrary + |> withName "SignatureLibrary" + + [] + [] + [] + [] + [] + [] + [] + let ``Accessor implementation flags are metadata, not custom attributes`` scenario = + let library, methods, properties = + match scenario with + | "issue" -> + FSharp + """ +module P +open System.Runtime.CompilerServices + +[] +type A = + static member P1 with [] get () = 1 + +[] +type B = + static member P2 with [] set (v: int) = ignore v + +[] +type C = + [] + static member M1() = 1 + """, + [ + "P+A", "get_P1", 0x100, true, [] + "P+B", "set_P2", 0x8, true, [] + "P+C", "M1", 0x100, true, [] + ], + [] + | "instance markers" -> + instanceLibrary, + [ + "InstanceLibrary.C", "get_P", 0x100, false, [ "Markers.GetterMarkerAttribute" ] + "InstanceLibrary.C", "set_P", 0xA8, false, [ "Markers.SetterMarkerAttribute" ] + ], + [ "InstanceLibrary.C", "P", [ "Markers.PropertyMarkerAttribute" ] ] + | "standalone PreserveSig" -> + FSharp + """ +namespace Standalone +open System.Runtime.CompilerServices +open System.Runtime.InteropServices +type C() = + member _.P with [] get () = 1 + [] + member _.M() = 1 + """, + [ + "Standalone.C", "get_P", 0x88, false, [] + "Standalone.C", "M", 0x88, false, [] + ], + [] + | "explicit interface" -> + interfaceLibrary, + [ + "InterfaceLibrary.C", "InterfaceLibrary.I.get_P", 0x8, false, [] + "InterfaceLibrary.C", "InterfaceLibrary.I.set_P", 0x20, false, [] + ], + [] + | "extrinsic extension" -> + FSharp( + markerSource + + """ +module Extensions = + open System.Runtime.CompilerServices + type System.String with + member s.P with [] get () = s.Length + """ + ), + [ + "Markers.Extensions", "String.get_P", 0x108, true, [ "Markers.GetterMarkerAttribute" ] + ], + [] + | "neutral signature" -> + signatureLibrary, + [ + "SignatureLibrary+C", "get_P", 0x8, false, [] + "SignatureLibrary+C", "set_P", 0xA0, false, [] + ], + [] + | _ -> failwithf "Unknown accessor scenario: %s" scenario + + library |> verifyMetadata methods properties + + let implementationFlagCases = + [ + for attribute, flags, ordinary in + [ + "MethodImpl(MethodImplOptions.NoInlining)", 0x8, false + "MethodImpl(MethodImplOptions.Synchronized)", 0x20, false + "MethodImpl(MethodImplOptions.PreserveSig)", 0x80, false + "MethodImpl(MethodImplOptions.AggressiveInlining)", 0x100, false + "PreserveSig", 0x80, true + "MethodImpl(MethodImplOptions.NoInlining ||| MethodImplOptions.AggressiveInlining)", 0x108, true + "MethodImpl(MethodImplOptions.ForwardRef)", 0, false + "MethodImpl(MethodImplOptions.InternalCall)", 0, false + "MethodImpl(MethodImplOptions.NoOptimization)", 0, false + "MethodImpl(MethodImplOptions.Unmanaged)", 0, false + "MethodImpl(enum(0x200))", 0, true // AggressiveOptimization is absent on net472. + "MethodImpl(MethodImplOptions.NoInlining, MethodCodeType = MethodCodeType.Native)", 0x8, true + "MethodImpl(8s)", 0, true + "", 0, false + ] do + for accessor in [ true; false ] do + if accessor || ordinary then + yield [| box attribute; box flags; box accessor |] + ] + + [] + let ``Implementation flag compatibility`` attribute flags accessor = + let annotation = if attribute = "" then "" else "[<" + attribute + ">]" + + let memberSource = + if accessor then + "member _.P with " + annotation + " get () = 1" + else + annotation + "\n member _.M() = 1" + + FSharp( + """ +namespace Compatibility +open System.Runtime.CompilerServices +open System.Runtime.InteropServices +type C() = + """ + + memberSource + ) + |> verifyMetadata [ "Compatibility.C", (if accessor then "get_P" else "M"), flags, false, [] ] [] + + [] + let ``Property-level MethodImpl remains warning FS0842 unless promoted`` promote = + let compilation = + FSharp + """ +module InvalidTarget +open System.Runtime.CompilerServices +type C() = + [] + member _.P = 1 +""" + |> asLibrary + + let result, severity = + if promote then + compilation |> withOptions [ "--warnaserror:842" ] |> typecheck |> shouldFail, Error 842 + else + compilation |> ignoreWarnings |> typecheck |> shouldSucceed, Warning 842 + + result + |> withSingleDiagnostic ( + severity, + Line 5, + Col 7, + Line 5, + Col 47, + "This attribute cannot be applied to property, event, return value. Valid targets are: constructor, method" + ) + + [] + let ``External callers can get and set attributed properties`` () = + FSharp + """ +module Consumer +[] +let main _ = + let normal = InstanceLibrary.C() + if normal.P <> 1 then failwith "normal getter" + normal.P <- 11 + if normal.P <> 11 then failwith "normal setter" + let explicit = InterfaceLibrary.C() :> InterfaceLibrary.I + if explicit.P <> 2 then failwith "interface getter" + explicit.P <- 22 + if explicit.P <> 22 then failwith "interface setter" + let constrained = SignatureLibrary.C() + if constrained.P <> 3 then failwith "signature getter" + constrained.P <- 33 + if constrained.P <> 33 then failwith "signature setter" + 0 +""" + |> withReferences [ instanceLibrary; interfaceLibrary; signatureLibrary ] + |> asExe + |> compileExeAndRun + |> shouldSucceed From a261c9d24f767c88e9f1addd237be1de9b2f9215 Mon Sep 17 00:00:00 2001 From: Copilot Date: Tue, 15 Sep 2026 16:46:37 +0200 Subject: [PATCH 3/4] Add release notes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12329be-d120-43cd-baaa-0a0683e64c00 --- docs/release-notes/.FSharp.Compiler.Service/11.0.100.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index ea10192f980..e981f0bf2eb 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -106,7 +106,7 @@ * Fix internal error `FS0192: encodeCustomAttrElemType` when using arrays of user-defined types as custom attribute arguments. Empty arrays (e.g. `[]`) now compile successfully; non-empty arrays of unencodable types report a proper diagnostic (FS3887) instead of an internal error. ([Issue #12796](https://github.com/dotnet/fsharp/issues/12796), [PR #19472](https://github.com/dotnet/fsharp/pull/19472)) * Cleanup in IL base-call checking: skip `resolveILMethodRefWithRescope` when the method is not declared on the immediate IL type, instead of relying on the `try/with` around a `failwith`. `FS1201` still applies when the abstract member is declared on the immediate IL base. ([Issue #20264](https://github.com/dotnet/fsharp/issues/20264), [PR #20272](https://github.com/dotnet/fsharp/pull/20272)) * Fix internal compiler error in `use` bindings when a C#-style `Dispose` extension method is in scope alongside `IDisposable.Dispose`. ([Issue #19552](https://github.com/dotnet/fsharp/issues/19552), [PR #19568](https://github.com/dotnet/fsharp/pull/19568)) -* Fix `MethodImpl` and `PreserveSig` attributes on property accessors to emit method implementation flags instead of real custom attributes. ([Issue #20288](https://github.com/dotnet/fsharp/issues/20288)) +* Fix `MethodImpl` and `PreserveSig` attributes on property accessors to emit method implementation flags instead of real custom attributes. ([Issue #20288](https://github.com/dotnet/fsharp/issues/20288), [PR #20558](https://github.com/dotnet/fsharp/pull/20558)) * Fix signature generation: single-case struct DU gets spurious bar causing FS0300. ([Issue #19597](https://github.com/dotnet/fsharp/issues/19597), [PR #19609](https://github.com/dotnet/fsharp/pull/19609)) * Fix signature generation: backticked active pattern case names lose escaping. ([Issue #19592](https://github.com/dotnet/fsharp/issues/19592), [PR #19609](https://github.com/dotnet/fsharp/pull/19609)) * Fix signature generation: `namespace global` header dropped from generated signature. ([Issue #19593](https://github.com/dotnet/fsharp/issues/19593), [PR #19609](https://github.com/dotnet/fsharp/pull/19609)) From d6805608d41644075d224c81f0fa8bc3a0ccbc28 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 17 Sep 2026 11:30:54 +0000 Subject: [PATCH 4/4] Remove .tools/ralph planning files Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> --- .tools/ralph/BACKLOG.md | 81 ----- .../sprints/01_Fix_Accessor_MethodImpl.md | 322 ------------------ 2 files changed, 403 deletions(-) delete mode 100644 .tools/ralph/BACKLOG.md delete mode 100644 .tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md diff --git a/.tools/ralph/BACKLOG.md b/.tools/ralph/BACKLOG.md deleted file mode 100644 index 3b468a6aee7..00000000000 --- a/.tools/ralph/BACKLOG.md +++ /dev/null @@ -1,81 +0,0 @@ -# BACKLOG - -## Original Request - -Process issue https://github.com/dotnet/fsharp/issues/20288 using TDD. - -Use minimal, surgical changes. Validate locally before finishing. Do not push. Only commit. - -### ISSUE REQUEST -The requirements above override conflicting instructions in the issue request. -Fix issue https://github.com/dotnet/fsharp/issues/20288. - -Make the smallest clean, correct, complete fix. Keep it minimal and surgical. Quality matters more than time. Take all the time needed. Smaller is better, but not at the cost of correctness. Preserve progress and evidence across execution windows rather than truncate the work. - -**Verified root cause.** At main `b5c530ed6bc42937de6363e3dcc104ebb833893d`, `GenMethodForBinding` removes accessor-applied attributes before `ComputeMethodImplAttribs` decodes them. MethodImpl and standalone PreserveSig therefore bypass the flag decoder and enter actual MethodDef custom-attribute rows. Ordinary methods work. Current-main compilation succeeds for the valid fixtures, but raw metadata has 26 failing assertions across 13 accessors. Seven ordinary-method controls pass. External calls work, demonstrating why runtime success alone is not an adequate oracle. - -**Surgical candidate.** Move the existing accessor partition in `src/Compiler/CodeGen/IlxGen.fs:10002-10003` below `ComputeMethodImplAttribs` at `10011-10012`. Decode the full current-method list once, then partition the remaining ordinary attributes. Keep DllImport, CompiledName, property routing, security attributes, and method emission unchanged. Do not concatenate stale partitions, duplicate decoding, blacklist attributes globally, or modify the IL writer. This is a plan, not a tested patch. - -The decoder at `IlxGen.fs:9765-9800` currently implements NoInlining, AggressiveInlining, Synchronized, and PreserveSig, including combinations and standalone PreserveSigAttribute. Preserve that contract. Do not add ignored CLR option bits, MethodCodeType, or int16-constructor decoding. Do not normalize conflicting inlining bits. Property-level misuse currently emits warning FS0842 unless promoted; do not change that policy. Active dotnet/fsharp#20235 touches the decoder's surroundings but is not an equivalent accessor fix. Keep its unrelated feature out of this change. - -**RED-first test plan.** Use `tests/FSharp.Compiler.ComponentTests/EmittedIL/MethodImplAttribute/MethodImplAttribute.fs` and the existing `withMetadataReader` helper. Compile valid sources successfully, then inspect raw MethodDef implementation flags and actual CustomAttribute handles. Require expected method rows to exist. Do not use reflection-synthesized pseudo attributes or a retained call as the oracle. - -| Scenario | Required assertion | -|---|---| -| 1. Exact issue: static getter with AggressiveInlining, static setter with NoInlining, ordinary method control | Getter flags `0x100`, setter `0x8`, no actual MethodImpl custom attributes. Method control remains `0x100`. | -| 2. Instance property with different getter/setter flags and distinct property/getter/setter marker attributes | Getter exactly `0x100`, setter exactly `0xA8`. Preserve each marker's target. Do not leak flags or markers between accessors. | -| 3. Standalone PreserveSig plus MethodImpl(NoInlining) on a getter | Flags `0x88`, neither pseudo attribute stored as a real custom attribute. Ordinary equivalent remains unchanged. | -| 4. Explicit interface implementation | Inspect concrete accessor MethodDefs, not abstract slots. Getter `0x8`, setter `0x20`, no real MethodImpl attributes. | -| 5. Extension property | Static emitted accessor `0x108`, no real MethodImpl, ordinary getter marker retained. Cover the second accessor emission path. | -| 6. Attributed concrete accessors behind a neutral `.fsi` | Getter `0x8`, setter `0xA0`, no real pseudo attributes in the library. Reuse existing paired-source helpers. | -| 7. Ordinary-method and unannotated controls | Existing eight option baselines stay unchanged. Use compact parity checks for all supported bits, ignored bits, ignored MethodCodeType, int16 overload, and an unannotated accessor. Do not turn ignored options into new features. | -| 8. Property-level target misuse | Preserve FS0842 and current default severity. Promote it explicitly only when the test intends rejection. | -| 9. Separate compilation consuming the library | Normal, explicit-interface, and signature-constrained accessor calls retain their values and behavior. This is a control, not the flags oracle. | - -Rows 1-6 are the primary issue and five meaningful RED variants. Assert exact flags and pseudo-attribute absence together. Passing controls must not conceal a missing or skipped regression. Keep fixtures compact, share metadata checks, and parameterize only genuine variations. Reuse the ordinary-option baseline coverage instead of expanding a cross-product of flags, platforms, and compiler switches. - -First record current-main RED failures caused by wrong metadata, not invalid syntax or typechecking. Make the surgical change and get those tests GREEN without changing their expectations. Do not regenerate baselines or weaken absence assertions to obtain GREEN. Run the focused MethodImpl tests and the existing `MethodImplNoInline02_fs` optimization/realsig cases. Their starting baseline passed nine and four cases respectively. - -Format only changed F# files. Invoke `fsharp-diagnostics` after compiler edits and invoke the expert-review skill on the final work. Remove noise and deduplicate production/test code through existing helpers. Leave a clean, compact test suite and concise release note. No new API, diagnostic, attribute framework, or unrelated cleanup is needed. - -Sources: [issue](https://github.com/dotnet/fsharp/issues/20288), [early partition](https://github.com/dotnet/fsharp/blob/b5c530ed6bc42937de6363e3dcc104ebb833893d/src/Compiler/CodeGen/IlxGen.fs#L9993-L10012), [existing decoder](https://github.com/dotnet/fsharp/blob/b5c530ed6bc42937de6363e3dcc104ebb833893d/src/Compiler/CodeGen/IlxGen.fs#L9765-L9800), [attribute emission and flags](https://github.com/dotnet/fsharp/blob/b5c530ed6bc42937de6363e3dcc104ebb833893d/src/Compiler/CodeGen/IlxGen.fs#L10233-L10303). - -## Analysis - -The current task is architecture, not implementation. Produce self-contained sprint instructions and leave the compiler unchanged. - -The initial worktree is clean. HEAD matches the supplied baseline: `b5c530ed6bc42937de6363e3dcc104ebb833893d`. - -The requested output directory is ignored by Git. Preserve the requested planning files explicitly when committing. - -The shared issue queue database is absent at both configured Windows paths. This backlog preserves the implementation request locally. - -Repository inspection confirms the proposed ordering defect. `ComputeMethodImplAttribs` filters both pseudo attributes and decodes only the four supplied bits. The normal and extension emission paths both consume the accessor partition later. - -The issue is open with no comments. PR #20235 is open and implements runtime async. Its larger feature is explicitly out of scope. - -The test module contains eight option baselines and one inline-keyword warning case. `MethodImplNoInline02_fs` in `EmittedIL\Misc\Misc.fs` declares the four optimization/realsig combinations. - -`withMetadataReader` owns PE-reader setup in `tests\FSharp.Test.Utilities\Compiler.fs`. Public `Fsi`, `FsSource`, `withAdditionalSourceFile`, and `withReferences` helpers cover paired libraries and separate consumers. Do not depend on later-compiled signature-test modules. - -The required .NET 11 SDK is missing locally. `dotnet --version` reports the missing SDK, and only SDKs 8, 9, and 10 are installed. No `.dotnet` or `artifacts` directory exists. The sprint includes repository-wrapper provisioning before RED tests. Compiler builds and regression execution are implementation work, not completed planning evidence. - -The repository variable `VNEXT` is `11.0.100`. The release-note sink is `docs\release-notes\.FSharp.Compiler.Service\11.0.100.md`. - -## Approach - -Use one atomic sprint for the compiler change and its tests. Keep the RED evidence, GREEN verification, compatibility controls, review, and release note in that sprint. - -The sprint includes all nine requested scenarios, exact flags, handle-resolution requirements, example syntax, named helpers, concrete commands, and independent completion criteria. It distinguishes supplied investigation counts from local evidence. - -Run a structural and coverage check of both output files. Verify the original implementation request is preserved, paths resolve, and no production files changed. Commit only the requested planning files, even though `.tools` is ignored. Do not push. - -Local plan validation passed: one sprint, nine scenarios, 20 completion criteria, required frontmatter and headings, no checkboxes, and 11 existing reference paths. Git confirmed no production or staged changes before staging the plan. The native PowerShell check replaced an unavailable Python command. No compiler build or regression result is claimed. - -Final implementation verification must inspect the sprint's RED/GREEN logs, exact metadata expectations, unchanged baselines, local review result, release note, and commit. Successful runtime calls alone are insufficient. - -## Sprint Overview - -| # | Name | Purpose | -|---|---|---| -| 1 | `01_Fix_Accessor_MethodImpl.md` | Reproduce incorrect accessor metadata, move the partition, and verify the complete fix locally. | diff --git a/.tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md b/.tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md deleted file mode 100644 index eb9ee7c2b6b..00000000000 --- a/.tools/ralph/sprints/01_Fix_Accessor_MethodImpl.md +++ /dev/null @@ -1,322 +0,0 @@ ---- ---- -# Sprint: Fix MethodImpl metadata on property accessors - -## Context - WHY this sprint exists - -Implement https://github.com/dotnet/fsharp/issues/20288 in `Q:\fsharp-worktrees\issue-877` using TDD. This file contains the complete work unit. No other sprint or backlog is required. - -Make the smallest clean, correct, complete fix. Validate locally, then commit. Do not push, open a PR, or post GitHub comments. - -At baseline `b5c530ed6bc42937de6363e3dcc104ebb833893d`, property accessor attributes bypass the existing implementation-flag decoder. `MethodImplAttribute` and standalone `PreserveSigAttribute` can enter real CustomAttribute rows instead. Ordinary methods work. - -In `src\Compiler\CodeGen\IlxGen.fs`, `GenMethodForBinding` partitions `attrsAppliedToGetterOrSetter` before calling `ComputeMethodImplAttribs`. The decoder therefore cannot see those attributes. - -The user reports 26 wrong-metadata assertions across 13 accessors, seven passing ordinary-method controls, and successful external calls. These are supplied investigation results, not tests executed by this planner. Runtime success alone cannot prove this fix. - -The issue description calls property-level misuse an error. The verified requirement takes precedence: preserve warning FS0842 at default severity. Explicit promotion can make it an error. - -PR #20235, "Enable runtime async via compiler intrinsics", also changes `IlxGen.fs`. It is not this accessor fix. Do not import its feature. - -### Starting state and prerequisites - -Planning inspected a clean worktree at the baseline above. Planning commits can now precede this sprint. Preserve existing changes and do not reset the branch. - -`global.json` requires SDK `11.0.100-rc.1.26420.103` and selects Microsoft.Testing.Platform. During planning, `dotnet --version` failed because that SDK was missing. Installed SDKs were 8, 9, and 10. No local `.dotnet` directory or `artifacts` directory existed. - -Provision the required SDK with the repository wrapper if it is still missing. Do not edit `global.json` or dependency versions to avoid setup. - -```powershell -Set-Location 'Q:\fsharp-worktrees\issue-877' -$env:BUILDING_USING_DOTNET = 'true' -dotnet --version -# Only when the required SDK is missing: -& .\eng\common\dotnet.ps1 --version -``` - -After provisioning, use the resolved SDK consistently. The commands below use `dotnet`. Substitute `& .\.dotnet\dotnet.exe` if the local SDK is not selected. Set `BUILDING_USING_DOTNET=true` in each new shell. - -Read `.github\copilot-instructions.md`, `.github\instructions\CodeGen.instructions.md`, `.github\instructions\ExpertReview.instructions.md`, `.github\instructions\ComponentTests.instructions.md`, `.github\instructions\NoBloat.instructions.md`, and `docs\representations.md` before editing. - -Files under `eng\common` are Arcade-owned. Run their setup wrapper but do not modify them. - -## Description - WHAT to implement with DETAILED guidance - -### Files to modify - -| Path relative to the repository | Change | -|---|---| -| `src\Compiler\CodeGen\IlxGen.fs` | Move the existing accessor partition below `ComputeMethodImplAttribs` in `GenMethodForBinding`. | -| `tests\FSharp.Compiler.ComponentTests\EmittedIL\MethodImplAttribute\MethodImplAttribute.fs` | Add compact raw-metadata regressions and compatibility controls. Preserve existing tests and baselines. | -| `docs\release-notes\.FSharp.Compiler.Service\11.0.100.md` | Add one concise entry under `### Fixed`, after confirming the release target. | - -No project-file change is needed when tests remain in the existing module. No public API, new diagnostic, attribute framework, or IL-writer change is needed. - -### Existing code and reusable patterns - -`ComputeMethodImplAttribs` is near lines 9765-9800 of `IlxGen.fs`. It decodes `NoInlining` (`0x8`), `Synchronized` (`0x20`), `PreserveSig` (`0x80`), and `AggressiveInlining` (`0x100`). It also consumes standalone `PreserveSigAttribute`. - -It removes both pseudo attributes from its returned ordinary-attribute list. Preserve its supported bits, combinations, ignored options, ignored `MethodCodeType`, and ignored `int16` constructor behavior. - -`GenMethodForBinding` currently has this order near lines 9993-10012: - -```fsharp -let attrs = // existing DllImport and CompiledName filtering - ... -let attrsAppliedToGetterOrSetter, attrs = - List.partition (fun (Attrib(_, _, _, _, isAppliedToGetterOrSetter, _, _)) -> isAppliedToGetterOrSetter) attrs -let sourceNameAttribs, compiledName = - ... -let hasPreserveSigImplFlag, hasSynchronizedImplFlag, hasNoInliningFlag, hasAggressiveInliningImplFlag, attrs = - ComputeMethodImplAttribs cenv v attrs -let securityAttributes, attrs = - ... -``` - -The ellipses above indicate existing code, not code to insert. Relocate only the two-line accessor partition. Place it immediately after the decoder call and before the security partition. - -This decodes the full current-method list once, after DllImport/CompiledName filtering. Then it partitions the remaining ordinary attributes. - -Leave `sourceNameAttribs`, `compiledName`, security processing, property routing, and method construction unchanged. Leave the normal accessor path near lines 10233-10265 and extension path near lines 10270-10285 unchanged. Leave `.WithPreserveSig`, `.WithSynchronized`, `.WithNoInlining`, and `.WithAggressiveInlining` near lines 10297-10303 unchanged. - -Use these existing test APIs from `FSharp.Test.Compiler` in `tests\FSharp.Test.Utilities\Compiler.fs`: - -```fsharp -FSharp source -|> asLibrary -|> compile -|> shouldSucceed -|> withMetadataReader (fun reader -> (* inspect raw metadata here *)) -``` - -`withMetadataReader`, near line 1000, already owns the PE-reader lifetime. Do not add another output-path/PE-reader wrapper. - -For paired sources, use the existing `Fsi` and `FsSource` helpers. They produce matching `test.fsi` and `test.fs` names: - -```fsharp -let library = - Fsi signatureSource - |> withAdditionalSourceFile (FsSource implementationSource) - |> asLibrary - |> withName "AccessorLibrary" -``` - -Examples exist in `tests\FSharp.Compiler.ComponentTests\Signatures\TestHelpers.fs` and `tests\FSharp.Compiler.ComponentTests\Conformance\Signatures\SignatureEnforcedAttributes.fs`. Reuse the public source helpers, not those modules' private helpers. The signature helpers module compiles after this test module. - -For a separate consumer, use `withReferences [ library ]`, `asExe`, an explicit entry point, and `compileExeAndRun |> shouldSucceed`. Pass a `CompilationUnit` to `withReferences`, not a `CompilationResult`. Keep consumer source out of the library's source list. - -### Step 1: Establish the baseline and preserve evidence - -Create an ignored evidence directory at `.tools\ralph\evidence\20288`. Save logs there, not among committed source files. Preserve the baseline hash, commands, SDK version, exit codes, test counts, and RED/GREEN diagnostics. - -Use `.tools\ralph\evidence\20288\progress.txt` for a short resume record. Include completed scenarios and the next command. Never mark a phase complete without its evidence. - -Before production edits, run the existing focused MethodImpl tests and `MethodImplNoInline02_fs`. The supplied starting results were nine and four passing cases respectively. Re-establish those counts locally. - -Build/test commands are given below. Resolve setup failures before diagnosing the regression. Missing SDKs, invalid fixture syntax, and typechecking failures do not count as RED. - -### Step 2: Add raw-metadata tests and obtain RED - -Keep additions in the existing `EmittedIL.MethodImplAttribute` module. Retain its eight option-baseline tests and existing inline-keyword warning test without changes. - -Use one shared metadata-checking path for new fixtures. Parameterize actual variations, rather than copying complete test bodies. Keep each of scenarios 1-6 independently observable in test output. A failure in scenario 1 must not prevent scenarios 2-6 from running. - -Select each expected declaring type and method by exact metadata identity. Require exactly one match, or fail with its expected identity. Do not filter discovered methods and then pass on an empty sequence. - -Read `MethodDefinition.ImplAttributes` as an integer and assert the complete expected value, not just selected bits. Read `MethodDefinition.GetCustomAttributes()` and resolve actual constructor handles through `MetadataReader.GetCustomAttribute`. - -Use `System.Reflection.Metadata`. External attribute constructors usually use `HandleKind.MemberReference` with a `TypeReference` parent. Locally declared markers can use `HandleKind.MethodDefinition` and `GetDeclaringType()`. Resolve both correctly. Handle any encountered `TypeDefinition` parent. Fail explicitly on unexpected shapes rather than returning an empty name or silently skipping a row. - -Compare the combined observation of exact flags and pseudo-attribute absence for each accessor. Include full attribute names: - -```text -System.Runtime.CompilerServices.MethodImplAttribute -System.Runtime.InteropServices.PreserveSigAttribute -``` - -No real row for either pseudo attribute is allowed on the tested concrete methods. Do not reject unrelated compiler-generated attributes. For marker checks, compare the marker subset precisely. - -Do not use reflection attribute enumeration. Reflection can synthesize pseudo attributes from implementation flags. Do not use an IL call instruction or successful execution as the flags oracle. - -#### Required scenario matrix - -| # | Fixture and construction guidance | Assertions | -|---|---|---| -| 1 | Exact issue source shown below: static getter, static setter, and ordinary static method. | `A.get_P1 = 0x100`, `B.set_P2 = 0x8`, `C.M1 = 0x100`. Both accessors and the method lack real pseudo attributes. | -| 2 | Instance property with getter `AggressiveInlining` and setter `NoInlining ||| Synchronized ||| PreserveSig`. Define three small marker attribute types, valid respectively on property, getter method, and setter method. | Getter `0x100`, setter `0xA8`, no real pseudo attributes. Read the PropertyDef and both MethodDefs. Each marker appears exactly once on its intended row and on neither other row. | -| 3 | Getter with standalone `[]` plus `[]`. Include an ordinary-method equivalent. | Both methods have `0x88`. Neither pseudo attribute has a real row. | -| 4 | Interface with `abstract P: int with get, set`, implemented explicitly by a concrete class. Attribute the concrete getter with `NoInlining`, and setter with `Synchronized`. | Concrete getter `0x8`, concrete setter `0x20`, no real pseudo attributes. Select the implementing type's MethodDefs, not interface slots. Require non-abstract methods with bodies. | -| 5 | Extrinsic extension property, such as `type System.String with ...` inside a module. Apply `NoInlining ||| AggressiveInlining` and an ordinary getter marker. | Emitted extension accessor is static, has `0x108`, retains its marker, and lacks real pseudo attributes. Do not normalize conflicting inlining flags. This must exercise the extension emission path. | -| 6 | Neutral `.fsi` declares a class constructor and `member P: int with get, set`. Matching `.fs` puts `NoInlining` on the getter and `Synchronized ||| PreserveSig` on the setter. | Concrete library getter `0x8`, setter `0xA0`, no real pseudo attributes. No attributes are needed in the signature. | -| 7 | Compact ordinary-method/accessor parity controls, described below. | Preserve current ordinary-method behavior and ensure an unannotated accessor remains `0x0`. Keep all eight existing option baselines unchanged. | -| 8 | Place `[]` before a property member rather than on its accessor. | Default diagnostic is warning FS0842. A separate, explicitly promoted case rejects it as error FS0842. | -| 9 | Separately compile an executable referencing normal, explicit-interface, and signature-constrained library fixtures. Reuse fixture source definitions. | Getters return expected values. Setters update backing values and subsequent getters observe the updates. Assert interface-dispatched calls and signature-constrained calls. This is a control only. | - -Exact issue source: - -```fsharp -module P -open System.Runtime.CompilerServices - -[] -type A = - static member P1 with [] get () = 1 - -[] -type B = - static member P2 with [] set (v: int) = ignore v - -[] -type C = - [] - static member M1() = 1 -``` - -For instance and explicit-interface fixtures, use a mutable integer backing field with a simple get/set body. Place accessor attributes after `with` or `and`, immediately before `get` or `set`. Ensure setters accept `int`. - -For scenario 5, extend an external type in a module to guarantee an extrinsic extension. A declaration such as `type System.String with member s.P with [<...>] get () = s.Length` is the intended shape. Discover the emitted qualified method name, then assert its exact identity and static flag. Do not assume an ordinary PropertyDef exists for this path. - -Use the paired-source helper pattern for scenario 6. The neutral signature can have this shape: - -```fsharp -module SignatureLibrary -type C = - new: unit -> C - member P: int with get, set -``` - -For scenario 7, use a compact data table, not a flags/platform/optimization/realsig cross-product. These constructor expressions and results describe the existing decoder: - -| Attribute argument or form | Expected implementation flags | -|---|---| -| `MethodImplOptions.NoInlining` | `0x8` | -| `MethodImplOptions.Synchronized` | `0x20` | -| `MethodImplOptions.PreserveSig` | `0x80` | -| `MethodImplOptions.AggressiveInlining` | `0x100` | -| Standalone `PreserveSigAttribute` | `0x80` | -| `MethodImplOptions.NoInlining ||| MethodImplOptions.AggressiveInlining` | `0x108` | -| Each of `ForwardRef`, `InternalCall`, `NoOptimization`, `Unmanaged`, and `AggressiveOptimization` | `0x0` | -| `MethodImplOptions.NoInlining, MethodCodeType = MethodCodeType.Native` | `0x8`, with the method still implemented as IL | -| `MethodImpl(8s)` using the `int16` constructor | `0x0` | -| No attribute | `0x0` | - -Reuse existing ordinary-option baseline coverage instead of adding equivalent text baselines. Add only the small raw checks needed for accessor parity and uncovered ordinary forms. Apply the pseudo-attribute absence check to annotated controls too. Record ordinary-control results before the compiler edit. - -For scenario 8, use `typecheck`, not compilation, because only diagnostic policy matters. Assert the exact diagnostic code and severity. Use `ignoreWarnings` only to let the harness accept the expected warning, then assert it with `withSingleDiagnostic` or equivalent exact diagnostics. - -`ignoreWarnings` changes harness acceptance; it is not an instruction to suppress FS0842. For the rejection case, use `withOptions [ "--warnaserror:842" ]` and assert an error. Do not add broad warning suppression to the valid metadata fixtures. - -Run all new scenarios against the unchanged compiler. Save metadata failures for each of scenarios 1-6. Each source must compile successfully before its metadata assertion fails. Capture actual flags and actual pseudo attributes, not merely a nonzero test exit code. - -Do not edit production code until these RED results exist. The supplied 26-assertion count describes an earlier investigation, not a requirement to duplicate its assertion structure. All six primary scenarios must be demonstrated locally. - -### Step 3: Apply the surgical change and obtain GREEN - -Move the existing partition as described above. Do not concatenate saved attribute partitions, decode twice, blacklist attributes globally, or modify `ComputeMethodImplAttribs`. - -Invoke the `fsharp-diagnostics` skill immediately after editing `IlxGen.fs`. Follow its parse-first, typecheck-second workflow: - -```powershell -& .\.github\skills\fsharp-diagnostics\scripts\get-fsharp-errors.ps1 -ParseOnly src\Compiler\CodeGen\IlxGen.fs -& .\.github\skills\fsharp-diagnostics\scripts\get-fsharp-errors.ps1 src\Compiler\CodeGen\IlxGen.fs -``` - -These checks do not replace a build. Rebuild the compiler and tests, then rerun the unchanged regression expectations. All scenarios must become GREEN. - -Keep DllImport handling, CompiledName handling, ordinary attributes, security attributes, property rows, and both accessor emission paths unchanged except for the corrected pseudo-attribute consumption. - -### Local validation commands - -Use Release for EmittedIL tests and optimization cases. `BUILDING_USING_DOTNET=true` selects the current .NET target instead of also building desktop tests on Windows. - -```powershell -$env:BUILDING_USING_DOTNET = 'true' -dotnet msbuild tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -getProperty:TargetFrameworks -dotnet build tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -c Release -v minimal -``` - -Stop on a failed build. Use `--no-build` only after a successful build of the current source. Confirm the test project uses the rebuilt local compiler, not an installed SDK compiler or stale output. - -Run these focused selections before adding tests, for RED, and after the fix as appropriate: - -```powershell -$env:BUILDING_USING_DOTNET = 'true' -dotnet test --project tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -c Release --no-build -- --filter-class "*EmittedIL.MethodImplAttribute*" -dotnet test --project tests\FSharp.Compiler.ComponentTests\FSharp.Compiler.ComponentTests.fsproj -c Release --no-build -- --filter-method "*MethodImplNoInline02_fs*" -``` - -The existing `MethodImplNoInline02_fs` test is in `tests\FSharp.Compiler.ComponentTests\EmittedIL\Misc\Misc.fs`, near line 200. Its `FileInlineData` sets both `Realsig` and `Optimize` to `BooleanOptions.Both`, yielding four cases. Preserve that matrix. - -Verify selection and nonzero test counts. If the installed runner rejects a selector, inspect its help and use the equivalent xUnit v3 filter. Do not report an empty run as success. If necessary, execute the built test DLL with the same filters after discovering its output path. - -Keep logs and exit codes for each command. Escalate to related tests only when focused results require it. A full component run is not a substitute for the specific regression evidence. - -On an actual build failure, invoke `binlog-analysis`, collect a binary log, and fix the cause. If stale bootstrap output is suspected, preserve logs first. Inspect `git clean -ndx -- artifacts` before cleaning only that generated directory with `git clean -xfd -- artifacts`, then rebuild. Do not delete source, `.tools\ralph`, or another worktree. - -### Step 4: Format, review, document, and commit - -Format only changed F# files. The explicit request overrides repository-wide formatting: - -```powershell -dotnet fantomas src\Compiler\CodeGen\IlxGen.fs tests\FSharp.Compiler.ComponentTests\EmittedIL\MethodImplAttribute\MethodImplAttribute.fs -``` - -If Fantomas is missing, restore the existing local tools with `dotnet tool restore`, then rerun. Do not install a new formatter or change tool versions. - -Inspect the formatting diff and remove unrelated churn. Re-run compiler diagnostics if formatting changes compiler source. Rebuild and rerun focused tests after the final source changes. - -Invoke the `reviewing-compiler-prs` skill and its `expert-reviewer` agent on the final implementation diff. This is the available expert-review workflow. Give it the issue contract, baseline hash, exact changed files, and RED/GREEN evidence. Review locally only. Do not let a reviewer post, push, or open a PR. - -Require checks of exact flags, actual pseudo-attribute absence, non-vacuous method selection, both emission paths, marker routing, and compatibility controls. Resolve concrete findings and rerun affected checks. Invoke `code-compaction` if the test diff becomes repetitive, overengineered, or exceeds its size trigger. Do not add test-only frameworks or unrelated cleanup. - -Invoke the `release-notes` skill. `VNEXT` was `11.0.100` during planning. Confirm with `gh api repos/dotnet/fsharp/actions/variables/VNEXT --jq .value`. Use the compiler-service sink, not FSharp.Core or a new language-feature note. - -Use the insertion helper rather than prepending: - -```powershell -dotnet fsi .github\skills\release-notes\pick-insert-line.fsx --file docs\release-notes\.FSharp.Compiler.Service\11.0.100.md --section Fixed -``` - -One suitable entry is: - -```markdown -* Fix `MethodImpl` and `PreserveSig` attributes on property accessors to emit method implementation flags instead of real custom attributes. ([Issue #20288](https://github.com/dotnet/fsharp/issues/20288)) -``` - -Use the issue link because this is commit-only work. Do not fabricate a PR number or open a PR to obtain one. - -Finish with `git diff --check` and inspect the complete diff. Existing `.bsl` and `.il.bsl` files must be unchanged. Do not set `TEST_UPDATE_BSL` or regenerate baselines to obtain GREEN. - -Stage only the implementation, regression tests, and release note. Do not commit evidence logs, generated binaries, temporary source files, or changes owned by another task. Existing planning files can remain tracked but must not enter the production diff as new implementation changes. - -Commit with a descriptive message, for example `Fix MethodImpl flags on property accessors`. Include these trailers, substituting the implementing agent's actual session ID: - -```text -Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> -Copilot-Session: -``` - -Record the resulting commit hash and final verification outcomes in the ignored progress record. Leave no uncommitted changes from this sprint. Do not push. - -## Definition of Done - independently verifiable criteria - -- The existing focused suite passes nine cases before new tests, and the existing optimization/realsig selection passes four cases. -- Before production edits, scenarios 1-6 compile successfully and each exposes incorrect raw metadata in preserved RED logs. -- Every expected type and concrete accessor is required to exist exactly once; no regression can pass through an empty selection. -- Scenarios 1-6 assert the exact flags from the matrix and absence of actual MethodImpl/PreserveSig custom attributes together. -- Scenario 2 verifies property/getter/setter marker ownership, including absence from the other two targets. -- Scenario 4 checks concrete explicit-interface methods with bodies, not abstract slots. -- Scenario 5 checks the static extension accessor, retains its marker, and preserves the combined `0x108` flags. -- Scenario 6 verifies the emitted library behind a neutral signature using paired-source helpers. -- Scenario 7 preserves supported bits, standalone PreserveSig, ignored bits, ignored MethodCodeType, ignored int16 decoding, and unannotated-accessor behavior. -- Scenario 8 preserves warning FS0842 by default and rejects only the explicitly promoted test case. -- Scenario 9 separately compiles and executes normal, explicit-interface, and signature-constrained getter/setter calls with value assertions. -- The production diff only relocates the existing accessor partition after the decoder; unrelated routing and emission logic remain unchanged. -- Compiler diagnostics and the Release build succeed with no new warnings after the final source edit. -- All focused MethodImpl tests and all four `MethodImplNoInline02_fs` cases pass locally with unchanged expectations and no skipped regressions. -- Existing option baselines are unchanged, and no baseline regeneration was used. -- Only changed F# files are formatted, and `git diff --check` succeeds. -- Local expert review is complete, concrete findings are resolved, and affected checks were rerun. -- A concise compiler-service release note links to issue #20288. -- Evidence and resume state persist under `.tools\ralph\evidence\20288`, while temporary implementation artifacts are removed. -- The implementation, tests, and release note are committed with the required trailers; no sprint-owned changes remain uncommitted, and nothing was pushed.