Skip to content

Comments

Hot reload#1

Open
NatElkins wants to merge 404 commits intomasterfrom
hot-reload
Open

Hot reload#1
NatElkins wants to merge 404 commits intomasterfrom
hot-reload

Conversation

@NatElkins
Copy link
Owner

@NatElkins NatElkins commented Nov 26, 2025

F# Hot Reload (Edit-and-Continue) Implementation

This PR introduces hot reload infrastructure for F# on top of .NET's MetadataUpdater.ApplyUpdate pipeline, with Roslyn-parity-oriented metadata behavior and multi-generation delta chaining.

Status Snapshot (2026-02-07)

  • PR: NatElkins/fsharp#1
  • Branch head: hot-reload@c454aeb
  • Scope has evolved significantly since the original PR text; this description reflects the current state.

Verified in this branch

  • ./.dotnet/dotnet build FSharp.sln -c Debug -v minimal
  • ./.dotnet/dotnet test tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj -c Debug --no-build --filter FullyQualifiedName~HotReload -v minimal ✅ (258 passed)
  • ./.dotnet/dotnet test tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj -c Debug --no-build --filter FullyQualifiedName~HotReload -v minimal ✅ (101 passed)

What This PR Adds

  • Hot reload session lifecycle in FSharpChecker:
    • StartHotReloadSession
    • EmitHotReloadDelta
    • EndHotReloadSession
    • HotReloadSessionActive
    • HotReloadCapabilities
  • Typed-tree diff + rude-edit classification pipeline for deciding delta vs rebuild.
  • Baseline capture (FSharpEmitBaseline) and generation chaining (EncId/EncBaseId) across multiple edits.
  • Delta emitters for metadata/IL/PDB with EncLog/EncMap tracking.
  • Runtime integration path exercised by component tests and demo tooling.
  • In-repo demo app (tests/projects/HotReloadDemo/HotReloadDemoApp) and smoke script (tests/scripts/hot-reload-demo-smoke.sh).

Recent Architecture/Parity Direction (late 2025)

  • Significant SRM boundary reduction/refactoring in hot reload paths:
    • byte-based baseline metadata readers
    • unified handle/index abstractions (BinaryConstants/delta token helpers)
    • PDB parity tests and shared reader/writer utilities
  • Roslyn parity fixes for StandAloneSig row handling and metadata sizing/encoding behaviors.
  • Method additions enabled with Roslyn-parity restrictions (c454aeb).

Supported vs Unsupported (Current)

Supported

  • Method-body edits with metadata/IL/PDB delta emission.
  • Multi-generation sessions.
  • Runtime apply scenarios validated by component tests (when runtime prerequisites are set).
  • Limited method additions under emitter restrictions (no incompatible type-layout changes).

Still rejected / rebuild required

  • Structural edits that change type layout/signature semantics beyond supported boundaries.
  • Edits that introduce unsupported metadata/token mapping cases (reported as UnsupportedEdit/rude edits).

Runtime Apply Capability Model

  • Capability flag is intentionally conservative:
    • RuntimeApply is only advertised when the runtime supports it and FSHARP_HOTRELOAD_ENABLE_RUNTIME_APPLY=1 is set.
  • Demo smoke supports both modes:
    • default: emit + validate deltas without runtime apply
    • opt-in: runtime apply path via HOTRELOAD_SMOKE_RUNTIME_APPLY=1

Remaining Follow-ups (Not Claimed Done Here)

  • Broaden structural edit coverage (especially synthesized/complex scenarios) while preserving Roslyn parity.
  • Improve telemetry/diagnostics surfacing for hosts.
  • Wire official dotnet watch integration path.
  • Continue tightening mdv + runtime parity checks as the final acceptance bar.

