Add the hot reload delta emitter and symbol matcher#20027
Conversation
❗ 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.
|
6767473 to
55ca844
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.
55ca844 to
9112dab
Compare
Verified the repository-wide Fantomas check, the service test project build with zero warnings and errors, and all 134 DeltaMetadata tests.
Verified with the repository-wide Fantomas check.
Verified with the repository-wide Fantomas check.
Carries the typed-tree formatting through the capability-aware emitter and verifies the complete repository with Fantomas.
github-actions
Bot
added
the
⚠️ Affects-Compiler-Output
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
T-Gro
left a comment
T-Gro
left a comment
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 core correctness surfaces of this large (34k+ line) hot reload delta slice: the delta emitter (IlxDeltaEmitter.fs), the raw baseline metadata reader (ILBaselineReader.fs), the coded-index sizing/encoding, the symbol matcher, and the shared IlxGen state-machine changes. The shared codegen path is correctly gated to be byte-identical no-ops when hot reload is inactive, the coded-index tag tables match ECMA-335 §II.24.2.6, and the diff classification is consistently fail-closed. The findings below concentrate in the delta serializer and the metadata reader. Given the size, I focused on the emitter, reader, and matcher; the PDB delta writer and the bulk of TypedTreeDiff.fs/HotReloadBaseline.fs had a lighter pass.
| |> Option.map (fun changes -> not (List.isEmpty changes.Added)) | ||
| |> Option.defaultValue false) | ||
|
|
||
| if not editDeclaresDefinitions then |
There was a problem hiding this comment.
Potential corruption on "no-edit" emits (High). When editDeclaresDefinitions is false (the sequence-point-only / generative re-emit case this block exists to neutralize), the cleanup clears the added member collections but not the added type collections that addAddedTypeDefinition already populated during collectTypeMappings (line 4564): addedTypeDeltaTokens, addedTypeDefs, addedTypeNewTokens, addedTypeShapes, addedTypeKeyByFreshFullName, addedTypeMetadataNameByKey.
Those surviving registrations still materialize downstream: ... || addedTypeDeltaTokens.Count > 0 keeps delta emission alive, baselineTokens @ (addedTypeDeltaTokens.Values ...) injects spurious TypeDef tokens, and the if addedTypeDefs.Count = 0 then ... else blocks emit new TypeDef rows + EncLog/EncMap entries. So a semantically-empty emit can still add TypeDef rows and advance the generation — the very corruption this gate was written to prevent (it only handles members).
Fix: in the not editDeclaresDefinitions branch also Clear() the added-type collections (and drop the fresh→delta TypeDef token mappings), or defer added-TypeDef allocation until after this gate.
| if traceMethodUpdates.Value then | ||
| for row in constantRowsSnapshot do | ||
| printfn | ||
| "[fsharp-hotreload][constant-add] rowId=%d parentFieldRow=%d typeCod |
There was a problem hiding this comment.
Duplicate TypeRef/MemberRef RIDs from desynced counters (High). buildCustomAttributeRows receives nextTypeRefRowId/nextMemberRefRowId by value (6022–6023) and keeps local mutable copies (1846–1847), allocating synthesized attribute TypeRef/MemberRef rows from those locals while appending to the shared typeReferenceRows/memberReferenceRows arrays. But buildAttributeRow also calls remapEntityToken for each attribute ctor, which appends to the same arrays using the driver-level counter. The two counters start equal but advance independently within the call, so a remapEntityToken-appended MemberRef and a synthesized-ctor MemberRef can both get RID N+1 → duplicate MemberRef row in the delta (corrupt EnC metadata).
Compounding it, 6027–6028 overwrite the driver counters with the local final values, discarding any advances remapEntityToken made during the call (RIDs double-allocated on the next append).
Fix: route all TypeRef/MemberRef additions in this call through a single shared allocator (pass the driver's Get/SetNext*RowId accessors) rather than an independent by-value copy.
|
|
||
| match shapeMatches with | ||
| | [| single |] -> Some single | ||
| | matches when usesRecordedSynthesizedSnapshot && matches.Length > 0 -> Some matches[0] |
There was a problem hiding this comment.
Match ladder can pick an arbitrary / shape-incompatible baseline row (Medium). The first arm [| single |] -> Some single is correctly fail-closed, but the two recorded-snapshot fallbacks are not: matches when ... matches.Length > 0 -> Some matches[0] picks [0] from multiple shape-compatible candidates, and [||] when ... availableMatches.Length > 0 -> Some availableMatches[0] fires when no shape matched at all, mapping the fresh synthesized type onto an arbitrary baseline TypeDef of differing shape. That contradicts the PR's "prefer a rude edit over a wrong mapping" invariant. If the recorded snapshot is meant to guarantee a unique positional identity here, that should be asserted.
Fix: restrict both fallbacks to availableMatches.Length = 1 (or gate on a positional-identity check) and otherwise fail closed / add a new row.
| else | ||
| metadataReader.GetString row.Name | ||
|
|
||
| match context.TryReuseBaselineAssemblyRef name with |
There was a problem hiding this comment.
AssemblyRef identity matched by simple name only (Medium). remapAssemblyRefToken reuses a baseline AssemblyRef via TryReuseBaselineAssemblyRef name using only the simple name, and tryTypeRefKey keys the scope as TypeReferenceScope.Assembly(assemblyRef.Name). ECMA-335 assembly identity also includes version/culture/public-key-token. If the module references two assemblies with the same simple name but different versions (or baseline/fresh differ in version), a TypeRef can be remapped onto the wrong baseline AssemblyRef row. Likelihood is low for typical single-target F# compiles, hence Medium.
Fix: key AssemblyRef reuse on the full identity (name+version+culture+PKT); fail closed when a legacy baseline lacks the data to disambiguate.
| if rowCounts.[i] > 0 then | ||
| rowCountSize <- rowCountSize + 4 | ||
|
|
||
| let tablesStart = tablesOffset + 24 + rowCountSize |
There was a problem hiding this comment.
Table-data start offset diverges from ECMA-335 and fails silently (High). parseTablesStream advances its cursor once per Valid bit set (rowCountOffset <- rowCountOffset + 4 inside if (valid &&& (1L <<< i)) <> 0L), i.e. the Rows array has exactly one 4-byte entry per bit in Valid (per SRM MetadataReader, which sizes it by popcount of Valid). But here rowCountSize is computed by counting only tables with rowCounts.[i] > 0. A table whose Valid bit is set with a row count of 0 is legal (and most plausible on the uncompressed #- stream this path explicitly supports, lines 646–650); for each such table tablesStart is 4 bytes too small, shifting every subsequent table offset and reading TypeDef/MethodDef/Param/etc. rows from the wrong location — a silently wrong token map rather than a rude edit/restart.
The correct offset is already computed inside parseTablesStream (the post-loop rowCountOffset) but is discarded in favor of returning the header offset.
Fix: return/use the post-loop rowCountOffset as tablesStart, or size rowCountSize by the popcount of valid rather than rowCounts.[i] > 0. Apply the same fix to the duplicated block in the PDB/MVID reader (parsePdbTablesStream).
|
|
||
| /// Read a little-endian 64-bit integer from bytes at offset. | ||
| let private readInt64 (bytes: byte[]) (offset: int) = | ||
| int64 (readInt32 bytes offset) ||| (int64 (readInt32 bytes (offset + 4)) <<< 32) |
There was a problem hiding this comment.
readInt64 sign-extends the low dword, corrupting the high half of the Valid/Sorted mask (Low, latent). int64 (readInt32 bytes offset) sign-extends when the low dword has bit 31 set, forcing bits 32–63 on before the |||. This primitive reads the 64-bit Valid bitmask; if the low-32 valid bits ever include bit 31 it would spuriously mark all tables 32–63 (Assembly, AssemblyRef, File, GenericParam, MethodSpec, …) as present. Bit 31 corresponds to an unused table slot in a normal gen-0 baseline so it isn't triggered today, but the primitive is silently incorrect.
Fix: mask before widening, e.g. (int64 (readInt32 bytes offset) &&& 0xFFFFFFFFL) ||| ((int64 (readInt32 bytes (offset + 4)) &&& 0xFFFFFFFFL) <<< 32), or read via BitConverter.ToUInt64.
2 participants
Adds the F# hot reload delta emitter and symbol matcher: given a baseline (#20026), a fresh compile's module, and a typed-tree diff (#20025), this layer produces the three Edit and Continue delta blobs (metadata delta with EncLog/EncMap through the #20019 writer, IL delta, PDB delta).
The synthesized-type match ladder is the core correctness surface: exact matching via replayed generation-0 names first (#20024 makes those byte-stable), then verified alias candidates, then a guarded positional-pairing recovery prepass (shape-verified bijection over line-normalized names), and fail-closed ambiguity errors for anything the ladder cannot prove; a rude edit and restart is always preferred over a wrong mapping. Task resumable-helper chains are reconciled by shape so an inserted
let!/do!updates the right baseline helper rows and appends genuinely new await machinery instead of corrupting existing rows.Everything is internal (
SurfaceAreaTestgreen) and nothing on main calls it yet; the session slice wires it up. Session, capture hook, and the public active-statement surface are deferred there.Tests: 30 direct emitter tests that build baselines through the #20026 layer, compile edited variants, and assert delta shape: EncLog/EncMap row kinds, updated method tokens, user-string heap content, exact and positional-pairing synthesized-type mappings, and fail-closed outcomes for ambiguous mixed buckets.
Sequencing
This PR is part of splitting the F# hot reload work (#19941) into small, independently reviewable PRs. The planned order:
--test:HotReloadDeltascapture hook (F# hot reload: Edit-and-Continue delta emission behind --test:HotReloadDeltas #19941 in its final, much smaller form).Refresh status (2026-07-17)