Add the F# hot reload session and FCS service surface#20030
Open
NatElkins wants to merge 39 commits into
Open
Conversation
Contributor
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
Warning No PR link found in some release notes, please consider adding it.
|
NatElkins
force-pushed
the
hotreload-session
branch
3 times, most recently
from
July 9, 2026 21:44
c96c141 to
884759d
Compare
…ethod CDI emission Adds an internal AbstractIL module implementing, byte for byte, the three Portable PDB CustomDebugInformation blob formats Roslyn persists per method for Edit and Continue (EnC Local Slot Map, EnC Lambda and Closure Map, EnC State Machine State Map), with serializers, deserializers, a portable PDB read-back helper, and an occurrence-key packing helper for deterministic syntax-offset slots. Plumbs an optional methodCustomDebugInfoRows side channel through the IL binary writer options into the portable PDB generator so a compilation can attach CDI rows to named methods. Names that do not identify exactly one method row are dropped. All existing writer call sites pass an empty map, so emitted PDBs are byte-identical to before. No in-tree caller populates the map yet; the consumer is the F# hot reload work in dotnet#19941, following the same pattern as dotnet#20017 (land isolated, test-covered infrastructure first, wire the feature later). Tests: blob round-trips, Roslyn golden-byte encodings, cross-validation against CDI blobs emitted by a real Roslyn compilation, fail-closed occurrence-key packing (including an int32-overflow regression where a wrapped negative key previously escaped the bound check), and end-to-end synthetic PDB emission proving correct MethodDef parenting, zero rows for an empty map, and no rows for absent or ambiguous names.
ProcessStartInfo.ArgumentList does not exist on net472, which the component tests also target on Windows CI. Build the quoted argument string by hand instead.
xunit 3.2.2 no longer discovers internal test classes, so the module was silently skipped after rebasing onto current main (pre-existing internal test modules like CompilerService.Caches are likewise undiscovered there). Public visibility restores discovery; 17 tests run and pass.
Add internal generated-name normalization and synthesized-name map replay support as a standalone slice. The new map state is side-channel based, all new compiler modules remain internal, and CompilerGlobalState preserves the existing no-map counter path while checking an accessor captured once per compiler state. Route existing IlxGen generated-name allocations through inert helper wrappers, add pure name-map and normalizer tests, add a normal compilation determinism guard over emitted generated names, and document the extracted seams in P5_REPORT.md. Verification: built FSharp.Compiler.Service, FSharp.Compiler.Service.Tests, FSharp.Compiler.ComponentTests, and FSharpSuite.Tests in Release; ran the migrated service test classes, the component determinism class, FSharpSuite DeterministicTests, and the FCS SurfaceArea class successfully.
Adds an internal, standalone ECMA-335 Edit-and-Continue metadata delta writer to AbstractIL: delta #- table stream and heap construction (DeltaMetadataTables, DeltaMetadataSerializer, DeltaTableLayout, DeltaIndexSizing), ECMA-335 II.24.2.6 coded-index encoding (DeltaMetadataEncoding), EncLog/EncMap emission, generation GUID chaining, user-string and standalone-signature token calculators (IlxDeltaStreams), and the coordinating writer (FSharpDeltaMetadataWriter) over a plain row-description input model (DeltaMetadataTypes, ILDeltaHandles, ILMetadataHeaps). The writer's inputs are row records (names, tokens, signatures, RVAs) plus heap offsets; it has no dependency on any semantic diffing or session machinery. It compiles with no in-tree consumer by design: the consumer is the F# hot reload work in dotnet#19941, following the same upstreaming pattern as dotnet#20017 and dotnet#20018 (land isolated, test-covered infrastructure first, wire the feature in a later PR). One line of ilwrite.fsi is touched to expose the pre-existing markerForUnicodeBytes so the delta writer reuses the exact string-marker logic of the full writer. No behavior change for any existing code path. Tests (130): coded-index encodings asserted against the production definitions and ECMA-335 II.24.2.6 order, System.Reflection.Metadata reader parity over emitted deltas, EncLog/EncMap correctness, stream layout, heap and index sizing, multi-generation heap-offset chaining asserted against computed expected values, standalone-signature rows asserted at baseline+1 from a real seeded baseline, and serializer failure paths.
…CI images The Roslyn cross-validation test shells out to dotnet build to produce a real Roslyn PDB. The process launch had two problems. It computed the host path by hand from __SOURCE_DIRECTORY__, which misses on CI images that carry no repo-local .dotnet at that depth, failing with Win32Exception before the build starts. It also left UseShellExecute at its default, which is true on net472 and rejects redirected streams, so every Desktop test leg failed deterministically with InvalidOperationException. Resolve the host like the rest of the test framework via TestFramework.initialConfig.DotNetExe, which prefers the repo-local .dotnet and falls back to PATH, and set UseShellExecute to false explicitly. Verified: FSharp.Compiler.ComponentTests builds clean; EncMethodDebugInformationTests 17 passed, 0 failed (net10.0); fantomas clean on the touched file.
Add an internal TypedTreeDiff module that snapshots CheckedImplFile bindings and entities, then classifies body, signature, inline, declaration add/remove, and type layout changes without depending on hot reload sessions, runtime capability negotiation, EnC capability names, baseline state, or delta emission. Add focused FCS tests for unchanged/reference-equal files, body edits, signature edits, additions, deletions, layout changes, and logical-name arity handling. Wire the module and tests into compile order, add a release note, and include P6_REPORT.md. Verification: - ./.dotnet/dotnet build src/Compiler/FSharp.Compiler.Service.fsproj -c Debug /p:BUILDING_USING_DOTNET=true - ./.dotnet/dotnet test tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj -c Debug /p:BUILDING_USING_DOTNET=true -- --filter-class "*TypedTreeDiffTests*" - ./.dotnet/dotnet test tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj -c Debug /p:BUILDING_USING_DOTNET=true -- --filter-class "*SurfaceAreaTest*"
…0260717/hotreload-baseline-reader
Add standalone baseline PE and portable PDB readers for hot reload, including token maps and MVID/PDB table snapshots. Carry the F# synthesized-name snapshot module CDI codec and portable PDB read path, with recorded snapshots taking precedence over IL reconstruction. Add focused component tests for snapshot round-trip, direct module CDI reading, baseline token maps, recorded fallback behavior, and EnC closure-name reconstruction.
…erload CI compiles the snapshot round-trip and baseline reader tests with FS0193: the explicit Assert.Equal<string> type application commits overload resolution to the scalar Equal(T, T) shape while both arguments are string arrays. Use Assert.Equal<string[]>, the form the neighboring name-map tests already use, so resolution lands on the structural array comparison everywhere. Verified: FSharp.Compiler.ComponentTests builds clean; EncMethodDebugInformationTests 22 passed, 0 failed (net10.0); fantomas clean on the touched file.
…/20260717/hotreload-delta-emitter
…h/20260717/hotreload-delta-emitter
Add the internal hot reload delta emitter, symbol matcher, and direct emitter tests. Keep session and service integration deferred.
…cation This slice carries the full typed-tree differ, which treats a parameter-type change as a removal plus an insert of distinct symbols rather than one matched symbol with a changed signature. The removal surfaces as the rude edit (DeclarationRemoved) and the replacement as an insert semantic edit, so the earlier expectations (no semantic edits, SignatureChange) no longer describe this tree. Assert the observable contract instead: exactly one rude edit with the removal classification. Hosts reject the whole update when any rude edit is present, so the accompanying insert never applies. Verified: TypedTreeDiffTests 8 passed 0 failed, fantomas clean on the touched file.
On CI the test projects fail to compile in shapes the local bootstrap accepts: Assert.Equal<string> over string arrays commits overload resolution to the scalar Equal shape (FS0193), and Assert.Equal<int[]> over int arrays commits to a Span overload carrying an unmanaged constraint (FS0001). Switch to the instantiations the rest of the suite already uses, Assert.Equal<string[]> and Assert.Equal<int list>, which resolve the same way under both compilers. HotReloadCheckerTests loads baseline assemblies through AssemblyLoadContext, which .NET Framework does not have, and FSharp.Compiler.Service.Tests also compiles for net472 on the Windows Desktop CI legs. Gate the file to .NET Core in the project file, the same way the SurfaceArea test is gated. The EnC CDI cross-validation test resolved the dotnet host by hand from __SOURCE_DIRECTORY__, which misses on CI images that carry no repo-local .dotnet at that depth. Resolve it through TestFramework.initialConfig.DotNetExe like the rest of the framework. Also repair two extraction oversights: the move of the EnC method debug information tests to the HotReload namespace left the old CompilerService copy in the tree uncompiled, so delete it, and the ComponentTests project lost the EmittedIL CompilerGeneratedNameDeterminism include during the hot reload test group reorganization, so restore it. Verified: service HotReload 413 passed 0 failed, component HotReload 236 passed 0 failed 2 skipped, EncMethodDebugInformationTests 14 passed, CompilerGeneratedNameDeterminism 1 passed, SurfaceAreaTest green with no baseline change, fantomas clean on touched files.
Three test-layer breaks surfaced by the first full CI run of this slice. String.Contains(string, StringComparison) does not exist on net472 and FSharp.Compiler.Service.Tests also compiles for net472 on the Windows Desktop legs. All twelve uses passed StringComparison.Ordinal, which is already the semantic of the always-available Contains(string) overload, so drop the argument. RoslynBaselineComparisons resolved tools/baselines/roslyn_tables.json with four parent hops from the source directory, one too many, which escapes the repository. It passed locally only because the checkout happens to sit inside a directory that carries an identical copy at that spot; CI agents have nothing there. Three hops is the repo root. The synthetic baseline build in TestHelpers computed the dotnet host path by hand from __SOURCE_DIRECTORY__, which misses on CI images that carry no repo-local .dotnet, failing ApplyUpdate tests on macOS agents before the build starts. Resolve it through TestFramework.initialConfig.DotNetExe like the rest of the framework. Verified: service HotReload 413 passed 0 failed, component HotReload 236 passed 0 failed 2 skipped, fantomas clean on touched files.
Same net472 break as the session tests fixed in the previous commit, in the nine remaining String.Contains(value, StringComparison.Ordinal) sites in DeltaBuilderTests. The Desktop compile stopped at the session tests first, which masked these. Contains(string) is ordinal, so the argument is redundant. Verified: service HotReload 413 passed 0 failed, fantomas clean.
RoslynBaselineComparisons parses its baseline table with System.Text.Json, which .NET Framework does not have, and FSharp.Compiler.Service.Tests compiles for net472 on every Windows CI job because the solution build covers both target frameworks. Gate the file to .NET Core in the project file like the SurfaceArea and checker tests. The nine comparisons keep running on all net10.0 legs. Verified: net10.0 build clean, RoslynBaselineComparisons 9 passed.
The synthesized-alignment and state-machine-shape tests build their updated sources by String.Replace over triple-quoted baseline literals. Triple-quoted literals inherit the source file's line endings, and the repo does not pin an eol for .fs files, so Windows checkouts hand these baselines CRLF while the replace needles use plain newline escapes. The needle then never matches and Replace silently returns the baseline unchanged, so the tests compile identical source twice: the alignment tests fail with NoChanges and the rejection tests invert because there is nothing left to reject. This broke six tests on the first Windows CI run that reached test execution; the compiler itself is unaffected since names and line shifts key off sequence-point line numbers. Normalize the affected baselines to plain newlines before any splicing so the fixture surgery is line-ending-agnostic on any host. Verified: component HotReload 236 passed 0 failed 2 skipped, fantomas clean on the touched file.
NatElkins
force-pushed
the
hotreload-session
branch
from
July 17, 2026 23:11
884759d to
b26df0f
Compare
Verified the repository-wide Fantomas check, the service test project build with zero warnings and errors, and all 134 DeltaMetadata tests.
Carries lower-slice formatting into the session branch and verifies the complete repository with Fantomas.
NatElkins
marked this pull request as ready for review
July 18, 2026 02:19
github-actions
Bot
added
the
⚠️ Affects-Compiler-Output
Contributor
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
T-Gro
reviewed
Jul 21, 2026
T-Gro
left a comment
T-Gro
left a comment
Member
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
Reviewed the hot-reload session slice's own source changes (the new src/Compiler/HotReload/* files, HotReloadEmitHook.fs, and the service.fs plumbing). State store locking/atomicity, generation arithmetic, rude-edit code assignment, and tracked-input parsing all checked out. Three fail-closed / graceful-degradation gaps are noted inline. The bulk of the diff (dependency-PR merges) was not re-reviewed here.
| "[fsharp-hotreload][closure-names] capture validation FAILED: derived tables disagree with the emit-time recording: %A" | ||
| mismatches | ||
|
|
||
| System.Diagnostics.Debug.Assert( |
Member
There was a problem hiding this comment.
Fail-open in Release builds. When the CDI-derived closure-name tables (baseline.EncClosureNames) disagree with the emit-time stamp→name recording (recordedRows), the only reaction is System.Diagnostics.Debug.Assert(false, ...). Debug.Assert is compiled out of Release builds, so in a Release compiler the mismatch is silently ignored and baseline is persisted at line 98 with the derived names — even though the emitted assembly's closures actually carry the recorded names. A later delta built against this baseline then references the wrong closure identities: a corrupt/partial delta is produced instead of failing the capture, which violates the PR's stated "rude edits always fail closed" contract for exactly the invariant this code declares must hold.
Fix: on a non-empty mismatches, fail the capture hard (e.g. error(...) / raise) so the invariant is enforced in Release as well as Debug, and no baseline with inconsistent closure names is ever handed to StartSession.
| if File.Exists(pdbPath) then | ||
| try | ||
| Some(File.ReadAllBytes(pdbPath)) | ||
| with :? IOException -> |
Member
There was a problem hiding this comment.
Handler too narrow for the promised graceful degradation. The comment above (lines 409–413) states that "Missing/unreadable PDBs degrade gracefully (analysis stays inert, fail closed)." But this only catches IOException. File.ReadAllBytes also throws UnauthorizedAccessException (ACL / read-only file or directory), and can throw SecurityException / NotSupportedException. Those escape the try, propagate out of the async, and fault the entire EmitDelta operation instead of degrading to None — so a permission-restricted PDB crashes the emit rather than falling back to no-debug-info emission.
Fix: broaden the handler to cover the graceful-degradation cases, e.g. with :? IOException | :? UnauthorizedAccessException -> None (optionally also SecurityException).
|
|
||
| let currentSize = if exists then FileInfo(path).Length else -1L | ||
|
|
||
| if currentWrite = lastWrite && currentSize = lastSize then |
Member
There was a problem hiding this comment.
A not-yet-existent file is treated as immediately "stable". The loop seeds lastWrite = DateTime.MinValue / lastSize = -1L (lines 290–291), which are the exact values produced for a missing file (lines 297–303). So when the target output assembly does not exist yet, iteration 1's currentWrite = lastWrite && currentSize = lastSize is already true, stableCount reaches StableFileRequiredStableReads after ~25ms, and the function returns declaring the file stable. The caller readIlModule then immediately does OpenILModuleReader and throws. This defeats the stated intent (the comment at line 286 says it should tolerate slow-I/O up to ~5s): if a freshly-built output assembly lags the read request — a realistic out-of-process hot-reload timing race — the loop does not wait for it to materialize.
Fix: only count a read as stable when the file exists, e.g. require exists && currentWrite = lastWrite && currentSize = lastSize, or seed lastWrite/lastSize with sentinels that a missing file can never match.
T-Gro
self-requested a review
July 21, 2026 11:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the F# hot reload session and its FCS surface:
FSharpChecker.CreateHotReloadSession, withAddProject(baseline capture from the on-disk assembly and PDB),EmitDelta(metadata/IL/PDB deltas through the #20027 emitter),Commit/Discardgeneration chaining, runtime capability negotiation, rude-edit diagnostics (FSharpHotReloadError.UnsupportedEditwith FSHRDL codes), and active-statement tracking and remapping.Also adds the write side that #20026 reads: under
--test:HotReloadDeltas, baseline compiles persist EnC closure-name method CDI and the recorded synthesized-name snapshot module CDI (the complete post-IlxGen name map in allocation order), plus the occurrence-keyed closure-name allocation and replay that keeps synthesized names stable across session compiles.Rude edits always fail closed: anything the diff, matcher, or emitter cannot prove safe surfaces as a rude edit for the host (dotnet-watch rebuilds and restarts). The flag-gated in-process compile perf path is not in this PR; it comes last in the train as an explicitly experimental slice. Without it, sessions work with the ordinary external build flow.
This is the slice that changes the public surface area (the session API); the baseline is updated accordingly.
Tests: this slice owns the end-to-end suites. Service HotReload: 329 passed, 0 failed (session lifecycle, disk-started topologies, naming replay incl. mixed recorded-snapshot buckets, name-equality assertions). Component HotReload: 241 passed, 0 failed, 2 expected skips (runtime ApplyUpdate paths incl. task class state machines, PDB CDI emission, baseline determinism, mdv validation, fail-closed contracts).
SurfaceAreaTestgreen.Sequencing
This PR is part of splitting the F# hot reload work (#19941) into small, independently reviewable PRs. The planned order:
--test:HotReloadDeltascapture hook.The dotnet-watch integration consuming this API is dotnet/sdk#55128.
Refresh status (2026-07-17)