Key Files

  • Service/session API: src/Compiler/Service/service.fs, src/Compiler/HotReload/EditAndContinueLanguageService.fs
  • Delta emission: src/Compiler/CodeGen/IlxDeltaEmitter.fs, src/Compiler/CodeGen/FSharpDeltaMetadataWriter.fs, src/Compiler/CodeGen/DeltaMetadataSerializer.fs
  • Baseline/capabilities: src/Compiler/CodeGen/HotReloadBaseline.fs, src/Compiler/HotReload/HotReloadCapabilities.fs
  • Tests:
    • tests/FSharp.Compiler.Service.Tests/HotReload/*
    • tests/FSharp.Compiler.ComponentTests/HotReload/*

This reverts commit 17a04f0ecc06a2e0be7c1cfee6e14c6faa7f1d70.
@T-Gro
Copy link

T-Gro commented Feb 20, 2026

Very nice stuff @NatElkins !
My first wave of feedback goes into proper layering/architecture.
My perspective is that we should try as hard as possible to avoid any "infection" of existing compiler path, and abstract away HR as much as we can from existing core compiler files.

That will relief the review burden when it comes to product safety, and we can then more focus on HR itself in next phases of the review 👍 .

Hot reload should be a plugin, not woven into the compiler

  • Goal: core compiler files should have zero hot reload imports, hot reload should plug in via abstractions
  • Roslyn reference: Compilation.Emit() has zero EnC awareness, delta emission is a separate API and separate writer class
  • Currently in this PR, three core files import hot reload directly:
    • IlxGen.fs imports SynthesizedTypeMaps
      • hotReloadIlxName at 8 call sites checks SynthesizedTypeMaps, falls back to FreshCompilerGeneratedName
      • But NiceNameGenerator.FreshCompilerGeneratedNameOfBasicName in CompilerGlobalState.fs already does the exact same dispatch via the injected getSynthesizedMap closure
      • hotReloadIlxName is redundant — call sites should just call the name generator directly
    • fsc.fs imports HotReloadBaseline, HotReloadPdb, HotReload
      • Should define an IHotReloadEmitHook interface in CodeGen/ (compiles before fsc.fs), implement in HotReload/ (compiles after)
      • Pass via TcConfig.hotReloadHook: IHotReloadEmitHook option
      • fsc.fs would call 3 generic hook methods instead of hot reload APIs directly
    • service.fs adds ~590 lines + 3 mutable fields directly to FSharpChecker
      • Should extract to FSharpHotReloadService
      • The core methods already take parseAndCheckProject as function param, not this — coupling is already low
      • FSharpChecker would keep thin delegates for backward compat

Normal compilation must produce upstream-identical output

  • Non-hot-reload code paths should be byte-for-byte identical to upstream
  • Roslyn reference: CurrentGenerationOrdinal = 0 → identical output to no-EnC
  • Currently the name generation format is changed for ALL compilations, even with hot reload off
    • Upstream: name@42-N
    • This PR: name@hotreload-N (all builds)

Session scoping

  • EditAndContinueLanguageService + HotReloadState are process-global singletons, one session at a time
  • Not a real problem in practice: dotnet watch is one project per process, VS debugs one TFM at a time
  • Should be documented as known limitation - and also via a (new) separate FSharpHotReloadService it can be more apparent what does or does not support multiple projects (multiple TFMs are also treated as multiple projects)

Low-level observations

  • CompilerGlobalState.fs, FreshCompilerGeneratedNameOfBasicName:
    • The None path (no hot reload active) calls makeHotReloadName → produces name@hotreload-N
    • Upstream produces name@StartLine-N via CompilerGeneratedNameSuffix basicName (string m.StartLine + ...)
    • The None path should preserve the upstream logic exactly
  • CompilerGlobalState.fs, IncrementOnly:
    • Return value changed from 1-based (Interlocked.Increment) to 0-based (count - 1)
    • IlxGen uses this in emitted names like $"T{unique}_{size}Bytes" — shifts all values
  • CompilerGlobalState.fs, ordinal keying:
    • Changed from (basicName, m.FileIndex) to (basicName, stableFileHash m.FileName)
    • Different keying → different ordinal allocation → different names even with same format
  • SynthesizedTypeMaps.fs, BeginSession vs GetOrAddName:
    • BeginSession resets ordinals one-by-one under syncLock, but GetOrAddName doesn't take syncLock
    • Non-atomic multi-key reset — fine as long as session start is serialized before compilation begins, worth an assert or comment

@T-Gro
Copy link

T-Gro commented Feb 20, 2026

Continuation of my review, now focused on the hot reload implementation itself (code, robustness, tests) in lower detail. This is more of an "closer to production" review - feel free to dismiss for now.

My viewing angles:

  • This code will need to be maintained in the repo by people not fully having the context
  • The code will need to co-evolve as core TypeTree types evolves
  • There is some coupling to IL gen and we need people touching codegen to be made aware that hot reload needs some love

Architecture

  • IlxDeltaEmitter.emitDelta is a 2,419-line single function

    • Mixes 8+ responsibilities: token remap, metadata emit, PDB, body rewrite, baseline mutation
    • Flag: hard to review, hard to test individual phases, high blast radius for changes
    • Risk: any change to delta emission requires understanding 2K+ lines of mutable state
    • Roslyn reference: separates DeltaMetadataWriter, EncVariableSlotAllocator, PdbWriter into distinct classes
  • Module-level mutables as state management

    • Three separate global mutable clusters with no coordination:
      • HotReloadState.fs:22-24mutable session, mutable lastCommittedSession behind sessionLock
      • service.fs:219-225currentSynthesizedTypeMaps, currentOutputFingerprint, hotReloadGate
      • CompilerGlobalState.fs:82-104SynthesizedTypeMaps mutable option
    • Flag: state flows via globals and closures, not parameters — lifecycle and ownership opaque
    • Risk: no coordination between the three clusters, reasoning about "what state is active when" requires reading across all three files
    • Roslyn reference: threads state through DebuggingSession/EditSession instance objects, explicit ownership

Robustness — coupling to compiler evolution

  • opDigest catch-all | _ -> "?" on ~50 TOp cases (TypedTreeDiff.fs:394-453)

    • Flag: new TOp intrinsics silently hash to same value
    • Risk: diff misses method body changes when new ops are added → wrong/missing deltas
    • Proposal: make exhaustive, like exprDigest already is — F# exhaustiveness checking will then enforce updates
  • isLikelyStateMachineDeclaringType string heuristic (TypedTreeDiff.fs:458-486)

    • Flag: hardcodes "AsyncBuilder", "TaskBuilder", "Resumable", "QueryBuilder"
    • Risk: FSharp.Core renames or adds new builders → rude edit classification silently breaks
    • Proposal: derive from actual type metadata or a maintained registry, not string matching
  • String-based symbol identity throughout the pipeline

    • Flag: entire chain from diff → token resolution → delta emission uses string comparison
      • SymbolId.QualifiedName (concatenated string path)
      • DeltaBuilder.fs:187-200 resolves via Map.containsKey name baseline.TypeTokens
      • IlxDeltaEmitter.fs:413-501 resolves via resolveBaselineTypeFullName (string matching)
      • MethodDefinitionKey (HotReloadBaseline.fsi:13-18) is strings + ILType structural equality
      • SymbolMatcher.fs:64-90 matches via GetBasicNameOfPossibleCompilerGeneratedName (string prefix)
    • Risk: if any compiler component changes name encoding (namespace mangling, nested type separators, generic arity format), cascading silent failures across all layers
    • Roslyn reference: uses ISymbol/Cci.IDefinition — compiler's own type-safe semantic objects with proper equality, not strings
  • Manual metadata serialization (DeltaMetadataSerializer.fs, DeltaMetadataTables.fs, ILBaselineReader.fs)

    • Flag: hand-rolled ECMA-335 table serialization, coded index sizing, stream layout. This is an existing problem if ilread.fs and ilwrite.fs as well though. The risk rather is - will a person updating ilread.fs/ilwrite.fs be told to also care for hot reload ser/de ?
    • Risk: must track .NET runtime metadata evolution manually — new tables, coded index families, heap format changes all require hand-updating multiple files
    • Roslyn reference: reuses System.Reflection.Metadata / inherits from shared MetadataWriter — gets updates via library upgrades
  • Coupling failure modes summary when it comes to evolution of core compiler types, and what could happen to hot reload — safe vs silent:

    • New Expr DU case → exprDigestcompile error ✅ (exhaustive match)
    • New TOp intrinsic → opDigestsilent ❌ (catch-all hides it)
    • New TType variant → tryTypeIdentityFromTTypesilent ❌ (falls to None, overload disambiguation breaks)
    • New IlxGen codegen pattern → hotReloadIlxNamesilent ❌ (no enforcement)
    • New CE builder / state machine → isLikelyStateMachineDeclaringTypesilent ❌ (string match misses)
    • New IL metadata table (.NET evolution) → remapEntityToken, baseline reader, serializer → silent ❌ (unremapped)
    • New IlxGen typar ordering → tryGetMethodTyparOrdinalsAndGenericAritysilent ❌ (identity mismatch)
    • Pattern: Expr changes are compiler-enforced safe. Everything else is silent wrong deltas.
  • No enforcement that new codegen uses hotReloadIlxName (IlxGen.fs:49-55, 8 call sites)

    • Flag: call sites are manually threaded, a developer adding a new AllocLocal or closure type has no signal to use the HR-aware path
    • Risk: synthesized name not tracked → SymbolMatcher fails to match → method update silently dropped
    • Proposal: even if hotReloadIlxName is removed (per my previous review), whatever naming path remains should be the only path — not an opt-in branch

Separation

  • DefinitionMap.fs in TypedTree/ declares FSharp.Compiler.HotReload.DefinitionMap
    • FSharpSymbolChanges.fs in CodeGen/ declares FSharp.Compiler.HotReload.SymbolChanges
    • Flag: HR-namespaced files living in core compiler directories
    • Proposal: move to HotReload/ to match their namespace

Duplication

  • isEnvVarTruthy duplicated 9 times across 8+ files

    • In: DeltaBuilder, EditAndContinueLanguageService, HotReloadCapabilities, FSharpDeltaMetadataWriter, IlxDeltaEmitter, service.fs, TypedTreeDiff, HotReloadBaseline, test helpers
    • Proposal: extract to a shared utility module
  • ApplyUpdate test setup duplicated 3×

    • Identical ~140-line C# source literal + reflection setup in ApplyUpdateChild.fs, ApplyUpdateConsole.fs, ApplyUpdateRunner.fs
    • Proposal: extract to shared test helper

Test coverage

  • Zero delta/runtime test coverage for 15+ F# constructs
    • This should be a main regression-test mechanism to fail a test when a different contributor will cause HR to fail, behave differently, or possibly even crash the compiler. I am fully aware the full language cannot be covered, but this is rather about spotting unintended regressions coming via unrelated compiler PRs.
    • Tier 1 (common patterns, should have before merge):
      • seq expressions, records, discriminated unions, structs, recursive bindings
    • Tier 2 (important, can follow up):
      • quotations, anonymous records, active patterns, object expressions, loops
    • For scale: Roslyn has dedicated 10K+ line test suites per construct category (closures, state machines, etc); F# currently has ~200 lines each
    • Suggestion: at minimum, add one "edit method body that uses X" test per Tier 1 construct to verify deltas round-trip correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants