Report the resolved overload for [<CustomOperation>] keywords in QuickInfo and SymbolUse by T-Gro · Pull Request #19865 · dotnet/fsharp

T-Gro · 2026-05-29T18:53:05Z

Summary

For overloaded [<CustomOperation>] keywords in a computation expression, QuickInfo and GetAllUsesOfAllSymbolsInFile now report the MethInfo that F# overload resolution actually picked, matching the behaviour of regular overloaded method calls. Previously both reported the first-registered overload regardless of resolution.

) Captures issue #11612: when a CE builder defines multiple [<CustomOperation>] overloads, FCS GetToolTip always returns the generic 'custom operation: NAME (bool)' description from the last registered overload rather than the resolved overload's parameter types. Current failures (RED): - CE custom operator QuickInfo shows resolved overload: tooltip is 'custom operation: whereOp (bool)\nCalls Builder.Wh...' - missing 'int' from resolved WhereInt overload - CE custom operator with three overloads shows resolved float overload: tooltip is 'custom operation: filterOp (bool)\nCalls Builder.F...' - missing 'float' - GetSymbolUse resolves correct CE operator overload: tooltip is 'custom operation: pickOne (bool)\nCalls Builder.Pi...' - missing 'int' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…11612) Aggregate sibling [<CustomOperation>] methods on the builder type when rendering Item.CustomOperation tooltips, placing the resolved overload first. When there is more than one overload, also append the first parameter's type so that distinct overloads are visibly distinguished. Fixes #11612. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-05-29T18:53:49Z

❗ 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

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

brianrourkeboll

Why show all of them instead of the resolved overload? (#15206 (comment))

In my opinion, the behavior would ideally be the same for overloaded custom operations as it is for overloaded methods.

T-Gro · 2026-06-01T13:00:35Z

@brianrourkeboll :

Let me test how much this complicates things - otherwise agree.

…ethInfo (#11612 / #15206) Reworks PR #19865 in response to review feedback: instead of rendering 'all overloads' in QuickInfo, sink the resolved overload so QuickInfo, GetAllUsesOfAllSymbolsInFile and other symbol-use consumers report the overload that F# overload resolution actually picks (matching how regular method overloads behave). The early Item.CustomOperation sink call still fires with opDatas[0] as the placeholder MethInfo so the ordering and presence of records in TcResolutions stays bit-for-bit identical to main for all single-overload and error-recovery cases. A lightweight ITypecheckResultsSink wrapper captures the singleton method-group resolution that lands at the synthesized mkSynCall range; after TcExpr finishes, drainDeferredCustomOpSinks calls CallNameResolutionSinkReplacing only when the captured overload differs from the fallback by signature. Covers both the unary ConsumeCustomOpClauses path and the join/zip/groupJoin path (mkJoinExpr / mkZipExpr). Reverts PR #19865's sibling-gathering changes in ServiceDeclarationLists.fs and the new TryGetCustomOperationName export in CheckComputationExpressions.fsi. Tests: - TooltipTests gain three regressions: XmlDoc-driven QuickInfo for the int vs string overload, GetAllUsesOfAllSymbolsInFile parameter-type check, and a smoke test for join-like overloaded operations. - Project12 (query / where / select symbol uses) verified unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Address findings from three concurrent reviews (Opus 4.7 high, GPT-5.5, Opus 4.8) of commit 4ba013a: * Remove unused MethodDisplayName field on DeferredCustomOpSink and the matching parameter on enqueueCustomOperationSink. The candidate-by-sig match in CustomOpResolutionCapturingSink already handles disambiguation. * Switch Resolved from option ref to a mutable record field, matching TcResultsSink.CurrentSink and avoiding the extra ref-cell allocation. * Flatten the nested match in CustomOpResolutionCapturingSink.tryCapture into a single combined pattern on (TryGetValue, item). * Introduce a local 'forward' helper to collapse the repeated forwardTo |> Option.iter (fun s -> ...) noise across the 9 forwarding members of the sink wrapper. * Use Option.bind for CurrentSourceText / FormatStringCheckContext. * Skip the sink wrapping + drain entirely when no custom-operation keyword usages were enqueued. The wrapper was previously installed for every CE — including async / task / seq / option that have no custom ops — paying per-notification allocation cost for no gain. * Rename 'tracked' to 'deferredSinksBySyntheticRange' for clarity. * Delete the vacuous join-like regression test: it created a builder with overloaded IsLikeZip [<CustomOperation>] methods but never used myzip in a CE body, so it passed even with the fix reverted. The unary-path symbol-use test in this file already exercises the same enqueue + drain mechanism that the join/zip/groupJoin path uses. All 60 TooltipTests still pass; Project12 'all symbols' still passes; 153 CE ComponentTests still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…n test Apply findings from second concurrent review round (Opus 4.7 high, GPT-5.5, Opus 4.8): * Drop unused `m: range` constructor parameter on `CustomOpResolutionCapturingSink` and switch `matchesCandidate` to use the per-entry `KeywordRange`, matching the drain. The wrapper no longer needs the unrelated `mWhole` plumbed through it. * Extract `isDifferentOverload entry resolved` named predicate from the `when` guard in `drainDeferredCustomOpSinks`. * Strengthen the wrapper-skip gate to also skip when `cenv.tcSink.CurrentSink.IsNone` — plain `dotnet build` (no IDE) has no consumer for the resolved sink record, so the wrapper and drain are pure waste on the CLI path. * Replace the local `forward f = forwardTo |> Option.iter f` helper with inline `match forwardTo with | Some s -> s.Notify… | None -> ()` in every interface member. This eliminates the per-notification closure allocation that the helper introduced inside the CE body. * Add `methInfosOfOpDatas` helper to deduplicate the 9-tuple `List.map (fun (_,...,_, mi) -> mi)` projection at the two enqueue sites. The shape comes from `getCustomOperationMethods`. * Add a real `IsLikeZip` regression test (`b { for x in [1;2] do myzip y in [3;4] select (x + y) }` × int/string overloads) covering the join/zip/groupJoin code path's `mOpCore.MakeSynthetic()` enqueue. Previously that path had no symbol-use regression — only the unary `ConsumeCustomOpClauses` path (which uses `mClause.MakeSynthetic()`) was tested. * Add a comment near the wrapper install site warning future maintainers not to forward through `cenv.tcSink` (that would recurse into the wrapper). Nested CEs correctly chain via captured `oldSinkOpt`. * Rename `range` parameter shadowing the `range` type to `m` in `tryCapture` for consistency with the rest of the file. All 61 TooltipTests pass, Project12 'all symbols' passes, 153 CE ComponentTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

User feedback: don't bloat `CheckComputationExpressions.fs` (already ~3300 lines) with sink-wrapping plumbing that's only used for one narrow feature. * New file: `src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs` - `type DeferredCustomOpSink` record - `type private CustomOpResolutionCapturingSink` wrapper class with all 11 `ITypecheckResultsSink` members - `enqueueDeferredCustomOpSink` helper (early fallback sink + queue add) - `withCapturingTcSink` helper that installs the wrapper, runs the caller's action, and drains the queue — gating on `queue.Count = 0 || sink.CurrentSink.IsNone` is now internal - `methInfosOfOpDatas` opData tuple projection (moved from CCE.fs) * `CheckComputationExpressions.fs` now contains only the surface: - One `open` of the new module - One field on `ComputationExpressionContext` - Two `enqueueDeferredCustomOpSink` calls at the unary/join-zip sites - One `withCapturingTcSink` call around `TcExpr lambdaExpr` Net effect on the big file vs main: from +282/-19 to +62/-37. The new module is ~200 LOC and self-contained — easy to delete or modify without touching the 3300-line CE checker. Tests unchanged. All 61 TooltipTests pass; Project12 passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Apply round-1 review consensus from 4 reviewers (Opus 4.7 high reuse, GPT-5.5 idiomatic, Opus 4.8 overengineering, Opus 4.7 high content-vs-file): * Rename file/module 'CheckComputationExpressionOverloads' → 'CheckComputationExpressionsCustomOps'. The old name read like the overload-resolution checker; this module never resolves overloads, it just captures+replays the result. The new name reflects that it's the CE checker's helper for CustomOperation symbol-use sinks. * Rename 'withCapturingTcSink' → 'captureCustomOperationOverloads'. The old name said 'capturing' without saying what. * Move 'methInfosOfOpDatas' back to CheckComputationExpressions.fs near 'getCustomOperationMethods'. The opData 9-tuple is owned by CCE.fs; the projection has no connection to sink replay, only happens to be used by the two enqueue call sites. * Swap match order in 'tryCapture' so the cheap 'item' DU pattern short-circuits before the dictionary lookup. * Switch the wrapper sink's 9 forwarding members from 'match forwardTo with | Some s -> ... | None -> ()' to 'forwardTo |> Option.iter (fun s -> ...)' for consistency with the Option.bind calls already used on CurrentSourceText / FormatStringCheckContext. * Fix stale xref 'enqueueCustomOperationSink' → 'enqueueDeferredCustomOpSink' on the deferredCustomOpSinks queue doc comment in CCE.fs. All four reviewers concurred that the dual early+late sink design, the 'Candidates' field, and the 'isDifferentOverload' equality guard are all load-bearing (Project12 'all symbols' depends on insertion ordering preserved by the early sink + the no-op drain). All 61 TooltipTests pass; Project12 passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Round-2 consensus from 4 reviewers (Opus 4.7 high reuse, GPT-5.5 idiomatic, Opus 4.8 overengineering, Opus 4.7 high content-vs-file): Content-vs-file reviewer: "split is at natural seam, ship it". Overengineering reviewer: "no blocking, no non-blocking, no suggestions worth acting on". Idiomatic + reuse reviewers each flagged exactly one optional change. Applied: * Reuse Opus 4.7 S2: add prior-art note at the top of the module citing TcMethodItemThen as the same idiom for type-providers static arguments at a single range; we're the cross-range generalisation. Not applied: * Reuse Opus 4.7 S1 (use MultiMap.initBy instead of imperative Dictionary build): MultiMap requires 'T: comparison and 'range' has NoComparison, so the helper is unusable for our key type. Tried it, build failed, reverted. * GPT-5.5 NB-1 (group sink/nenv/ad/queue into sub-record): other reviewers correctly call this a lateral move; positional args are fine at 2 call sites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Round-3 consensus from 4 reviewers (Opus 4.7 high reuse, GPT-5.5 idiomatic, Opus 4.8 overengineering, Opus 4.7 high content-vs-file): * GPT-5.5 idiomatic: "no remaining idiomatic improvements". * Opus 4.7 content-vs-file: "R2 stands verbatim. Ship it." * Opus 4.8 overengineering: "ship-ready, no Blocking, no Non-Blocking, no Suggestions worth acting on". * Opus 4.7 reuse: one optional suggestion — applied below. Applied: * Use Range.comparer (from FSharp.Compiler.Text.Range) instead of HashIdentity.Structural for the deferredSinksBySyntheticRange Dictionary. Matches the convention established at ServiceParseTreeWalk.fsi:260, 391 and avoids the generic-equality dispatch overhead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…mmittee Dispatched 5 parallel rewrite agents (Opus 4.6, Opus 4.7, Opus 4.7-high, Opus 4.8, GPT-5.5) each asked to make the file smaller / more elegant / more F# idiomatic, preserving public surface and load-bearing semantics. Followed up with a 3-voter committee (Opus 4.7-high, Opus 4.8, GPT-5.5) to synthesize. Unanimous 3-0 votes on all decisions: * A. Replace 'type private CustomOpResolutionCapturingSink' class with a 'let private makeCustomOpResolutionCapturingSink' factory returning an object expression. Drops constructor ceremony and ':>' upcast. * B. Inline 'matchesCandidate' (one-use 2-line List.exists). * C. Inline 'isDifferentOverload' (one-use single negated equiv check). * D2 (Opus 4.8's bold insight). 'captureCustomOperationOverloads' is already gated on 'sink.CurrentSink.IsSome', so pass the unwrapped ITypecheckResultsSink (not option) to the wrapper. Drops all 'forwardTo |> Option.iter (fun s -> ...)' plumbing (9 members) and both 'Option.bind' for CurrentSourceText/FormatStringCheckContext. Members become direct delegates. * E1. Keep nested match in tryCapture (outer on item, inner on dict lookup) so non-MethodGroup notifications skip the dict lookup on the hot path. NotifyNameResolution fires per name resolution in CE body. * F1. Minimal helpers — don't split into 7 tiny private functions. Also collapse 'if queue.Count = 0 || sink.CurrentSink.IsNone' into a single pattern 'match sink.CurrentSink with | Some oldSink when queue.Count > 0 -> ... | _ -> action()'. Net: 200 → 187 LOC (file) but -44/+32 in the part of the file that changed (the wrapper and the install/drain). The wrapper class is gone; the Option ceremony is gone; the named helpers are gone. Load-bearing semantics unchanged: early sink before queue.Add, Candidates + isDifferentOverload guard, Range.comparer dict, nested-CE chaining via sink.CurrentSink. Verified: 61 TooltipTests pass; Project12 'all symbols' passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ndSig hot-path scan User feedback flagged three real problems with the previous design: 1. 'mutable Resolved' on DeferredCustomOpSink — the record was already stored in mutable containers (ResizeArray + Dictionary), so a mutable field added nothing but ugly. Fully immutable record now; captured resolutions live in a side Dictionary keyed by SyntheticCallRange. 2. 'Candidates: MethInfo list' + 'matchesCandidate' did a LINEAR scan via the deep 'MethInfosEquivByNameAndSig EraseAll true' on EVERY name resolution at a tracked synthetic range. Overload resolution already resolved the call by the time it notifies — re-comparing against the candidate list is rubbish (user's word, fair). Dropped 'Candidates' entirely. The wrapper now does an O(1) string compare on the method LogicalName to filter out unrelated MethodGroup notifications that share the synthetic range (e.g. an outer 'For' call in a join/zip clause — discovered while debugging, was masked by the candidate check before). 3. Reinvented HashMultiMap — actually after this redesign the multi-map isn't needed at all: each SyntheticCallRange has exactly one resolved overload, so a plain Dictionary<range, _>(Range.comparer) is enough. At drain time, replace deep MethInfosEquivByNameAndSig with cheap MethInfo.MethInfosUseIdenticalDefinitions (ValRef / ILMethodRef ref equality). Same correctness for 'is this the same logical method as the fallback?' question, O(1) instead of deep type comparison. Net surface changes: * DeferredCustomOpSink: 9 fields -> 7, no 'mutable', no 'Candidates'. * enqueueDeferredCustomOpSink: 10 args -> 9, no 'candidates'. * captureCustomOperationOverloads: drops g/amap parameters. * CheckComputationExpressions.fs: drops 'methInfosOfOpDatas' helper. Hot-path cost per name-resolution notification at a tracked synthetic range: * Before: 1 dict lookup + 1 list iteration + up to N MethInfosEquivByNameAndSig deep comparisons. * After: 1 dict lookup + 1 string compare. Drain-time check (once per CO usage): * Before: 1 MethInfosEquivByNameAndSig (deep). * After: 1 MethInfosUseIdenticalDefinitions (ref equality on RawMetadata or valRefEq). All 61 TooltipTests pass. Project12 'all symbols' passes. 153 CE ComponentTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

User asked: what if Item.MethodGroup has more than 1 element so the [ mi ] pattern in tryCapture doesn't match? What scenario is that? Is it tested? Scenario: ResolveExprDotLongIdentAndComputeRange in NameResolution.fs:4310-4314 fires the *unrefined* Item.MethodGroup(name, [...allCandidates...], _) *before* AfterResolution.RecordResolution settles. Once overload resolution picks an overload, callSinkWithSpecificOverload re-sinks the refined Item.MethodGroup(name, [chosenMethod], None) singleton. The wrapper's [ mi ] pattern intentionally only matches the refined singleton — capturing the unrefined multi-list would replay a record pointing at the wrong overload (and we'd have no way to know which one was actually picked). If the refined notification *never* fires (overload resolution failed on broken user code), the dict slot stays at the pre-populated Fallback; the drain's MethInfosUseIdenticalDefinitions check decides to leave the eager Item.CustomOperation(opName, _, Some Fallback) sink record in place. This is graceful degradation — IDE features at the keyword still see *a* MethInfo instead of blanking out. Added an explicit test 'Broken overloaded CE custom op call falls back to the eager opDatas[0] sink record' that exercises this path: two overloaded Pick methods (int and string) called with a bool argument that matches neither. The test asserts (1) a type error is emitted and (2) the keyword still has exactly one Item.CustomOperation symbol-use record (the fallback). Without graceful fallback, the assertion on the keyword's symbol use would return zero entries. Also extended the xmldoc on makeCustomOpResolutionCapturingSink to explain the unrefined/refined split and why we only match the singleton. 62 TooltipTests pass (added 1); Project12 passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Per feedback: quality product code doesn't need commenting; test code can have some. Removed all xmldoc/inline comments from CheckComputationExpressionsCustomOps.fs and the related field/value sites in CheckComputationExpressions.fs. 175 -> 125 LOC in the module. The same 62 TooltipTests pass; Project12 'all symbols' passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

T-Gro · 2026-06-04T12:40:46Z

New approach - let the actual method resolution replace the overload in the sink once it happens.

abonie · 2026-06-11T15:07:54Z

+        for entry in queue do
+            let _, resolved, tpinst = capturedResolutions[entry.SyntheticCallRange]
+
+            if not (MethInfo.MethInfosUseIdenticalDefinitions resolved entry.Fallback) then


MethInfosUseIdenticalDefinitions guard discards the captured TyparInstantiation.

The eager sink at line 86 uses emptyTyparInst (translation runs before overload resolution). The wrapper captures the real tpinst into the dictionary slot at line 32, but the drain only fires Replacing when the resolved MethInfo differs from the fallback. For single-overload custom ops on a generic builder (and for multi-overload cases where resolution picks the first-registered overload), the guard short-circuits and the richer tpinst is dropped — QuickInfo stays on emptyTyparInst and shows the uninstantiated generic signature.

The deleted FUTURE: consider whether we can do better than emptyTyparInst here comment describes exactly this case, and the captured value is already sitting at line 115 — consider also gating on whether tpinst is non-empty.

Copilot and others added 2 commits May 29, 2026 18:22

T-Gro requested a review from a team as a code owner May 29, 2026 18:53

github-project-automation Bot added this to F# Compiler and Tooling May 29, 2026

github-project-automation Bot moved this to New in F# Compiler and Tooling May 29, 2026

Add PR link to release notes entry

6c5c951

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Move release notes entry for #11612 to 11.0.100.md

de10d70

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

brianrourkeboll reviewed May 29, 2026

View reviewed changes

auduchinok reviewed May 29, 2026

View reviewed changes

Comment thread tests/FSharp.Compiler.Service.Tests/TooltipTests.fs Outdated

github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label May 29, 2026

T-Gro and others added 11 commits June 4, 2026 05:09

T-Gro requested a review from abonie June 4, 2026 12:40

T-Gro changed the title ~~Show all overloads in QuickInfo for overloaded CE custom operators~~ Report the resolved overload for [<CustomOperation>] keywords in QuickInfo and SymbolUse Jun 4, 2026

abonie reviewed Jun 11, 2026

View reviewed changes

abonie approved these changes Jun 11, 2026

View reviewed changes

github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 11, 2026

Merge remote-tracking branch 'origin/main' into fix/issue-11612

7e0710e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report the resolved overload for [<CustomOperation>] keywords in QuickInfo and SymbolUse#19865

Report the resolved overload for [<CustomOperation>] keywords in QuickInfo and SymbolUse#19865
T-Gro wants to merge 16 commits into
mainfrom
fix/issue-11612

T-Gro commented May 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 29, 2026 •

edited

Loading

Uh oh!

brianrourkeboll left a comment

Uh oh!

Uh oh!

T-Gro commented Jun 1, 2026

Uh oh!

T-Gro commented Jun 4, 2026

Uh oh!

abonie Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

T-Gro commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

github-actions Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Release notes required

Uh oh!

brianrourkeboll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

T-Gro commented Jun 1, 2026

Uh oh!

T-Gro commented Jun 4, 2026

Uh oh!

abonie Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

T-Gro commented May 29, 2026 •

edited

Loading

github-actions Bot commented May 29, 2026 •

edited

Loading