Add typed-tree differ and edit classification for hot reload#20025
Add typed-tree differ and edit classification for hot reload#20025NatElkins wants to merge 3 commits into
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
dfbcfb8 to
09bbef5
Compare
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*"
09bbef5 to
f8d6d01
Compare
Verified with the repository-wide Fantomas check.
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 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:
- Unbounded non-tail recursion in
exprIdentity— a real robustness risk on a hot path that runs in-process on every edit; a deep body canStackOverflowException(uncatchable) and take down the host instead of degrading to a rude edit. - Positional pairing in
compareBindingLists— sound outcome but misattributes the removed/added/changed symbol for overload groups, which will mislead the downstream session's diagnostics. sprintf "%A"for method-ref identity — reflective formatting invoked perILCall/ILAttribnode 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.
| types returnTypes | ||
| ] | ||
|
|
||
| let rec private exprIdentity (denv: DisplayEnv) (expr: Expr) = |
There was a problem hiding this comment.
exprIdentity/decisionTreeIdentity/bindingIdentity form a mutually-recursive walk that is not tail-recursive and has no depth bound. A right-nested Expr.Sequential/Expr.Let/Expr.Match chain (common in large or machine-generated bodies) recurses linearly in the tree depth and can throw StackOverflowException, which is uncatchable in .NET and will crash the host process rather than degrade to a rude edit. This is the same class of bug recently fixed elsewhere in the tree (deep Sequential chains, #20028). Since this runs in-process during hot reload, consider a depth guard that fails closed to a rude edit (e.g. RudeEditKind.Unsupported) once a limit is exceeded, or an explicit worklist instead of stack recursion. Relatedly, Expr.Link expressionRef -> recurse expressionRef.Value (line 514) follows the ref unconditionally; a self-referential/recursive Link fixup would recurse without termination.
| | Choice1Of2 None -> () | ||
| | Choice2Of2 rudeEdit -> rude.Add rudeEdit | ||
|
|
||
| baseline |
There was a problem hiding this comment.
After sorting both lists by (SignatureText, LogicalName) and pairing by index, the leftover-detection here is positional: the reported DeclarationRemoved/DeclarationAdded symbols (and the SignatureChange from compareMatchedBinding on mispaired entries) are whichever items land at the tail/index, not the ones that were actually removed/added. Example: a key group with overloads whose signatures sort as [A,B,C] in baseline and [A,C] in updated (B removed) pairs B→C (spurious signature-change) and reports C as removed. The final outcome stays a rude edit, so it is sound (falls back to restart), but the symbol/message attributed to the edit is wrong and will mislead the downstream session's rude-edit diagnostics. Consider matching on the full key (including signature) as a set rather than index-pairing sorted lists.
| string valUseFlag | ||
| string isProperty | ||
| string noTailCall | ||
| sprintf "%A" methodRef |
There was a problem hiding this comment.
sprintf "%A" methodRef (and the same at line 639 for AttribKind.ILAttrib) uses reflection-based structural formatting, which is slow and allocation-heavy. It is invoked once per TOp.ILCall/ILAttrib node while hashing entire method bodies, on a path that runs on every hot-reload edit. Prefer a targeted, cheap key built from the method ref's declaring type ref, name, and argument/return type strings (as done elsewhere via tyToString) instead of %A.
Adds an internal typed-tree differ for F# hot reload:
TypedTreeDiffcompares twoCheckedImplFiles 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.
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 (F# hot reload: Edit-and-Continue delta emission behind --test:HotReloadDeltas #19941 in its final, much smaller form).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)