Skip to content

Add typed-tree differ and edit classification for hot reload#20025

Open
NatElkins wants to merge 4 commits into
dotnet:mainfrom
NatElkins:hotreload-typedtree-diff
Open

Add typed-tree differ and edit classification for hot reload#20025
NatElkins wants to merge 4 commits into
dotnet:mainfrom
NatElkins:hotreload-typedtree-diff

Conversation

@NatElkins

@NatElkins NatElkins commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Adds an internal typed-tree differ for F# hot reload: TypedTreeDiff compares two CheckedImplFiles and classifies each change as a semantic edit (method body update, insert) or a rude edit (declaration removed, signature change, type-parameter constraint change, inline-info change, type layout change). Reference-equal files short-circuit to an empty diff, which is the sound fast path when the transparent compiler returns unchanged files.

This is the classification layer the hot reload session uses to decide what goes into an Edit and Continue delta and what must fall back to a restart. It is pure: no session state, no runtime capability handling, no emission. Entity logical names carry generic arity where the typed tree provides it, so generic and non-generic types stay distinct in the diff.

Nothing on main calls this yet; the module is internal and the surface area baseline is untouched. Runtime capability negotiation and the deeper lambda and state machine occurrence classification stay in the later session slice, where their dependencies actually live.

Tests: 8 unit tests covering the fast path, body edits, inserts, removals, signature and constraint changes, and layout changes. SurfaceAreaTest green.

Sequencing

This PR is part of splitting the F# hot reload work (#19941) into small, independently reviewable PRs. The planned order:

  1. Wave 1 (independent of each other, no cross dependencies): Add ResetCompilerGeneratedNameState to compiler-generated name generators #20017 (generated-name counter reset), Add Roslyn-format EnC CustomDebugInformation codec and portable PDB method CDI emission #20018 (EnC CustomDebugInformation codec and method CDI emission), Add ECMA-335 EnC metadata delta writer #20019 (EnC metadata delta writer), Add stable synthesized-name replay infrastructure for hot reload #20024 (stable synthesized-name replay), and this PR.
  2. Wave 2: baseline reading and recorded CDI state (depends on Add Roslyn-format EnC CustomDebugInformation codec and portable PDB method CDI emission #20018 and Add stable synthesized-name replay infrastructure for hot reload #20024).
  3. Wave 3: the delta emitter and symbol matcher (consumes this differ).
  4. Wave 4: the hot reload session, FCS surface, and the --test:HotReloadDeltas capture hook (F# hot reload: Edit-and-Continue delta emission behind --test:HotReloadDeltas #19941 in its final, much smaller form).
  5. Last, explicitly experimental: Add an experimental flag-gated in-process compile path for hot reload sessions #20031 (the flag-gated in-process compile perf path).

Role in the train: the delta emitter (wave 3) consumes this diff to scope emission; the session (wave 4) consumes the rude-edit classification to decide restart versus apply.

Refresh status (2026-07-17)

  • Refreshed against dotnet/fsharp main at 5928e91; the current reviewed head is 97ab0e6.
  • A dedicated review pass completed for this PR, its findings were fixed in the lowest owning slice, and the PR has no unresolved review threads.
  • The downstream session, in-process compiler, umbrella, and SDK branches were restacked after the fixes, so this slice remains part of the decomposed review train.
  • The complete compiler stack passed the 11-step hot reload verifier, 456 service tests, 243 component tests with 2 expected skips, and 1411 EmittedIL tests with 3 expected skips. Replacement CI passed on this exact head.

Refresh status (2026-07-22)

  • Current head: 6129dda, refreshed against dotnet/fsharp main 69fca7f.
  • Stack position: independent wave-1 typed-tree differ; consumed by Add the hot reload delta emitter and symbol matcher #20027 and every later composite slice.
  • All July 21 findings are fixed and resolved: bounded/cycle-safe traversal, exact-first overload pairing, structural IL method identity, and fail-closed field-backed module initializer edits.
  • Local verification: service-test build passed with 0 warnings/errors and TypedTreeDiffTests passed 20/20.
  • Exact-head Azure compiler CI passed all 46 jobs. The base-branch check_release_notes workflow still fails before analysis because pull_request_target refuses fork checkout.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.

Reviewed the typed-tree differ (TypedTreeDiff.fs), which is the bulk of the change. The design is sound in the important direction: every MethodBody semantic edit is gated on equal signature/constraints/metadata/inline info, so ambiguous pairings degrade to rude edits (restart) rather than an unsound in-place patch. The reference-equality fast path and fail-closed try/with guards are reasonable. A few substantive concerns are noted inline:

  1. Unbounded non-tail recursion in exprIdentity — a real robustness risk on a hot path that runs in-process on every edit; a deep body can StackOverflowException (uncatchable) and take down the host instead of degrading to a rude edit.
  2. Positional pairing in compareBindingLists — sound outcome but misattributes the removed/added/changed symbol for overload groups, which will mislead the downstream session's diagnostics.
  3. sprintf "%A" for method-ref identity — reflective formatting invoked per ILCall/ILAttrib node while hashing whole method bodies; slow and allocation-heavy on the edit path.

One thing to verify downstream (not blocking, and explicitly deferred per the PR description): a body change to a module-level let value is classified as MethodBody, but that recompiles the module .cctor/static initializer. The session slice that consumes this must ensure runtime-capability negotiation treats static-initializer edits correctly rather than assuming a plain method-body delta.

Comment thread src/Compiler/TypedTree/TypedTreeDiff.fs Outdated
Comment thread src/Compiler/TypedTree/TypedTreeDiff.fs Outdated
Comment thread src/Compiler/TypedTree/TypedTreeDiff.fs Outdated
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Jul 21, 2026
@T-Gro
T-Gro self-requested a review July 21, 2026 10:58
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*"
Verified with the repository-wide Fantomas check.
@NatElkins
NatElkins force-pushed the hotreload-typedtree-diff branch from 97ab0e6 to 6129dda Compare July 22, 2026 18:23
@NatElkins

Copy link
Copy Markdown
Contributor Author

I also verified the field-backed module initializer concern from the review summary. The old path classified the edit as a MethodBody update. Commit 6129dda now fails closed with an Unsupported rude edit, and the regression test failed before the fix.

@github-actions github-actions Bot added the ⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen label Jul 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Compiler-Output
Affects-Compiler-Output: adds TypedTreeDiff.fs to compiler tree

Generated by PR Tooling Safety Check · opus46 5.3M ·

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

Labels

⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

2 participants