From e8f9625f2b254bed7c1d9dd29226e94f8afac7fa Mon Sep 17 00:00:00 2001 From: Copilot Date: Fri, 29 May 2026 18:22:28 +0200 Subject: [PATCH 01/15] Add failing FCS tooltip tests for overloaded CE custom operators (#11612) Captures issue #11612: when a CE builder defines multiple [] 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> --- .../TooltipTests.fs | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs index 2947e09f565..7220147402d 100644 --- a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs @@ -852,3 +852,99 @@ let inline fo{caret}o< ^T> (x: ^T) = x |> fun text -> // Type param appears in tooltip Assert.Contains("'T", text) + +// https://github.com/dotnet/fsharp/issues/11612 +// Tooltip for overloaded CE [] should reflect the resolved overload, +// not just the last one registered with the builder. +let private renderAllGroups (ToolTipText elements) = + let sb = System.Text.StringBuilder() + for el in elements do + match el with + | ToolTipElement.Group items -> + for item in items do + for line in item.MainDescription do + sb.Append(line.Text) |> ignore + sb.Append('\n') |> ignore + | ToolTipElement.CompositionError msg -> sb.AppendLine(msg) |> ignore + | ToolTipElement.None -> () + sb.ToString() + +[] +let ``CE custom operator QuickInfo shows resolved overload`` () = + Checker.getTooltip """ +type Builder() = + member _.Yield(x) = [x] + member _.For(xs, body) = xs |> List.collect body + [] + member _.WhereInt(xs: int list, [] f: int -> bool) = List.filter f xs + [] + member _.WhereStr(xs: string list, [] f: string -> bool) = List.filter f xs + +let b = Builder() +let result = b { for x in [1;2;3] do whereO{caret}p (x > 0) } +""" + |> renderAllGroups + |> fun text -> Assert.Contains("int", text) + +[] +let ``CE custom operator with three overloads shows resolved float overload`` () = + Checker.getTooltip """ +type Builder() = + member _.Yield(x) = [x] + member _.For(xs, body) = xs |> List.collect body + [] + member _.F1(xs: int list, [] f: int -> bool) = List.filter f xs + [] + member _.F2(xs: float list, [] f: float -> bool) = List.filter f xs + [] + member _.F3(xs: string list, [] f: string -> bool) = List.filter f xs + +let b = Builder() +let result = b { for x in [1.0;2.0] do filterO{caret}p (x > 0.0) } +""" + |> renderAllGroups + |> fun text -> Assert.Contains("float", text) + +[] +let ``CE single custom operator QuickInfo still works`` () = + Checker.getTooltip """ +type Builder() = + member _.Yield(x) = [x] + member _.For(xs, body) = xs |> List.collect body + [] + member _.W(xs: int list, [] f: int -> bool) = List.filter f xs + +let b = Builder() +let result = b { for x in [1;2;3] do whereSing{caret}le (x > 0) } +""" + |> renderAllGroups + |> fun text -> Assert.Contains("whereSingle", text) + +[] +let ``Regular method overload QuickInfo unaffected`` () = + Checker.getTooltip """ +type T() = + member _.M(x: int) = x + member _.M(x: string) = x.Length +let t = T() +let r = t.M{caret}(42) +""" + |> renderAllGroups + |> fun text -> Assert.Contains("int", text) + +[] +let ``GetSymbolUse resolves correct CE operator overload`` () = + Checker.getTooltip """ +type Builder() = + member _.Yield(x) = [x] + member _.For(xs, body) = xs |> List.collect body + [] + member _.PickInt(xs: int list, [] f: int -> bool) = List.filter f xs + [] + member _.PickStr(xs: string list, [] f: string -> bool) = List.filter f xs + +let b = Builder() +let result = b { for x in [1;2;3] do pickO{caret}ne (x > 0) } +""" + |> renderAllGroups + |> fun text -> Assert.Contains("int", text) \ No newline at end of file From 5fd6c99f811ecc0cd1e6b9142e94c6b724099f6b Mon Sep 17 00:00:00 2001 From: Copilot Date: Fri, 29 May 2026 19:33:41 +0200 Subject: [PATCH 02/15] Show all overloads in QuickInfo for overloaded CE custom operators (#11612) Aggregate sibling [] 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> --- .../.FSharp.Compiler.Service/9.0.300.md | 1 + .../CheckComputationExpressions.fs | 40 +++++++++-------- .../CheckComputationExpressions.fsi | 6 +++ .../Service/ServiceDeclarationLists.fs | 43 ++++++++++++++++--- 4 files changed, 66 insertions(+), 24 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md index 7f5ab7289d2..126a9858442 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md @@ -1,4 +1,5 @@ ### Fixed +* Fix QuickInfo for overloaded CE custom operators to show all overloads instead of always the last one. ([Issue #11612](https://github.com/dotnet/fsharp/issues/11612)) * Fix missing TailCall warning in TOp.IntegerForLoop ([PR #18399](https://github.com/dotnet/fsharp/pull/18399)) * Fix classification of `nameof` in `nameof<'T>`, `match … with nameof ident -> …`. ([Issue #10026](https://github.com/dotnet/fsharp/issues/10026), [PR #18300](https://github.com/dotnet/fsharp/pull/18300)) * Fix Realsig+ generates nested closures with incorrect Generic ([Issue #17797](https://github.com/dotnet/fsharp/issues/17797), [PR #17877](https://github.com/dotnet/fsharp/pull/17877)) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index 3ab446a6687..e1b0f1741fc 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -178,6 +178,26 @@ let transferVarSpaceReferences (expr: Expr) = for v in vals do v.SetHasBeenReferenced() +let TryGetCustomOperationName g m (methInfo: MethInfo) : string option = + TryBindMethInfoAttribute + g + m + g.attrib_CustomOperationAttribute + methInfo + IgnoreAttribute // We do not respect this attribute for IL methods + (fun attr -> + // NOTE: right now, we support of custom operations with spaces in them ([]) + // In the parameterless CustomOperationAttribute - we use the method name, and also allow it to be ````-quoted (member _.``foo bar`` _ = ...) + match attr with + // Empty string and parameterless constructor - we use the method name + | Attrib(unnamedArgs = [ AttribStringArg "" ]) // Empty string as parameter + | Attrib(unnamedArgs = []) -> // No parameters, same as empty string for compat reasons. + Some methInfo.LogicalName + // Use the specified name + | Attrib(unnamedArgs = [ AttribStringArg msg ]) -> Some msg + | _ -> None) + IgnoreAttribute // We do not respect this attribute for provided methods + let hasMethInfo nm cenv env mBuilderVal ad builderTy = match TryFindIntrinsicOrExtensionMethInfo ResultCollectionSettings.AtMostOneResult cenv env mBuilderVal ad nm builderTy with | [] -> false @@ -198,25 +218,7 @@ let getCustomOperationMethods (cenv: TcFileState) (env: TcEnv) ad mBuilderVal bu [ for methInfo in allMethInfos do if IsMethInfoAccessible cenv.amap mBuilderVal ad methInfo then - let nameSearch = - TryBindMethInfoAttribute - cenv.g - mBuilderVal - cenv.g.attrib_CustomOperationAttribute - methInfo - IgnoreAttribute // We do not respect this attribute for IL methods - (fun attr -> - // NOTE: right now, we support of custom operations with spaces in them ([]) - // In the parameterless CustomOperationAttribute - we use the method name, and also allow it to be ````-quoted (member _.``foo bar`` _ = ...) - match attr with - // Empty string and parameterless constructor - we use the method name - | Attrib(unnamedArgs = [ AttribStringArg "" ]) // Empty string as parameter - | Attrib(unnamedArgs = []) -> // No parameters, same as empty string for compat reasons. - Some methInfo.LogicalName - // Use the specified name - | Attrib(unnamedArgs = [ AttribStringArg msg ]) -> Some msg - | _ -> None) - IgnoreAttribute // We do not respect this attribute for provided methods + let nameSearch = TryGetCustomOperationName cenv.g mBuilderVal methInfo match nameSearch with | None -> () diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi index ac9554252f3..9486c1e13de 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi @@ -6,8 +6,14 @@ open FSharp.Compiler.CheckBasics open FSharp.Compiler.ConstraintSolver open FSharp.Compiler.Syntax open FSharp.Compiler.Text +open FSharp.Compiler.Infos +open FSharp.Compiler.TcGlobals open FSharp.Compiler.TypedTree +/// If the given method carries a [] attribute, return the operator name it declares +/// (the attribute argument, or the method's logical name when the attribute is parameterless/empty). +val TryGetCustomOperationName: g: TcGlobals -> m: range -> methInfo: MethInfo -> string option + val TcComputationExpression: cenv: TcFileState -> env: TcEnv -> diff --git a/src/Compiler/Service/ServiceDeclarationLists.fs b/src/Compiler/Service/ServiceDeclarationLists.fs index 98312a69306..15ca8eb2260 100644 --- a/src/Compiler/Service/ServiceDeclarationLists.fs +++ b/src/Compiler/Service/ServiceDeclarationLists.fs @@ -13,6 +13,8 @@ open Internal.Utilities.Library.Extras open FSharp.Compiler open FSharp.Compiler.AbstractIL.Diagnostics open FSharp.Compiler.AccessibilityLogic +open FSharp.Compiler.AttributeChecking +open FSharp.Compiler.CheckComputationExpressions open FSharp.Compiler.Diagnostics open FSharp.Compiler.EditorServices open FSharp.Compiler.DiagnosticsLogger @@ -31,6 +33,7 @@ open FSharp.Compiler.Text.TaggedText open FSharp.Compiler.TypedTree open FSharp.Compiler.TypedTreeBasics open FSharp.Compiler.TypedTreeOps +open FSharp.Compiler.TypeHierarchy /// A single data tip display element [] @@ -333,25 +336,55 @@ module DeclarationListHelpers = // Custom operations in queries | Item.CustomOperation (customOpName, usageText, Some minfo) -> + // Gather all sibling [] overloads declared on the same builder type, + // so that QuickInfo shows every overload (with the resolved one first) rather than only the one + // that name resolution happened to pick. See https://github.com/dotnet/fsharp/issues/11612. + let siblingMethInfos = + let enclTy = minfo.ApparentEnclosingType + // NOTE: extension-method-defined custom operators are not surfaced here. The type checker + // discovers them via AllMethInfosOfTypeInScope (which honours nenv); at tooltip time we + // only have the resolved minfo and intrinsic methods. This matches the common case + // (CustomOperation methods are nearly always intrinsic members of the builder type). + let allMeths = GetIntrinsicMethInfosOfType infoReader None ad AllowMultiIntfInstantiations.Yes IgnoreOverrides m enclTy + let matched = allMeths |> List.filter (fun mi -> TryGetCustomOperationName g m mi = Some customOpName) + let isResolved (mi: MethInfo) = MethInfosEquivByNameAndSig EraseAll true g amap m mi minfo + match List.tryFind isResolved matched with + | Some resolved -> resolved :: (matched |> List.filter (fun mi -> not (isResolved mi))) + | None -> if List.isEmpty matched then [minfo] else matched + // Build 'custom operation: where (bool) // - // Calls QueryBuilder.Where' - let layout = + // Calls QueryBuilder.Where' for each overload + let showInputTy = List.length siblingMethInfos > 1 + let layoutOne (mi: MethInfo) = + let inputTyL = + if not showInputTy then emptyL else + match mi.GetParamTypes(amap, m, mi.FormalMethodInst) with + | (firstTy :: _) :: _ -> + let firstTy, _ = PrettyTypes.PrettifyType g firstTy + SepL.colon ^^ layoutType denv firstTy + | _ -> emptyL wordL (tagText (FSComp.SR.typeInfoCustomOperation())) ^^ RightL.colon ^^ ( match usageText() with | Some t -> wordL (tagText t) | None -> - let argTys = ParamNameAndTypesOfUnaryCustomOperation g minfo |> List.map (fun (ParamNameAndType(_, ty)) -> ty) + let argTys = ParamNameAndTypesOfUnaryCustomOperation g mi |> List.map (fun (ParamNameAndType(_, ty)) -> ty) let argTys, _ = PrettyTypes.PrettifyTypes g argTys wordL (tagMethod customOpName) ^^ sepListL SepL.space (List.map (fun ty -> LeftL.leftParen ^^ layoutType denv ty ^^ SepL.rightParen) argTys) ) ^^ SepL.lineBreak ^^ SepL.lineBreak ^^ wordL (tagText (FSComp.SR.typeInfoCallsWord())) ^^ - layoutTyconRef denv minfo.ApparentEnclosingTyconRef ^^ + layoutTyconRef denv mi.ApparentEnclosingTyconRef ^^ SepL.dot ^^ - wordL (tagMethod minfo.DisplayName) + wordL (tagMethod mi.DisplayName) ^^ + inputTyL + + let layout = + siblingMethInfos + |> List.map layoutOne + |> List.reduce (fun a b -> a ^^ SepL.lineBreak ^^ SepL.lineBreak ^^ b) let layout = PrintUtilities.squashToWidth width layout let layout = toArray layout From 6c5c9518c27c40efa84ba0ea37fadab4af40a387 Mon Sep 17 00:00:00 2001 From: Copilot Date: Fri, 29 May 2026 20:53:34 +0200 Subject: [PATCH 03/15] Add PR link to release notes entry Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/release-notes/.FSharp.Compiler.Service/9.0.300.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md index 126a9858442..7287d250453 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md @@ -1,5 +1,5 @@ ### Fixed -* Fix QuickInfo for overloaded CE custom operators to show all overloads instead of always the last one. ([Issue #11612](https://github.com/dotnet/fsharp/issues/11612)) +* Fix QuickInfo for overloaded CE custom operators to show all overloads instead of always the last one. ([Issue #11612](https://github.com/dotnet/fsharp/issues/11612), [PR #19865](https://github.com/dotnet/fsharp/pull/19865)) * Fix missing TailCall warning in TOp.IntegerForLoop ([PR #18399](https://github.com/dotnet/fsharp/pull/18399)) * Fix classification of `nameof` in `nameof<'T>`, `match … with nameof ident -> …`. ([Issue #10026](https://github.com/dotnet/fsharp/issues/10026), [PR #18300](https://github.com/dotnet/fsharp/pull/18300)) * Fix Realsig+ generates nested closures with incorrect Generic ([Issue #17797](https://github.com/dotnet/fsharp/issues/17797), [PR #17877](https://github.com/dotnet/fsharp/pull/17877)) From de10d70fc0cd69beb9d92b516d3de980dac29ae1 Mon Sep 17 00:00:00 2001 From: Copilot Date: Fri, 29 May 2026 22:16:36 +0200 Subject: [PATCH 04/15] Move release notes entry for #11612 to 11.0.100.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/release-notes/.FSharp.Compiler.Service/11.0.100.md | 1 + docs/release-notes/.FSharp.Compiler.Service/9.0.300.md | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 43601b6e34c..60b7d53e1d0 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -67,6 +67,7 @@ * Reference assembly MVIDs are now deterministic across compiler invocations. Previously, `--refout` / `true` produced a different MVID every build because the implied signature hash used .NET's randomized `String.GetHashCode()`. ([Issue #19751](https://github.com/dotnet/fsharp/issues/19751), [PR #19801](https://github.com/dotnet/fsharp/pull/19801)) * Parser: recover on unfinished if and binary expressions ([PR #19724](https://github.com/dotnet/fsharp/pull/19724)) +* Fix QuickInfo for overloaded CE custom operators to show all overloads instead of always the last one. ([Issue #11612](https://github.com/dotnet/fsharp/issues/11612), [PR #19865](https://github.com/dotnet/fsharp/pull/19865)) ### Added diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md index 7287d250453..7f5ab7289d2 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md @@ -1,5 +1,4 @@ ### Fixed -* Fix QuickInfo for overloaded CE custom operators to show all overloads instead of always the last one. ([Issue #11612](https://github.com/dotnet/fsharp/issues/11612), [PR #19865](https://github.com/dotnet/fsharp/pull/19865)) * Fix missing TailCall warning in TOp.IntegerForLoop ([PR #18399](https://github.com/dotnet/fsharp/pull/18399)) * Fix classification of `nameof` in `nameof<'T>`, `match … with nameof ident -> …`. ([Issue #10026](https://github.com/dotnet/fsharp/issues/10026), [PR #18300](https://github.com/dotnet/fsharp/pull/18300)) * Fix Realsig+ generates nested closures with incorrect Generic ([Issue #17797](https://github.com/dotnet/fsharp/issues/17797), [PR #17877](https://github.com/dotnet/fsharp/pull/17877)) From 4ba013aff186c23faeda3710db84df44ea0aad53 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 05:09:22 +0200 Subject: [PATCH 05/15] Resolve CE [] overload sink to the actually-picked MethInfo (#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> --- .../.FSharp.Compiler.Service/11.0.100.md | 2 +- .../CheckComputationExpressions.fs | 249 ++++++++++++++++-- .../CheckComputationExpressions.fsi | 6 - .../Service/ServiceDeclarationLists.fs | 43 +-- .../TooltipTests.fs | 153 ++++++++--- 5 files changed, 354 insertions(+), 99 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 60b7d53e1d0..f17aaacfb0d 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -67,7 +67,7 @@ * Reference assembly MVIDs are now deterministic across compiler invocations. Previously, `--refout` / `true` produced a different MVID every build because the implied signature hash used .NET's randomized `String.GetHashCode()`. ([Issue #19751](https://github.com/dotnet/fsharp/issues/19751), [PR #19801](https://github.com/dotnet/fsharp/pull/19801)) * Parser: recover on unfinished if and binary expressions ([PR #19724](https://github.com/dotnet/fsharp/pull/19724)) -* Fix QuickInfo for overloaded CE custom operators to show all overloads instead of always the last one. ([Issue #11612](https://github.com/dotnet/fsharp/issues/11612), [PR #19865](https://github.com/dotnet/fsharp/pull/19865)) +* Fix QuickInfo / `GetAllUsesOfAllSymbolsInFile` for overloaded CE `[]` keywords to report the actually-resolved overload instead of the first-registered one. ([Issue #11612](https://github.com/dotnet/fsharp/issues/11612), [Issue #15206](https://github.com/dotnet/fsharp/issues/15206), [PR #19865](https://github.com/dotnet/fsharp/pull/19865)) ### Added diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index e1b0f1741fc..9753ebd00b1 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -13,6 +13,7 @@ open FSharp.Compiler.CheckBasics open FSharp.Compiler.ConstraintSolver open FSharp.Compiler.DiagnosticsLogger open FSharp.Compiler.Features +open FSharp.Compiler.Import open FSharp.Compiler.Infos open FSharp.Compiler.InfoReader open FSharp.Compiler.NameResolution @@ -21,6 +22,7 @@ open FSharp.Compiler.Syntax open FSharp.Compiler.SyntaxTrivia open FSharp.Compiler.Xml open FSharp.Compiler.SyntaxTreeOps +open FSharp.Compiler.TcGlobals open FSharp.Compiler.Text open FSharp.Compiler.Text.Range open FSharp.Compiler.TypedTree @@ -41,6 +43,36 @@ type CustomOperationsMode = | Allowed | Denied +/// Information about a `[]` keyword usage whose `Item.CustomOperation` +/// sink record needs to be deferred until normal overload resolution picks the actual +/// `MethInfo`. See https://github.com/dotnet/fsharp/issues/11612 and #15206. +[] +type DeferredCustomOpSink = + { + /// Source range of the operator keyword (where the tooltip / find-references will land). + KeywordRange: range + /// Custom operation name (the `[]` argument, or method name). + OpName: string + /// Usage-text factory for the rendered tooltip. + UsageText: unit -> string option + /// Synthetic range used by `mkSynCall` for the synthesized `builder.MethName(args)` call; + /// this is the range at which overload resolution will notify the chosen `MethInfo`. + SyntheticCallRange: range + /// Display name passed to `mkSynCall` (used to disambiguate the captured notification). + MethodDisplayName: string + /// All `MethInfo` candidates registered for this keyword (used to validate that the captured + /// resolution corresponds to one of the actual overloads). + Candidates: MethInfo list + /// Fallback `MethInfo` used when no resolution is captured (e.g. error-recovery paths). + Fallback: MethInfo + /// Name-environment in scope at the keyword (used for the eventual sink call). + NameEnv: NameResolutionEnv + /// Access rights in scope at the keyword. + AccessRights: AccessorDomain + /// Mutable cell into which the sink wrapper writes the resolved `MethInfo` (last-write-wins). + Resolved: (MethInfo * TyparInstantiation) option ref + } + [] type ComputationExpressionContext<'a> = { @@ -61,10 +93,55 @@ type ComputationExpressionContext<'a> = origComp: SynExpr mWhole: range emptyVarSpace: LazyWithContext * TcEnv, range> + /// Queue of custom-operation keyword usages whose `Item.CustomOperation` sink record + /// is deferred until after overload resolution of the desugared call. See #11612 / #15206. + deferredCustomOpSinks: ResizeArray } let inline noTailCall ceenv = { ceenv with tailCall = false } +/// Enqueue a deferred `Item.CustomOperation` sink record for `nm.idRange`. The actual sink +/// call will fire after overload resolution of the synthesized `mkSynCall` completes +/// (see `TcComputationExpression` for the wrapper that captures the resolution). +/// +/// The early sink call (with the `opDatas[0]` fallback `MethInfo`) still happens at the +/// original site so existing tests / consumers that depend on the early ordering of +/// `Item.CustomOperation` records in `TcResolutions` continue to work. The late drain only +/// fires `CallNameResolutionSinkReplacing` when the wrapper actually captured a resolution +/// different from the fallback. +let enqueueCustomOperationSink + (ceenv: ComputationExpressionContext<_>) + (nm: Ident) + opName + usageText + syntheticCallRange + methodDisplayName + candidates + (fallback: MethInfo) + = + // Record the early Item.CustomOperation sink call exactly as the original (pre-fix) code + // did, using opDatas[0] as the placeholder MethInfo. This preserves ordering for any + // consumer that walks TcResolutions and relies on the original sequence. + let fallbackItem = Item.CustomOperation(opName, usageText, Some fallback) + + CallNameResolutionSink + ceenv.cenv.tcSink + (nm.idRange, ceenv.env.NameEnv, fallbackItem, emptyTyparInst, ItemOccurrence.Use, ceenv.env.eAccessRights) + + ceenv.deferredCustomOpSinks.Add + { + KeywordRange = nm.idRange + OpName = opName + UsageText = usageText + SyntheticCallRange = syntheticCallRange + MethodDisplayName = methodDisplayName + Candidates = candidates + Fallback = fallback + NameEnv = ceenv.env.NameEnv + AccessRights = ceenv.env.eAccessRights + Resolved = ref None + } + let inline TryFindIntrinsicOrExtensionMethInfo collectionSettings (cenv: cenv) (env: TcEnv) m ad nm ty = AllMethInfosOfTypeInScope collectionSettings cenv.infoReader env.NameEnv (Some nm) ad IgnoreOverrides m ty |> List.filter (IsExtensionMethCompatibleWithTy cenv.infoReader m ty) @@ -1126,15 +1203,20 @@ let rec TryTranslateComputationExpression | Some opDatas -> let opName, _, _, _, _, _, _, _, methInfo = opDatas[0] - // Record the resolution of the custom operation for posterity - let item = - Item.CustomOperation(opName, (fun () -> customOpUsageText ceenv nm), Some methInfo) + // Defer the resolution of the custom operation sink record until after + // overload resolution. See enqueueCustomOperationSink for the rationale + // (https://github.com/dotnet/fsharp/issues/11612, #15206). + let candidates = opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi) -> mi) - // FUTURE: consider whether we can do better than emptyTyparInst here, in order to display instantiations - // of type variables in the quick info provided in the IDE. - CallNameResolutionSink - cenv.tcSink - (nm.idRange, ceenv.env.NameEnv, item, emptyTyparInst, ItemOccurrence.Use, ceenv.env.eAccessRights) + enqueueCustomOperationSink + ceenv + nm + opName + (fun () -> customOpUsageText ceenv nm) + (mOpCore.MakeSynthetic()) + methInfo.DisplayName + candidates + methInfo let mkJoinExpr keySelector1 keySelector2 innerPat e = let mSynthetic = mOpCore.MakeSynthetic() @@ -2431,15 +2513,22 @@ and ConsumeCustomOpClauses let isLikeGroupJoin = customOperationIsLikeZip ceenv nm - // Record the resolution of the custom operation for posterity - let item = - Item.CustomOperation(opName, (fun () -> customOpUsageText ceenv nm), Some methInfo) - - // FUTURE: consider whether we can do better than emptyTyparInst here, in order to display instantiations - // of type variables in the quick info provided in the IDE. - CallNameResolutionSink - ceenv.cenv.tcSink - (nm.idRange, ceenv.env.NameEnv, item, emptyTyparInst, ItemOccurrence.Use, ceenv.env.eAccessRights) + // Defer the resolution of the custom operation sink record until after overload + // resolution of the synthesized call has picked an overload. See #11612 / #15206. + // The fallback methInfo (opDatas[0]) is recorded if no resolution is captured (error + // recovery paths, e.g. isLikeZip/Join/GroupJoin or wrong-arg-count, may not produce + // a resolvable call). + let candidates = opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi) -> mi) + + enqueueCustomOperationSink + ceenv + nm + opName + (fun () -> customOpUsageText ceenv nm) + (mClause.MakeSynthetic()) + methInfo.DisplayName + candidates + methInfo if isLikeZip || isLikeJoin || isLikeGroupJoin then errorR (Error(FSComp.SR.tcBinaryOperatorRequiresBody (nm.idText, Option.get (customOpUsageText ceenv nm)), nm.idRange)) @@ -2952,6 +3041,94 @@ and TranslateComputationExpression (ceenv: ComputationExpressionContext<'a>) fir translatedCtxt fillExpr) +/// Sink wrapper used by `TcComputationExpression` to capture the resolved `MethInfo` +/// for synthesized custom-operation calls. The wrapper forwards every notification +/// to the original sink (captured **before** installation, to avoid recursion) and +/// in addition records the singleton method-group resolution that lands at one of +/// the tracked synthetic call ranges. +/// +/// Last-write-wins: type inference may notify the method group multiple times +/// (unrefined first, refined later via `AfterResolution.RecordResolution`); we +/// always keep the most recent singleton resolution that matches a candidate. +type private CustomOpResolutionCapturingSink + (g: TcGlobals, amap: ImportMap, m: range, forwardTo: ITypecheckResultsSink option, tracked: Dictionary) + = + + let matchesCandidate (mi: MethInfo) (candidates: MethInfo list) = + candidates + |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap m c mi) + + let tryCapture (range: range) (item: Item) (tpinst: TyparInstantiation) = + match tracked.TryGetValue range with + | false, _ -> () + | true, entries -> + match item with + | Item.MethodGroup(_, [ mi ], _) -> + for entry in entries do + if matchesCandidate mi entry.Candidates then + entry.Resolved.Value <- Some(mi, tpinst) + | _ -> () + + interface ITypecheckResultsSink with + member _.NotifyEnvWithScope(m, nenv, ad) = + forwardTo |> Option.iter (fun s -> s.NotifyEnvWithScope(m, nenv, ad)) + + member _.NotifyExprHasType(ty, nenv, ad, m) = + forwardTo |> Option.iter (fun s -> s.NotifyExprHasType(ty, nenv, ad, m)) + + member _.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) = + forwardTo + |> Option.iter (fun s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m)) + + member _.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) = + tryCapture m item tpinst + + forwardTo + |> Option.iter (fun s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace)) + + member _.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) = + tryCapture m item tpinst + + forwardTo + |> Option.iter (fun s -> + s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace)) + + member _.NotifyFormatSpecifierLocation(m, numArgs) = + forwardTo |> Option.iter (fun s -> s.NotifyFormatSpecifierLocation(m, numArgs)) + + member _.NotifyRelatedSymbolUse(m, item, kind) = + forwardTo |> Option.iter (fun s -> s.NotifyRelatedSymbolUse(m, item, kind)) + + member _.NotifyOpenDeclaration openDeclaration = + forwardTo |> Option.iter (fun s -> s.NotifyOpenDeclaration openDeclaration) + + member _.CurrentSourceText = + match forwardTo with + | Some s -> s.CurrentSourceText + | None -> None + + member _.FormatStringCheckContext = + match forwardTo with + | Some s -> s.FormatStringCheckContext + | None -> None + +/// Drain the deferred custom-operation sink queue: for each enqueued entry whose +/// wrapper captured an overload resolution that is *different* from the fallback that +/// was eagerly sunk in `enqueueCustomOperationSink`, replace the keyword's sink record +/// with one carrying the resolved `MethInfo` (and any captured `TyparInstantiation`). +/// Entries with no captured resolution, or whose captured resolution is signature- +/// equivalent to the fallback, are left as-is — the eagerly-sunk fallback is already +/// correct, and *not* calling `Replacing` here preserves the original sink ordering +/// that other tests / consumers rely on. +let private drainDeferredCustomOpSinks (g: TcGlobals) (amap: ImportMap) (sink: TcResultsSink) (entries: ResizeArray) = + for entry in entries do + match entry.Resolved.Value with + | Some(resolved, tpinst) when not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) -> + let item = Item.CustomOperation(entry.OpName, entry.UsageText, Some resolved) + + CallNameResolutionSinkReplacing sink (entry.KeywordRange, entry.NameEnv, item, tpinst, ItemOccurrence.Use, entry.AccessRights) + | _ -> () + /// Used for all computation expressions except sequence expressions let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv (mWhole, interpExpr: Expr, builderTy, comp: SynExpr) = let overallTy = overallTy.Commit @@ -3023,6 +3200,11 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv let origComp = comp + /// Queue of `Item.CustomOperation` sink records whose final `MethInfo` depends on + /// the overload resolution that happens inside the `TcExpr` call below. See + /// `enqueueCustomOperationSink` and `DeferredCustomOpSink`. + let deferredCustomOpSinks = ResizeArray() + let ceenv = { cenv = cenv @@ -3040,6 +3222,7 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv origComp = origComp mWhole = mWhole emptyVarSpace = LazyWithContext.NotLazy([], env) + deferredCustomOpSinks = deferredCustomOpSinks } /// Inside the 'query { ... }' use a modified name environment that contains fake 'CustomOperation' entries @@ -3113,7 +3296,37 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv | _ -> env let lambdaExpr, tpenv = - TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr + // Wrap cenv.tcSink with a local sink that captures the resolved MethInfo for each + // synthesized custom-operation call site. Forwarding goes through the *captured* + // old sink (oldSinkOpt), never through cenv.tcSink, to avoid recursion through + // the wrapper we just installed. See #11612 / #15206. + let oldSinkOpt = cenv.tcSink.CurrentSink + + let tracked = + let d = Dictionary(HashIdentity.Structural) + + for entry in deferredCustomOpSinks do + let existing = + match d.TryGetValue entry.SyntheticCallRange with + | true, xs -> xs + | false, _ -> [] + + d[entry.SyntheticCallRange] <- entry :: existing + + d + + let captureSink = + CustomOpResolutionCapturingSink(cenv.g, cenv.amap, mWhole, oldSinkOpt, tracked) :> ITypecheckResultsSink + + let lambdaExpr, tpenv = + use _holder = WithNewTypecheckResultsSink(captureSink, cenv.tcSink) + TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr + + // Drain deferred sink records now that overload resolution has captured each + // resolved MethInfo (or fell back to opDatas[0] for error-recovery paths). + drainDeferredCustomOpSinks cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks + + lambdaExpr, tpenv // For queries, transfer HasBeenReferenced from compiler-generated varSpace Vals to user Vals if isQuery then diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi index 9486c1e13de..ac9554252f3 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fsi @@ -6,14 +6,8 @@ open FSharp.Compiler.CheckBasics open FSharp.Compiler.ConstraintSolver open FSharp.Compiler.Syntax open FSharp.Compiler.Text -open FSharp.Compiler.Infos -open FSharp.Compiler.TcGlobals open FSharp.Compiler.TypedTree -/// If the given method carries a [] attribute, return the operator name it declares -/// (the attribute argument, or the method's logical name when the attribute is parameterless/empty). -val TryGetCustomOperationName: g: TcGlobals -> m: range -> methInfo: MethInfo -> string option - val TcComputationExpression: cenv: TcFileState -> env: TcEnv -> diff --git a/src/Compiler/Service/ServiceDeclarationLists.fs b/src/Compiler/Service/ServiceDeclarationLists.fs index 15ca8eb2260..98312a69306 100644 --- a/src/Compiler/Service/ServiceDeclarationLists.fs +++ b/src/Compiler/Service/ServiceDeclarationLists.fs @@ -13,8 +13,6 @@ open Internal.Utilities.Library.Extras open FSharp.Compiler open FSharp.Compiler.AbstractIL.Diagnostics open FSharp.Compiler.AccessibilityLogic -open FSharp.Compiler.AttributeChecking -open FSharp.Compiler.CheckComputationExpressions open FSharp.Compiler.Diagnostics open FSharp.Compiler.EditorServices open FSharp.Compiler.DiagnosticsLogger @@ -33,7 +31,6 @@ open FSharp.Compiler.Text.TaggedText open FSharp.Compiler.TypedTree open FSharp.Compiler.TypedTreeBasics open FSharp.Compiler.TypedTreeOps -open FSharp.Compiler.TypeHierarchy /// A single data tip display element [] @@ -336,55 +333,25 @@ module DeclarationListHelpers = // Custom operations in queries | Item.CustomOperation (customOpName, usageText, Some minfo) -> - // Gather all sibling [] overloads declared on the same builder type, - // so that QuickInfo shows every overload (with the resolved one first) rather than only the one - // that name resolution happened to pick. See https://github.com/dotnet/fsharp/issues/11612. - let siblingMethInfos = - let enclTy = minfo.ApparentEnclosingType - // NOTE: extension-method-defined custom operators are not surfaced here. The type checker - // discovers them via AllMethInfosOfTypeInScope (which honours nenv); at tooltip time we - // only have the resolved minfo and intrinsic methods. This matches the common case - // (CustomOperation methods are nearly always intrinsic members of the builder type). - let allMeths = GetIntrinsicMethInfosOfType infoReader None ad AllowMultiIntfInstantiations.Yes IgnoreOverrides m enclTy - let matched = allMeths |> List.filter (fun mi -> TryGetCustomOperationName g m mi = Some customOpName) - let isResolved (mi: MethInfo) = MethInfosEquivByNameAndSig EraseAll true g amap m mi minfo - match List.tryFind isResolved matched with - | Some resolved -> resolved :: (matched |> List.filter (fun mi -> not (isResolved mi))) - | None -> if List.isEmpty matched then [minfo] else matched - // Build 'custom operation: where (bool) // - // Calls QueryBuilder.Where' for each overload - let showInputTy = List.length siblingMethInfos > 1 - let layoutOne (mi: MethInfo) = - let inputTyL = - if not showInputTy then emptyL else - match mi.GetParamTypes(amap, m, mi.FormalMethodInst) with - | (firstTy :: _) :: _ -> - let firstTy, _ = PrettyTypes.PrettifyType g firstTy - SepL.colon ^^ layoutType denv firstTy - | _ -> emptyL + // Calls QueryBuilder.Where' + let layout = wordL (tagText (FSComp.SR.typeInfoCustomOperation())) ^^ RightL.colon ^^ ( match usageText() with | Some t -> wordL (tagText t) | None -> - let argTys = ParamNameAndTypesOfUnaryCustomOperation g mi |> List.map (fun (ParamNameAndType(_, ty)) -> ty) + let argTys = ParamNameAndTypesOfUnaryCustomOperation g minfo |> List.map (fun (ParamNameAndType(_, ty)) -> ty) let argTys, _ = PrettyTypes.PrettifyTypes g argTys wordL (tagMethod customOpName) ^^ sepListL SepL.space (List.map (fun ty -> LeftL.leftParen ^^ layoutType denv ty ^^ SepL.rightParen) argTys) ) ^^ SepL.lineBreak ^^ SepL.lineBreak ^^ wordL (tagText (FSComp.SR.typeInfoCallsWord())) ^^ - layoutTyconRef denv mi.ApparentEnclosingTyconRef ^^ + layoutTyconRef denv minfo.ApparentEnclosingTyconRef ^^ SepL.dot ^^ - wordL (tagMethod mi.DisplayName) ^^ - inputTyL - - let layout = - siblingMethInfos - |> List.map layoutOne - |> List.reduce (fun a b -> a ^^ SepL.lineBreak ^^ SepL.lineBreak ^^ b) + wordL (tagMethod minfo.DisplayName) let layout = PrintUtilities.squashToWidth width layout let layout = toArray layout diff --git a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs index 7220147402d..dff8a97a592 100644 --- a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs @@ -4,6 +4,7 @@ #nowarn "57" open FSharp.Compiler.CodeAnalysis +open FSharp.Compiler.Diagnostics open FSharp.Compiler.Service.Tests.Common open FSharp.Compiler.Text open FSharp.Compiler.EditorServices @@ -853,9 +854,14 @@ let inline fo{caret}o< ^T> (x: ^T) = x // Type param appears in tooltip Assert.Contains("'T", text) -// https://github.com/dotnet/fsharp/issues/11612 -// Tooltip for overloaded CE [] should reflect the resolved overload, -// not just the last one registered with the builder. +// https://github.com/dotnet/fsharp/issues/11612 / #15206 +// QuickInfo / SymbolUse for an overloaded CE [] keyword must reflect the +// overload picked by normal F# overload resolution, not the first-registered one (which +// corresponded to the last declared overload of the builder). +// +// NOTE: F# only allows overloaded custom operations whose underlying CLR members share the +// same name (overloading is by signature, not by member name). Tests below use a single +// shared method name `Filter` to mirror what the language actually supports. let private renderAllGroups (ToolTipText elements) = let sb = System.Text.StringBuilder() for el in elements do @@ -865,45 +871,53 @@ let private renderAllGroups (ToolTipText elements) = for line in item.MainDescription do sb.Append(line.Text) |> ignore sb.Append('\n') |> ignore + for line in item.XmlDoc |> (function FSharpXmlDoc.FromXmlText t -> t.UnprocessedLines |> Array.toList | _ -> []) do + sb.AppendLine(line) |> ignore | ToolTipElement.CompositionError msg -> sb.AppendLine(msg) |> ignore | ToolTipElement.None -> () sb.ToString() [] -let ``CE custom operator QuickInfo shows resolved overload`` () = - Checker.getTooltip """ +let ``CE custom operator QuickInfo XmlDoc reflects resolved int overload`` () = + let text = + Checker.getTooltip """ type Builder() = member _.Yield(x) = [x] member _.For(xs, body) = xs |> List.collect body - [] - member _.WhereInt(xs: int list, [] f: int -> bool) = List.filter f xs - [] - member _.WhereStr(xs: string list, [] f: string -> bool) = List.filter f xs + /// INT_OVERLOAD_MARKER + [] + member _.Filter(xs: int list, [] f: int -> bool) = List.filter f xs + /// STRING_OVERLOAD_MARKER + [] + member _.Filter(xs: string list, [] f: string -> bool) = List.filter f xs let b = Builder() -let result = b { for x in [1;2;3] do whereO{caret}p (x > 0) } +let result = b { for x in [1;2;3] do filterO{caret}p (x > 0) } """ - |> renderAllGroups - |> fun text -> Assert.Contains("int", text) + |> renderAllGroups + Assert.Contains("INT_OVERLOAD_MARKER", text) + Assert.DoesNotContain("STRING_OVERLOAD_MARKER", text) [] -let ``CE custom operator with three overloads shows resolved float overload`` () = - Checker.getTooltip """ +let ``CE custom operator QuickInfo XmlDoc reflects resolved string overload`` () = + let text = + Checker.getTooltip """ type Builder() = member _.Yield(x) = [x] member _.For(xs, body) = xs |> List.collect body + /// INT_OVERLOAD_MARKER [] - member _.F1(xs: int list, [] f: int -> bool) = List.filter f xs - [] - member _.F2(xs: float list, [] f: float -> bool) = List.filter f xs + member _.Filter(xs: int list, [] f: int -> bool) = List.filter f xs + /// STRING_OVERLOAD_MARKER [] - member _.F3(xs: string list, [] f: string -> bool) = List.filter f xs + member _.Filter(xs: string list, [] f: string -> bool) = List.filter f xs let b = Builder() -let result = b { for x in [1.0;2.0] do filterO{caret}p (x > 0.0) } +let result = b { for x in ["a";"b"] do filterO{caret}p (x.Length > 0) } """ - |> renderAllGroups - |> fun text -> Assert.Contains("float", text) + |> renderAllGroups + Assert.Contains("STRING_OVERLOAD_MARKER", text) + Assert.DoesNotContain("INT_OVERLOAD_MARKER", text) [] let ``CE single custom operator QuickInfo still works`` () = @@ -932,19 +946,86 @@ let r = t.M{caret}(42) |> renderAllGroups |> fun text -> Assert.Contains("int", text) +// https://github.com/dotnet/fsharp/issues/15206 +// GetAllUsesOfAllSymbolsInFile must report the resolved overload's MethInfo for each +// keyword usage, not the first-registered one. [] -let ``GetSymbolUse resolves correct CE operator overload`` () = - Checker.getTooltip """ -type Builder() = - member _.Yield(x) = [x] - member _.For(xs, body) = xs |> List.collect body - [] - member _.PickInt(xs: int list, [] f: int -> bool) = List.filter f xs - [] - member _.PickStr(xs: string list, [] f: string -> bool) = List.filter f xs - -let b = Builder() -let result = b { for x in [1;2;3] do pickO{caret}ne (x > 0) } -""" - |> renderAllGroups - |> fun text -> Assert.Contains("int", text) \ No newline at end of file +let ``GetAllUsesOfAllSymbolsInFile reports resolved overload for each CE custom operation use`` () = + let source = """ +module M +type FooBuilder() = + member _.Yield _ = 1 + member _.For(xs, body) = xs |> Seq.iter body + [] + member _.Create(_, i1: int, s: string, i2: int) = [i1; i2] + [] + member _.Create(_, i: int, s1: string, s2: string) = [i; s1.Length; s2.Length] + +let b = FooBuilder() +let _ = b { for x in [1] do create 1 "" 2 } +let _ = b { for x in [1] do create 1 "" "" } +""" + let _, checkResults = getParseAndCheckResults source + + // Find the two usages of the `create` keyword. + let createKeywordUses = + checkResults.GetAllUsesOfAllSymbolsInFile() + |> Seq.filter (fun u -> + match u.Symbol with + | :? FSharpMemberOrFunctionOrValue as mfv -> + mfv.LogicalName = "Create" + && (u.Range.StartLine = 12 || u.Range.StartLine = 13) + && not u.IsFromDefinition + | _ -> false) + |> Seq.sortBy (fun u -> u.Range.StartLine) + |> List.ofSeq + + Assert.Equal(2, createKeywordUses.Length) + + let lastParamType (mfv: FSharpMemberOrFunctionOrValue) = + mfv.CurriedParameterGroups + |> Seq.collect id + |> Seq.last + |> fun p -> p.Type.Format(FSharpDisplayContext.Empty) + + // First usage: 'create 1 "" 2' should resolve to the (int, string, int) overload. + let firstMfv = createKeywordUses[0].Symbol :?> FSharpMemberOrFunctionOrValue + Assert.Contains("int", lastParamType firstMfv) + + // Second usage: 'create 1 "" ""' should resolve to the (int, string, string) overload. + let secondMfv = createKeywordUses[1].Symbol :?> FSharpMemberOrFunctionOrValue + Assert.Contains("string", lastParamType secondMfv) + + // Sanity: each usage must resolve to a *different* overload (different MethInfo). + Assert.NotEqual(lastParamType firstMfv, lastParamType secondMfv) + +// Coverage for the join/zip/groupJoin code path which uses mkJoinExpr/mkZipExpr instead +// of the unary ConsumeCustomOpClauses translation. The fix routes that path through the +// same deferred-sink mechanism. This regression test is conservative: it asserts that if +// the CE source happens to resolve and produce two symbol uses, they resolve to the +// correct distinct overloads. It is skipped otherwise (precise join-like CE syntax is +// finicky and not the focus of #11612 / #15206). +[] +let ``GetAllUsesOfAllSymbolsInFile reports resolved overload for join-like CE custom operation`` () = + let source = """ +module M +type ZBuilder() = + member _.Source(xs: int seq) = xs + member _.Source(xs: string seq) = xs + member _.For(xs, body) = xs |> Seq.collect body + member _.Yield(x) = Seq.singleton x + [] + member _.MyZip(xs: int seq, ys: int seq, f: int -> int -> int) = + Seq.map2 f xs ys + [] + member _.MyZip(xs: string seq, ys: string seq, f: string -> string -> string) = + Seq.map2 f xs ys + +let _ = ZBuilder() +""" + let _, checkResults = getParseAndCheckResults source + // Smoke test: typechecking the builder with overloaded IsLikeZip [] + // methods must not crash. The CE call site is intentionally elided; this test exists + // to ensure the deferred-sink mechanism does not break the join/zip/groupJoin discovery. + let diags = checkResults.Diagnostics + Assert.DoesNotContain(diags, fun d -> d.Severity = FSharpDiagnosticSeverity.Error) \ No newline at end of file From 0eb958f5eca712f909c9d5bf414c617db6ce0f36 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 09:35:54 +0200 Subject: [PATCH 06/15] Apply review feedback: simplify CE custom-op deferred sink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address findings from three concurrent reviews (Opus 4.7 high, GPT-5.5, Opus 4.8) of commit 4ba013aff1: * 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 [] 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> --- .../CheckComputationExpressions.fs | 128 ++++++++---------- .../TooltipTests.fs | 34 +---- 2 files changed, 59 insertions(+), 103 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index 9753ebd00b1..9caa4f2bdb5 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -58,8 +58,6 @@ type DeferredCustomOpSink = /// Synthetic range used by `mkSynCall` for the synthesized `builder.MethName(args)` call; /// this is the range at which overload resolution will notify the chosen `MethInfo`. SyntheticCallRange: range - /// Display name passed to `mkSynCall` (used to disambiguate the captured notification). - MethodDisplayName: string /// All `MethInfo` candidates registered for this keyword (used to validate that the captured /// resolution corresponds to one of the actual overloads). Candidates: MethInfo list @@ -70,7 +68,7 @@ type DeferredCustomOpSink = /// Access rights in scope at the keyword. AccessRights: AccessorDomain /// Mutable cell into which the sink wrapper writes the resolved `MethInfo` (last-write-wins). - Resolved: (MethInfo * TyparInstantiation) option ref + mutable Resolved: (MethInfo * TyparInstantiation) option } [] @@ -115,7 +113,6 @@ let enqueueCustomOperationSink opName usageText syntheticCallRange - methodDisplayName candidates (fallback: MethInfo) = @@ -134,12 +131,11 @@ let enqueueCustomOperationSink OpName = opName UsageText = usageText SyntheticCallRange = syntheticCallRange - MethodDisplayName = methodDisplayName Candidates = candidates Fallback = fallback NameEnv = ceenv.env.NameEnv AccessRights = ceenv.env.eAccessRights - Resolved = ref None + Resolved = None } let inline TryFindIntrinsicOrExtensionMethInfo collectionSettings (cenv: cenv) (env: TcEnv) m ad nm ty = @@ -1214,7 +1210,6 @@ let rec TryTranslateComputationExpression opName (fun () -> customOpUsageText ceenv nm) (mOpCore.MakeSynthetic()) - methInfo.DisplayName candidates methInfo @@ -2520,15 +2515,7 @@ and ConsumeCustomOpClauses // a resolvable call). let candidates = opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi) -> mi) - enqueueCustomOperationSink - ceenv - nm - opName - (fun () -> customOpUsageText ceenv nm) - (mClause.MakeSynthetic()) - methInfo.DisplayName - candidates - methInfo + enqueueCustomOperationSink ceenv nm opName (fun () -> customOpUsageText ceenv nm) (mClause.MakeSynthetic()) candidates methInfo if isLikeZip || isLikeJoin || isLikeGroupJoin then errorR (Error(FSComp.SR.tcBinaryOperatorRequiresBody (nm.idText, Option.get (customOpUsageText ceenv nm)), nm.idRange)) @@ -3051,66 +3038,61 @@ and TranslateComputationExpression (ceenv: ComputationExpressionContext<'a>) fir /// (unrefined first, refined later via `AfterResolution.RecordResolution`); we /// always keep the most recent singleton resolution that matches a candidate. type private CustomOpResolutionCapturingSink - (g: TcGlobals, amap: ImportMap, m: range, forwardTo: ITypecheckResultsSink option, tracked: Dictionary) - = + ( + g: TcGlobals, + amap: ImportMap, + m: range, + forwardTo: ITypecheckResultsSink option, + deferredSinksBySyntheticRange: Dictionary + ) = let matchesCandidate (mi: MethInfo) (candidates: MethInfo list) = candidates |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap m c mi) let tryCapture (range: range) (item: Item) (tpinst: TyparInstantiation) = - match tracked.TryGetValue range with - | false, _ -> () - | true, entries -> - match item with - | Item.MethodGroup(_, [ mi ], _) -> - for entry in entries do - if matchesCandidate mi entry.Candidates then - entry.Resolved.Value <- Some(mi, tpinst) - | _ -> () + match deferredSinksBySyntheticRange.TryGetValue range, item with + | (true, entries), Item.MethodGroup(_, [ mi ], _) -> + for entry in entries do + if matchesCandidate mi entry.Candidates then + entry.Resolved <- Some(mi, tpinst) + | _ -> () + + let forward f = forwardTo |> Option.iter f interface ITypecheckResultsSink with member _.NotifyEnvWithScope(m, nenv, ad) = - forwardTo |> Option.iter (fun s -> s.NotifyEnvWithScope(m, nenv, ad)) + forward (fun s -> s.NotifyEnvWithScope(m, nenv, ad)) member _.NotifyExprHasType(ty, nenv, ad, m) = - forwardTo |> Option.iter (fun s -> s.NotifyExprHasType(ty, nenv, ad, m)) + forward (fun s -> s.NotifyExprHasType(ty, nenv, ad, m)) member _.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) = - forwardTo - |> Option.iter (fun s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m)) + forward (fun s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m)) member _.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - - forwardTo - |> Option.iter (fun s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace)) + forward (fun s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace)) member _.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - forwardTo - |> Option.iter (fun s -> + forward (fun s -> s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace)) member _.NotifyFormatSpecifierLocation(m, numArgs) = - forwardTo |> Option.iter (fun s -> s.NotifyFormatSpecifierLocation(m, numArgs)) + forward (fun s -> s.NotifyFormatSpecifierLocation(m, numArgs)) member _.NotifyRelatedSymbolUse(m, item, kind) = - forwardTo |> Option.iter (fun s -> s.NotifyRelatedSymbolUse(m, item, kind)) + forward (fun s -> s.NotifyRelatedSymbolUse(m, item, kind)) member _.NotifyOpenDeclaration openDeclaration = - forwardTo |> Option.iter (fun s -> s.NotifyOpenDeclaration openDeclaration) + forward (fun s -> s.NotifyOpenDeclaration openDeclaration) - member _.CurrentSourceText = - match forwardTo with - | Some s -> s.CurrentSourceText - | None -> None + member _.CurrentSourceText = forwardTo |> Option.bind (fun s -> s.CurrentSourceText) member _.FormatStringCheckContext = - match forwardTo with - | Some s -> s.FormatStringCheckContext - | None -> None + forwardTo |> Option.bind (fun s -> s.FormatStringCheckContext) /// Drain the deferred custom-operation sink queue: for each enqueued entry whose /// wrapper captured an overload resolution that is *different* from the fallback that @@ -3122,7 +3104,7 @@ type private CustomOpResolutionCapturingSink /// that other tests / consumers rely on. let private drainDeferredCustomOpSinks (g: TcGlobals) (amap: ImportMap) (sink: TcResultsSink) (entries: ResizeArray) = for entry in entries do - match entry.Resolved.Value with + match entry.Resolved with | Some(resolved, tpinst) when not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) -> let item = Item.CustomOperation(entry.OpName, entry.UsageText, Some resolved) @@ -3296,37 +3278,43 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv | _ -> env let lambdaExpr, tpenv = - // Wrap cenv.tcSink with a local sink that captures the resolved MethInfo for each - // synthesized custom-operation call site. Forwarding goes through the *captured* - // old sink (oldSinkOpt), never through cenv.tcSink, to avoid recursion through - // the wrapper we just installed. See #11612 / #15206. - let oldSinkOpt = cenv.tcSink.CurrentSink + if deferredCustomOpSinks.Count = 0 then + // No custom-operation keyword usages — skip the sink wrapping/draining overhead + // entirely. This is the common case for async/task/seq/option/list CEs. + TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr + else + // Wrap cenv.tcSink with a local sink that captures the resolved MethInfo for each + // synthesized custom-operation call site. Forwarding goes through the *captured* + // old sink (oldSinkOpt), never through cenv.tcSink, to avoid recursion through + // the wrapper we just installed. See #11612 / #15206. + let oldSinkOpt = cenv.tcSink.CurrentSink - let tracked = - let d = Dictionary(HashIdentity.Structural) + let deferredSinksBySyntheticRange = + let d = Dictionary(HashIdentity.Structural) - for entry in deferredCustomOpSinks do - let existing = - match d.TryGetValue entry.SyntheticCallRange with - | true, xs -> xs - | false, _ -> [] + for entry in deferredCustomOpSinks do + let existing = + match d.TryGetValue entry.SyntheticCallRange with + | true, xs -> xs + | false, _ -> [] - d[entry.SyntheticCallRange] <- entry :: existing + d[entry.SyntheticCallRange] <- entry :: existing - d + d - let captureSink = - CustomOpResolutionCapturingSink(cenv.g, cenv.amap, mWhole, oldSinkOpt, tracked) :> ITypecheckResultsSink + let captureSink = + CustomOpResolutionCapturingSink(cenv.g, cenv.amap, mWhole, oldSinkOpt, deferredSinksBySyntheticRange) + :> ITypecheckResultsSink - let lambdaExpr, tpenv = - use _holder = WithNewTypecheckResultsSink(captureSink, cenv.tcSink) - TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr + let lambdaExpr, tpenv = + use _holder = WithNewTypecheckResultsSink(captureSink, cenv.tcSink) + TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr - // Drain deferred sink records now that overload resolution has captured each - // resolved MethInfo (or fell back to opDatas[0] for error-recovery paths). - drainDeferredCustomOpSinks cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks + // Drain deferred sink records now that overload resolution has captured each + // resolved MethInfo (or fell back to opDatas[0] for error-recovery paths). + drainDeferredCustomOpSinks cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks - lambdaExpr, tpenv + lambdaExpr, tpenv // For queries, transfer HasBeenReferenced from compiler-generated varSpace Vals to user Vals if isQuery then diff --git a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs index dff8a97a592..2643f4d7b54 100644 --- a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs @@ -4,7 +4,6 @@ #nowarn "57" open FSharp.Compiler.CodeAnalysis -open FSharp.Compiler.Diagnostics open FSharp.Compiler.Service.Tests.Common open FSharp.Compiler.Text open FSharp.Compiler.EditorServices @@ -997,35 +996,4 @@ let _ = b { for x in [1] do create 1 "" "" } Assert.Contains("string", lastParamType secondMfv) // Sanity: each usage must resolve to a *different* overload (different MethInfo). - Assert.NotEqual(lastParamType firstMfv, lastParamType secondMfv) - -// Coverage for the join/zip/groupJoin code path which uses mkJoinExpr/mkZipExpr instead -// of the unary ConsumeCustomOpClauses translation. The fix routes that path through the -// same deferred-sink mechanism. This regression test is conservative: it asserts that if -// the CE source happens to resolve and produce two symbol uses, they resolve to the -// correct distinct overloads. It is skipped otherwise (precise join-like CE syntax is -// finicky and not the focus of #11612 / #15206). -[] -let ``GetAllUsesOfAllSymbolsInFile reports resolved overload for join-like CE custom operation`` () = - let source = """ -module M -type ZBuilder() = - member _.Source(xs: int seq) = xs - member _.Source(xs: string seq) = xs - member _.For(xs, body) = xs |> Seq.collect body - member _.Yield(x) = Seq.singleton x - [] - member _.MyZip(xs: int seq, ys: int seq, f: int -> int -> int) = - Seq.map2 f xs ys - [] - member _.MyZip(xs: string seq, ys: string seq, f: string -> string -> string) = - Seq.map2 f xs ys - -let _ = ZBuilder() -""" - let _, checkResults = getParseAndCheckResults source - // Smoke test: typechecking the builder with overloaded IsLikeZip [] - // methods must not crash. The CE call site is intentionally elided; this test exists - // to ensure the deferred-sink mechanism does not break the join/zip/groupJoin discovery. - let diags = checkResults.Diagnostics - Assert.DoesNotContain(diags, fun d -> d.Severity = FSharpDiagnosticSeverity.Error) \ No newline at end of file + Assert.NotEqual(lastParamType firstMfv, lastParamType secondMfv) \ No newline at end of file From 2d9b6bc7883e325ba78d78ff57e3cfd07471fe6c Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 11:12:30 +0200 Subject: [PATCH 07/15] Second review round: extract helpers, drop unused params, restore join test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../CheckComputationExpressions.fs | 89 ++++++++++++------- .../TooltipTests.fs | 58 +++++++++++- 2 files changed, 116 insertions(+), 31 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index 9caa4f2bdb5..2b5eee14f87 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -98,6 +98,11 @@ type ComputationExpressionContext<'a> = let inline noTailCall ceenv = { ceenv with tailCall = false } +/// Project the `MethInfo` out of every `opData` tuple returned by `getCustomOperationMethods` +/// (a 9-tuple whose last field is the `MethInfo`). +let inline methInfosOfOpDatas opDatas = + opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi: MethInfo) -> mi) + /// Enqueue a deferred `Item.CustomOperation` sink record for `nm.idRange`. The actual sink /// call will fire after overload resolution of the synthesized `mkSynCall` completes /// (see `TcComputationExpression` for the wrapper that captures the resolution). @@ -1202,15 +1207,13 @@ let rec TryTranslateComputationExpression // Defer the resolution of the custom operation sink record until after // overload resolution. See enqueueCustomOperationSink for the rationale // (https://github.com/dotnet/fsharp/issues/11612, #15206). - let candidates = opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi) -> mi) - enqueueCustomOperationSink ceenv nm opName (fun () -> customOpUsageText ceenv nm) (mOpCore.MakeSynthetic()) - candidates + (methInfosOfOpDatas opDatas) methInfo let mkJoinExpr keySelector1 keySelector2 innerPat e = @@ -2513,9 +2516,14 @@ and ConsumeCustomOpClauses // The fallback methInfo (opDatas[0]) is recorded if no resolution is captured (error // recovery paths, e.g. isLikeZip/Join/GroupJoin or wrong-arg-count, may not produce // a resolvable call). - let candidates = opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi) -> mi) - - enqueueCustomOperationSink ceenv nm opName (fun () -> customOpUsageText ceenv nm) (mClause.MakeSynthetic()) candidates methInfo + enqueueCustomOperationSink + ceenv + nm + opName + (fun () -> customOpUsageText ceenv nm) + (mClause.MakeSynthetic()) + (methInfosOfOpDatas opDatas) + methInfo if isLikeZip || isLikeJoin || isLikeGroupJoin then errorR (Error(FSComp.SR.tcBinaryOperatorRequiresBody (nm.idText, Option.get (customOpUsageText ceenv nm)), nm.idRange)) @@ -3041,53 +3049,66 @@ type private CustomOpResolutionCapturingSink ( g: TcGlobals, amap: ImportMap, - m: range, forwardTo: ITypecheckResultsSink option, deferredSinksBySyntheticRange: Dictionary ) = - let matchesCandidate (mi: MethInfo) (candidates: MethInfo list) = - candidates - |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap m c mi) + let matchesCandidate (mi: MethInfo) (entry: DeferredCustomOpSink) = + entry.Candidates + |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange c mi) - let tryCapture (range: range) (item: Item) (tpinst: TyparInstantiation) = - match deferredSinksBySyntheticRange.TryGetValue range, item with + let tryCapture (m: range) (item: Item) (tpinst: TyparInstantiation) = + match deferredSinksBySyntheticRange.TryGetValue m, item with | (true, entries), Item.MethodGroup(_, [ mi ], _) -> for entry in entries do - if matchesCandidate mi entry.Candidates then + if matchesCandidate mi entry then entry.Resolved <- Some(mi, tpinst) | _ -> () - let forward f = forwardTo |> Option.iter f - interface ITypecheckResultsSink with member _.NotifyEnvWithScope(m, nenv, ad) = - forward (fun s -> s.NotifyEnvWithScope(m, nenv, ad)) + match forwardTo with + | Some s -> s.NotifyEnvWithScope(m, nenv, ad) + | None -> () member _.NotifyExprHasType(ty, nenv, ad, m) = - forward (fun s -> s.NotifyExprHasType(ty, nenv, ad, m)) + match forwardTo with + | Some s -> s.NotifyExprHasType(ty, nenv, ad, m) + | None -> () member _.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) = - forward (fun s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m)) + match forwardTo with + | Some s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) + | None -> () member _.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - forward (fun s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace)) + + match forwardTo with + | Some s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) + | None -> () member _.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - forward (fun s -> - s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace)) + match forwardTo with + | Some s -> s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) + | None -> () member _.NotifyFormatSpecifierLocation(m, numArgs) = - forward (fun s -> s.NotifyFormatSpecifierLocation(m, numArgs)) + match forwardTo with + | Some s -> s.NotifyFormatSpecifierLocation(m, numArgs) + | None -> () member _.NotifyRelatedSymbolUse(m, item, kind) = - forward (fun s -> s.NotifyRelatedSymbolUse(m, item, kind)) + match forwardTo with + | Some s -> s.NotifyRelatedSymbolUse(m, item, kind) + | None -> () member _.NotifyOpenDeclaration openDeclaration = - forward (fun s -> s.NotifyOpenDeclaration openDeclaration) + match forwardTo with + | Some s -> s.NotifyOpenDeclaration openDeclaration + | None -> () member _.CurrentSourceText = forwardTo |> Option.bind (fun s -> s.CurrentSourceText) @@ -3103,9 +3124,12 @@ type private CustomOpResolutionCapturingSink /// correct, and *not* calling `Replacing` here preserves the original sink ordering /// that other tests / consumers rely on. let private drainDeferredCustomOpSinks (g: TcGlobals) (amap: ImportMap) (sink: TcResultsSink) (entries: ResizeArray) = + let isDifferentOverload (entry: DeferredCustomOpSink) (resolved: MethInfo) = + not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) + for entry in entries do match entry.Resolved with - | Some(resolved, tpinst) when not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) -> + | Some(resolved, tpinst) when isDifferentOverload entry resolved -> let item = Item.CustomOperation(entry.OpName, entry.UsageText, Some resolved) CallNameResolutionSinkReplacing sink (entry.KeywordRange, entry.NameEnv, item, tpinst, ItemOccurrence.Use, entry.AccessRights) @@ -3278,15 +3302,21 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv | _ -> env let lambdaExpr, tpenv = - if deferredCustomOpSinks.Count = 0 then - // No custom-operation keyword usages — skip the sink wrapping/draining overhead - // entirely. This is the common case for async/task/seq/option/list CEs. + // Skip the sink wrapping/draining overhead entirely when either: + // - there are no custom-operation keyword usages (most async/task/seq/option/list CEs), or + // - no IDE sink is listening (e.g. plain `dotnet build`), so the resolved sink record + // we would replay has no consumer. + if deferredCustomOpSinks.Count = 0 || cenv.tcSink.CurrentSink.IsNone then TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr else // Wrap cenv.tcSink with a local sink that captures the resolved MethInfo for each // synthesized custom-operation call site. Forwarding goes through the *captured* // old sink (oldSinkOpt), never through cenv.tcSink, to avoid recursion through // the wrapper we just installed. See #11612 / #15206. + // + // Nested CEs: an inner `TcComputationExpression` will capture *this* wrapper as + // its own `oldSinkOpt` and chain outer→inner correctly. Do not "optimise" by + // forwarding directly to `cenv.tcSink` here — that would recurse into the wrapper. let oldSinkOpt = cenv.tcSink.CurrentSink let deferredSinksBySyntheticRange = @@ -3303,8 +3333,7 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv d let captureSink = - CustomOpResolutionCapturingSink(cenv.g, cenv.amap, mWhole, oldSinkOpt, deferredSinksBySyntheticRange) - :> ITypecheckResultsSink + CustomOpResolutionCapturingSink(cenv.g, cenv.amap, oldSinkOpt, deferredSinksBySyntheticRange) :> ITypecheckResultsSink let lambdaExpr, tpenv = use _holder = WithNewTypecheckResultsSink(captureSink, cenv.tcSink) diff --git a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs index 2643f4d7b54..80c2c95f5aa 100644 --- a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs @@ -996,4 +996,60 @@ let _ = b { for x in [1] do create 1 "" "" } Assert.Contains("string", lastParamType secondMfv) // Sanity: each usage must resolve to a *different* overload (different MethInfo). - Assert.NotEqual(lastParamType firstMfv, lastParamType secondMfv) \ No newline at end of file + Assert.NotEqual(lastParamType firstMfv, lastParamType secondMfv) + +// Covers the join/zip/groupJoin code path which enqueues with `mOpCore.MakeSynthetic()` +// in `mkJoinExpr`/`mkZipExpr` rather than `mClause.MakeSynthetic()` used by the unary +// `ConsumeCustomOpClauses` path. Validates that the deferred-sink mechanism also picks +// the resolved overload here. +[] +let ``GetAllUsesOfAllSymbolsInFile reports resolved overload for IsLikeZip CE custom operation`` () = + let source = """ +module M +type ZBuilder() = + member _.Yield (x: 'a) = [x] + member _.For(xs: 'a list, body: 'a -> 'b list) = xs |> List.collect body + [] + member _.Select(xs: 'a list, [] f: 'a -> 'b) = List.map f xs + [] + member _.MyZip(outer: int list, inner: int list, resultSelector: int -> int -> 'r) = + List.map2 resultSelector outer inner + [] + member _.MyZip(outer: string list, inner: string list, resultSelector: string -> string -> 'r) = + List.map2 resultSelector outer inner + +let b = ZBuilder() +let _ = b { for x in [1;2] do + myzip y in [3;4] + select (x + y) } +let _ = b { for x in ["a";"b"] do + myzip y in ["c";"d"] + select (x + y) } +""" + let _, checkResults = getParseAndCheckResults source + + let myzipKeywordUses = + checkResults.GetAllUsesOfAllSymbolsInFile() + |> Seq.filter (fun u -> + match u.Symbol with + | :? FSharpMemberOrFunctionOrValue as mfv -> + mfv.LogicalName = "MyZip" && not u.IsFromDefinition + | _ -> false) + |> Seq.sortBy (fun u -> u.Range.StartLine) + |> List.ofSeq + + Assert.Equal(2, myzipKeywordUses.Length) + + let firstParamType (mfv: FSharpMemberOrFunctionOrValue) = + mfv.CurriedParameterGroups + |> Seq.collect id + |> Seq.head + |> fun p -> p.Type.Format(FSharpDisplayContext.Empty) + + let firstMfv = myzipKeywordUses[0].Symbol :?> FSharpMemberOrFunctionOrValue + Assert.Contains("int", firstParamType firstMfv) + + let secondMfv = myzipKeywordUses[1].Symbol :?> FSharpMemberOrFunctionOrValue + Assert.Contains("string", firstParamType secondMfv) + + Assert.NotEqual(firstParamType firstMfv, firstParamType secondMfv) \ No newline at end of file From fa5b9f600a3a18b1043d00d5422dd1d525fbdfbf Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 12:06:03 +0200 Subject: [PATCH 08/15] Extract deferred CO sink machinery into its own module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../CheckComputationExpressionOverloads.fs | 207 +++++++++++++++ .../CheckComputationExpressions.fs | 245 ++---------------- src/Compiler/FSharp.Compiler.Service.fsproj | 1 + 3 files changed, 227 insertions(+), 226 deletions(-) create mode 100644 src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs new file mode 100644 index 00000000000..e532679fd80 --- /dev/null +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs @@ -0,0 +1,207 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +/// Capture-and-replay machinery used by `TcComputationExpression` to sink the *resolved* +/// `MethInfo` for overloaded `[]` keywords at the keyword's source range +/// (instead of the first-registered overload), so QuickInfo and `GetAllUsesOfAllSymbolsInFile` +/// behave the same way they do for regular overloaded method calls. +/// +/// See https://github.com/dotnet/fsharp/issues/11612 and https://github.com/dotnet/fsharp/issues/15206. +module internal FSharp.Compiler.CheckComputationExpressionOverloads + +open System.Collections.Generic +open FSharp.Compiler.AccessibilityLogic +open FSharp.Compiler.Import +open FSharp.Compiler.Infos +open FSharp.Compiler.NameResolution +open FSharp.Compiler.Syntax +open FSharp.Compiler.TcGlobals +open FSharp.Compiler.Text +open FSharp.Compiler.TypedTree +open FSharp.Compiler.TypedTreeOps + +/// Information about a `[]` keyword usage whose `Item.CustomOperation` +/// sink record needs to be upgraded once normal overload resolution picks an overload. +[] +type DeferredCustomOpSink = + { + KeywordRange: range + OpName: string + UsageText: unit -> string option + SyntheticCallRange: range + Candidates: MethInfo list + Fallback: MethInfo + NameEnv: NameResolutionEnv + AccessRights: AccessorDomain + mutable Resolved: (MethInfo * TyparInstantiation) option + } + +/// Project the `MethInfo` out of every `opData` tuple returned by `getCustomOperationMethods` +/// (a 9-tuple whose last field is the `MethInfo`). +let inline methInfosOfOpDatas opDatas = + opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi: MethInfo) -> mi) + +/// Sink wrapper that forwards every notification to the supplied `forwardTo` and additionally +/// records the singleton `Item.MethodGroup` resolution that lands at one of the tracked +/// synthetic call ranges. Last-write-wins, validated by `MethInfosEquivByNameAndSig` against +/// the deferred entry's candidates so unrelated notifications at the same range are ignored. +type private CustomOpResolutionCapturingSink + ( + g: TcGlobals, + amap: ImportMap, + forwardTo: ITypecheckResultsSink option, + deferredSinksBySyntheticRange: Dictionary + ) = + + let matchesCandidate (mi: MethInfo) (entry: DeferredCustomOpSink) = + entry.Candidates + |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange c mi) + + let tryCapture (m: range) (item: Item) (tpinst: TyparInstantiation) = + match deferredSinksBySyntheticRange.TryGetValue m, item with + | (true, entries), Item.MethodGroup(_, [ mi ], _) -> + for entry in entries do + if matchesCandidate mi entry then + entry.Resolved <- Some(mi, tpinst) + | _ -> () + + interface ITypecheckResultsSink with + member _.NotifyEnvWithScope(m, nenv, ad) = + match forwardTo with + | Some s -> s.NotifyEnvWithScope(m, nenv, ad) + | None -> () + + member _.NotifyExprHasType(ty, nenv, ad, m) = + match forwardTo with + | Some s -> s.NotifyExprHasType(ty, nenv, ad, m) + | None -> () + + member _.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) = + match forwardTo with + | Some s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) + | None -> () + + member _.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) = + tryCapture m item tpinst + + match forwardTo with + | Some s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) + | None -> () + + member _.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) = + tryCapture m item tpinst + + match forwardTo with + | Some s -> s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) + | None -> () + + member _.NotifyFormatSpecifierLocation(m, numArgs) = + match forwardTo with + | Some s -> s.NotifyFormatSpecifierLocation(m, numArgs) + | None -> () + + member _.NotifyRelatedSymbolUse(m, item, kind) = + match forwardTo with + | Some s -> s.NotifyRelatedSymbolUse(m, item, kind) + | None -> () + + member _.NotifyOpenDeclaration openDeclaration = + match forwardTo with + | Some s -> s.NotifyOpenDeclaration openDeclaration + | None -> () + + member _.CurrentSourceText = forwardTo |> Option.bind (fun s -> s.CurrentSourceText) + + member _.FormatStringCheckContext = + forwardTo |> Option.bind (fun s -> s.FormatStringCheckContext) + +/// Eagerly sink an `Item.CustomOperation` at the keyword range using the fallback `MethInfo` +/// (`opDatas[0]`) and enqueue the entry for later upgrade once overload resolution finishes. +/// The early sink preserves the byte-for-byte sink ordering that downstream consumers +/// (e.g. `Test Project12 all symbols`) rely on for single-overload custom operations. +let enqueueDeferredCustomOpSink + (sink: TcResultsSink) + (nenv: NameResolutionEnv) + (ad: AccessorDomain) + (queue: ResizeArray) + (nm: Ident) + opName + usageText + syntheticCallRange + candidates + (fallback: MethInfo) + = + let fallbackItem = Item.CustomOperation(opName, usageText, Some fallback) + + CallNameResolutionSink sink (nm.idRange, nenv, fallbackItem, emptyTyparInst, ItemOccurrence.Use, ad) + + queue.Add + { + KeywordRange = nm.idRange + OpName = opName + UsageText = usageText + SyntheticCallRange = syntheticCallRange + Candidates = candidates + Fallback = fallback + NameEnv = nenv + AccessRights = ad + Resolved = None + } + +/// Run `action` (typically the `TcExpr` of the desugared CE lambda) with a sink wrapper +/// installed that captures the resolved `MethInfo` for each enqueued custom-operation +/// keyword. After `action` returns, replace each early sink record whose captured overload +/// is signature-different from the fallback with one carrying the resolved overload. +/// +/// Short-circuits the wrapping and draining entirely when no custom operations were +/// enqueued (most async/task/seq/option/list CEs) or when no IDE sink is listening +/// (plain `dotnet build`) — in either case the resolved sink record has no consumer. +/// +/// Nested CEs: the inner call captures *this* wrapper as its own `forwardTo` via +/// `sink.CurrentSink`, so notifications chain outer→inner correctly. Do not forward +/// through `sink` directly inside the wrapper or it would recurse. +let withCapturingTcSink + (g: TcGlobals) + (amap: ImportMap) + (sink: TcResultsSink) + (queue: ResizeArray) + (action: unit -> 'T) + : 'T = + if queue.Count = 0 || sink.CurrentSink.IsNone then + action () + else + let oldSinkOpt = sink.CurrentSink + + let deferredSinksBySyntheticRange = + let d = Dictionary(HashIdentity.Structural) + + for entry in queue do + let existing = + match d.TryGetValue entry.SyntheticCallRange with + | true, xs -> xs + | false, _ -> [] + + d[entry.SyntheticCallRange] <- entry :: existing + + d + + let captureSink = + CustomOpResolutionCapturingSink(g, amap, oldSinkOpt, deferredSinksBySyntheticRange) :> ITypecheckResultsSink + + let result = + use _holder = WithNewTypecheckResultsSink(captureSink, sink) + action () + + let isDifferentOverload (entry: DeferredCustomOpSink) (resolved: MethInfo) = + not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) + + for entry in queue do + match entry.Resolved with + | Some(resolved, tpinst) when isDifferentOverload entry resolved -> + let item = Item.CustomOperation(entry.OpName, entry.UsageText, Some resolved) + + CallNameResolutionSinkReplacing + sink + (entry.KeywordRange, entry.NameEnv, item, tpinst, ItemOccurrence.Use, entry.AccessRights) + | _ -> () + + result diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index 2b5eee14f87..90470d7a369 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -7,13 +7,13 @@ module internal FSharp.Compiler.CheckComputationExpressions open Internal.Utilities.Library open FSharp.Compiler.AccessibilityLogic open FSharp.Compiler.AttributeChecking +open FSharp.Compiler.CheckComputationExpressionOverloads open FSharp.Compiler.CheckExpressionsOps open FSharp.Compiler.CheckExpressions open FSharp.Compiler.CheckBasics open FSharp.Compiler.ConstraintSolver open FSharp.Compiler.DiagnosticsLogger open FSharp.Compiler.Features -open FSharp.Compiler.Import open FSharp.Compiler.Infos open FSharp.Compiler.InfoReader open FSharp.Compiler.NameResolution @@ -22,7 +22,6 @@ open FSharp.Compiler.Syntax open FSharp.Compiler.SyntaxTrivia open FSharp.Compiler.Xml open FSharp.Compiler.SyntaxTreeOps -open FSharp.Compiler.TcGlobals open FSharp.Compiler.Text open FSharp.Compiler.Text.Range open FSharp.Compiler.TypedTree @@ -43,34 +42,6 @@ type CustomOperationsMode = | Allowed | Denied -/// Information about a `[]` keyword usage whose `Item.CustomOperation` -/// sink record needs to be deferred until normal overload resolution picks the actual -/// `MethInfo`. See https://github.com/dotnet/fsharp/issues/11612 and #15206. -[] -type DeferredCustomOpSink = - { - /// Source range of the operator keyword (where the tooltip / find-references will land). - KeywordRange: range - /// Custom operation name (the `[]` argument, or method name). - OpName: string - /// Usage-text factory for the rendered tooltip. - UsageText: unit -> string option - /// Synthetic range used by `mkSynCall` for the synthesized `builder.MethName(args)` call; - /// this is the range at which overload resolution will notify the chosen `MethInfo`. - SyntheticCallRange: range - /// All `MethInfo` candidates registered for this keyword (used to validate that the captured - /// resolution corresponds to one of the actual overloads). - Candidates: MethInfo list - /// Fallback `MethInfo` used when no resolution is captured (e.g. error-recovery paths). - Fallback: MethInfo - /// Name-environment in scope at the keyword (used for the eventual sink call). - NameEnv: NameResolutionEnv - /// Access rights in scope at the keyword. - AccessRights: AccessorDomain - /// Mutable cell into which the sink wrapper writes the resolved `MethInfo` (last-write-wins). - mutable Resolved: (MethInfo * TyparInstantiation) option - } - [] type ComputationExpressionContext<'a> = { @@ -92,57 +63,12 @@ type ComputationExpressionContext<'a> = mWhole: range emptyVarSpace: LazyWithContext * TcEnv, range> /// Queue of custom-operation keyword usages whose `Item.CustomOperation` sink record - /// is deferred until after overload resolution of the desugared call. See #11612 / #15206. + /// is upgraded after overload resolution. See `CheckComputationExpressionOverloads`. deferredCustomOpSinks: ResizeArray } let inline noTailCall ceenv = { ceenv with tailCall = false } -/// Project the `MethInfo` out of every `opData` tuple returned by `getCustomOperationMethods` -/// (a 9-tuple whose last field is the `MethInfo`). -let inline methInfosOfOpDatas opDatas = - opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi: MethInfo) -> mi) - -/// Enqueue a deferred `Item.CustomOperation` sink record for `nm.idRange`. The actual sink -/// call will fire after overload resolution of the synthesized `mkSynCall` completes -/// (see `TcComputationExpression` for the wrapper that captures the resolution). -/// -/// The early sink call (with the `opDatas[0]` fallback `MethInfo`) still happens at the -/// original site so existing tests / consumers that depend on the early ordering of -/// `Item.CustomOperation` records in `TcResolutions` continue to work. The late drain only -/// fires `CallNameResolutionSinkReplacing` when the wrapper actually captured a resolution -/// different from the fallback. -let enqueueCustomOperationSink - (ceenv: ComputationExpressionContext<_>) - (nm: Ident) - opName - usageText - syntheticCallRange - candidates - (fallback: MethInfo) - = - // Record the early Item.CustomOperation sink call exactly as the original (pre-fix) code - // did, using opDatas[0] as the placeholder MethInfo. This preserves ordering for any - // consumer that walks TcResolutions and relies on the original sequence. - let fallbackItem = Item.CustomOperation(opName, usageText, Some fallback) - - CallNameResolutionSink - ceenv.cenv.tcSink - (nm.idRange, ceenv.env.NameEnv, fallbackItem, emptyTyparInst, ItemOccurrence.Use, ceenv.env.eAccessRights) - - ceenv.deferredCustomOpSinks.Add - { - KeywordRange = nm.idRange - OpName = opName - UsageText = usageText - SyntheticCallRange = syntheticCallRange - Candidates = candidates - Fallback = fallback - NameEnv = ceenv.env.NameEnv - AccessRights = ceenv.env.eAccessRights - Resolved = None - } - let inline TryFindIntrinsicOrExtensionMethInfo collectionSettings (cenv: cenv) (env: TcEnv) m ad nm ty = AllMethInfosOfTypeInScope collectionSettings cenv.infoReader env.NameEnv (Some nm) ad IgnoreOverrides m ty |> List.filter (IsExtensionMethCompatibleWithTy cenv.infoReader m ty) @@ -1205,10 +1131,12 @@ let rec TryTranslateComputationExpression let opName, _, _, _, _, _, _, _, methInfo = opDatas[0] // Defer the resolution of the custom operation sink record until after - // overload resolution. See enqueueCustomOperationSink for the rationale - // (https://github.com/dotnet/fsharp/issues/11612, #15206). - enqueueCustomOperationSink - ceenv + // overload resolution (https://github.com/dotnet/fsharp/issues/11612, #15206). + enqueueDeferredCustomOpSink + ceenv.cenv.tcSink + ceenv.env.NameEnv + ceenv.env.eAccessRights + ceenv.deferredCustomOpSinks nm opName (fun () -> customOpUsageText ceenv nm) @@ -2513,11 +2441,11 @@ and ConsumeCustomOpClauses // Defer the resolution of the custom operation sink record until after overload // resolution of the synthesized call has picked an overload. See #11612 / #15206. - // The fallback methInfo (opDatas[0]) is recorded if no resolution is captured (error - // recovery paths, e.g. isLikeZip/Join/GroupJoin or wrong-arg-count, may not produce - // a resolvable call). - enqueueCustomOperationSink - ceenv + enqueueDeferredCustomOpSink + ceenv.cenv.tcSink + ceenv.env.NameEnv + ceenv.env.eAccessRights + ceenv.deferredCustomOpSinks nm opName (fun () -> customOpUsageText ceenv nm) @@ -3036,105 +2964,6 @@ and TranslateComputationExpression (ceenv: ComputationExpressionContext<'a>) fir translatedCtxt fillExpr) -/// Sink wrapper used by `TcComputationExpression` to capture the resolved `MethInfo` -/// for synthesized custom-operation calls. The wrapper forwards every notification -/// to the original sink (captured **before** installation, to avoid recursion) and -/// in addition records the singleton method-group resolution that lands at one of -/// the tracked synthetic call ranges. -/// -/// Last-write-wins: type inference may notify the method group multiple times -/// (unrefined first, refined later via `AfterResolution.RecordResolution`); we -/// always keep the most recent singleton resolution that matches a candidate. -type private CustomOpResolutionCapturingSink - ( - g: TcGlobals, - amap: ImportMap, - forwardTo: ITypecheckResultsSink option, - deferredSinksBySyntheticRange: Dictionary - ) = - - let matchesCandidate (mi: MethInfo) (entry: DeferredCustomOpSink) = - entry.Candidates - |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange c mi) - - let tryCapture (m: range) (item: Item) (tpinst: TyparInstantiation) = - match deferredSinksBySyntheticRange.TryGetValue m, item with - | (true, entries), Item.MethodGroup(_, [ mi ], _) -> - for entry in entries do - if matchesCandidate mi entry then - entry.Resolved <- Some(mi, tpinst) - | _ -> () - - interface ITypecheckResultsSink with - member _.NotifyEnvWithScope(m, nenv, ad) = - match forwardTo with - | Some s -> s.NotifyEnvWithScope(m, nenv, ad) - | None -> () - - member _.NotifyExprHasType(ty, nenv, ad, m) = - match forwardTo with - | Some s -> s.NotifyExprHasType(ty, nenv, ad, m) - | None -> () - - member _.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) = - match forwardTo with - | Some s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) - | None -> () - - member _.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) = - tryCapture m item tpinst - - match forwardTo with - | Some s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) - | None -> () - - member _.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) = - tryCapture m item tpinst - - match forwardTo with - | Some s -> s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) - | None -> () - - member _.NotifyFormatSpecifierLocation(m, numArgs) = - match forwardTo with - | Some s -> s.NotifyFormatSpecifierLocation(m, numArgs) - | None -> () - - member _.NotifyRelatedSymbolUse(m, item, kind) = - match forwardTo with - | Some s -> s.NotifyRelatedSymbolUse(m, item, kind) - | None -> () - - member _.NotifyOpenDeclaration openDeclaration = - match forwardTo with - | Some s -> s.NotifyOpenDeclaration openDeclaration - | None -> () - - member _.CurrentSourceText = forwardTo |> Option.bind (fun s -> s.CurrentSourceText) - - member _.FormatStringCheckContext = - forwardTo |> Option.bind (fun s -> s.FormatStringCheckContext) - -/// Drain the deferred custom-operation sink queue: for each enqueued entry whose -/// wrapper captured an overload resolution that is *different* from the fallback that -/// was eagerly sunk in `enqueueCustomOperationSink`, replace the keyword's sink record -/// with one carrying the resolved `MethInfo` (and any captured `TyparInstantiation`). -/// Entries with no captured resolution, or whose captured resolution is signature- -/// equivalent to the fallback, are left as-is — the eagerly-sunk fallback is already -/// correct, and *not* calling `Replacing` here preserves the original sink ordering -/// that other tests / consumers rely on. -let private drainDeferredCustomOpSinks (g: TcGlobals) (amap: ImportMap) (sink: TcResultsSink) (entries: ResizeArray) = - let isDifferentOverload (entry: DeferredCustomOpSink) (resolved: MethInfo) = - not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) - - for entry in entries do - match entry.Resolved with - | Some(resolved, tpinst) when isDifferentOverload entry resolved -> - let item = Item.CustomOperation(entry.OpName, entry.UsageText, Some resolved) - - CallNameResolutionSinkReplacing sink (entry.KeywordRange, entry.NameEnv, item, tpinst, ItemOccurrence.Use, entry.AccessRights) - | _ -> () - /// Used for all computation expressions except sequence expressions let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv (mWhole, interpExpr: Expr, builderTy, comp: SynExpr) = let overallTy = overallTy.Commit @@ -3302,48 +3131,12 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv | _ -> env let lambdaExpr, tpenv = - // Skip the sink wrapping/draining overhead entirely when either: - // - there are no custom-operation keyword usages (most async/task/seq/option/list CEs), or - // - no IDE sink is listening (e.g. plain `dotnet build`), so the resolved sink record - // we would replay has no consumer. - if deferredCustomOpSinks.Count = 0 || cenv.tcSink.CurrentSink.IsNone then - TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr - else - // Wrap cenv.tcSink with a local sink that captures the resolved MethInfo for each - // synthesized custom-operation call site. Forwarding goes through the *captured* - // old sink (oldSinkOpt), never through cenv.tcSink, to avoid recursion through - // the wrapper we just installed. See #11612 / #15206. - // - // Nested CEs: an inner `TcComputationExpression` will capture *this* wrapper as - // its own `oldSinkOpt` and chain outer→inner correctly. Do not "optimise" by - // forwarding directly to `cenv.tcSink` here — that would recurse into the wrapper. - let oldSinkOpt = cenv.tcSink.CurrentSink - - let deferredSinksBySyntheticRange = - let d = Dictionary(HashIdentity.Structural) - - for entry in deferredCustomOpSinks do - let existing = - match d.TryGetValue entry.SyntheticCallRange with - | true, xs -> xs - | false, _ -> [] - - d[entry.SyntheticCallRange] <- entry :: existing - - d - - let captureSink = - CustomOpResolutionCapturingSink(cenv.g, cenv.amap, oldSinkOpt, deferredSinksBySyntheticRange) :> ITypecheckResultsSink - - let lambdaExpr, tpenv = - use _holder = WithNewTypecheckResultsSink(captureSink, cenv.tcSink) - TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr - - // Drain deferred sink records now that overload resolution has captured each - // resolved MethInfo (or fell back to opDatas[0] for error-recovery paths). - drainDeferredCustomOpSinks cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks - - lambdaExpr, tpenv + // Install a sink wrapper to capture the resolved MethInfo for each enqueued + // custom-operation keyword, then drain the queue once TcExpr has resolved them. + // No-op when no custom operations are present, or when no IDE sink is listening. + // See `CheckComputationExpressionOverloads.withCapturingTcSink` and #11612 / #15206. + withCapturingTcSink cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks (fun () -> + TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr) // For queries, transfer HasBeenReferenced from compiler-generated varSpace Vals to user Vals if isQuery then diff --git a/src/Compiler/FSharp.Compiler.Service.fsproj b/src/Compiler/FSharp.Compiler.Service.fsproj index 360247d7a20..f5038bd88dc 100644 --- a/src/Compiler/FSharp.Compiler.Service.fsproj +++ b/src/Compiler/FSharp.Compiler.Service.fsproj @@ -407,6 +407,7 @@ + From e27bbd66b61d71a367ae1e7c4fd443061adf14e6 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 12:17:47 +0200 Subject: [PATCH 09/15] R1 review pass: rename extracted module + helpers, fix stale xrefs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../CheckComputationExpressions.fs | 14 +++-- ...> CheckComputationExpressionsCustomOps.fs} | 58 +++++++------------ src/Compiler/FSharp.Compiler.Service.fsproj | 2 +- 3 files changed, 32 insertions(+), 42 deletions(-) rename src/Compiler/Checking/Expressions/{CheckComputationExpressionOverloads.fs => CheckComputationExpressionsCustomOps.fs} (80%) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index 90470d7a369..8aa6b0f56ab 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -7,7 +7,7 @@ module internal FSharp.Compiler.CheckComputationExpressions open Internal.Utilities.Library open FSharp.Compiler.AccessibilityLogic open FSharp.Compiler.AttributeChecking -open FSharp.Compiler.CheckComputationExpressionOverloads +open FSharp.Compiler.CheckComputationExpressionsCustomOps open FSharp.Compiler.CheckExpressionsOps open FSharp.Compiler.CheckExpressions open FSharp.Compiler.CheckBasics @@ -63,7 +63,7 @@ type ComputationExpressionContext<'a> = mWhole: range emptyVarSpace: LazyWithContext * TcEnv, range> /// Queue of custom-operation keyword usages whose `Item.CustomOperation` sink record - /// is upgraded after overload resolution. See `CheckComputationExpressionOverloads`. + /// is upgraded after overload resolution. See `CheckComputationExpressionsCustomOps`. deferredCustomOpSinks: ResizeArray } @@ -207,6 +207,10 @@ let hasMethInfo nm cenv env mBuilderVal ad builderTy = | [] -> false | _ -> true +/// Project the `MethInfo` out of every `opData` 9-tuple produced by `getCustomOperationMethods` below. +let inline methInfosOfOpDatas opDatas = + opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi: MethInfo) -> mi) + let getCustomOperationMethods (cenv: TcFileState) (env: TcEnv) ad mBuilderVal builderTy = let allMethInfos = AllMethInfosOfTypeInScope @@ -3037,7 +3041,7 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv /// Queue of `Item.CustomOperation` sink records whose final `MethInfo` depends on /// the overload resolution that happens inside the `TcExpr` call below. See - /// `enqueueCustomOperationSink` and `DeferredCustomOpSink`. + /// `enqueueDeferredCustomOpSink` and `DeferredCustomOpSink` in `CheckComputationExpressionsCustomOps`. let deferredCustomOpSinks = ResizeArray() let ceenv = @@ -3134,8 +3138,8 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv // Install a sink wrapper to capture the resolved MethInfo for each enqueued // custom-operation keyword, then drain the queue once TcExpr has resolved them. // No-op when no custom operations are present, or when no IDE sink is listening. - // See `CheckComputationExpressionOverloads.withCapturingTcSink` and #11612 / #15206. - withCapturingTcSink cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks (fun () -> + // See `CheckComputationExpressionsCustomOps.captureCustomOperationOverloads` and #11612 / #15206. + captureCustomOperationOverloads cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks (fun () -> TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr) // For queries, transfer HasBeenReferenced from compiler-generated varSpace Vals to user Vals diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs similarity index 80% rename from src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs rename to src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs index e532679fd80..253a36a8382 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressionOverloads.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs @@ -6,7 +6,7 @@ /// behave the same way they do for regular overloaded method calls. /// /// See https://github.com/dotnet/fsharp/issues/11612 and https://github.com/dotnet/fsharp/issues/15206. -module internal FSharp.Compiler.CheckComputationExpressionOverloads +module internal FSharp.Compiler.CheckComputationExpressionsCustomOps open System.Collections.Generic open FSharp.Compiler.AccessibilityLogic @@ -35,11 +35,6 @@ type DeferredCustomOpSink = mutable Resolved: (MethInfo * TyparInstantiation) option } -/// Project the `MethInfo` out of every `opData` tuple returned by `getCustomOperationMethods` -/// (a 9-tuple whose last field is the `MethInfo`). -let inline methInfosOfOpDatas opDatas = - opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi: MethInfo) -> mi) - /// Sink wrapper that forwards every notification to the supplied `forwardTo` and additionally /// records the singleton `Item.MethodGroup` resolution that lands at one of the tracked /// synthetic call ranges. Last-write-wins, validated by `MethInfosEquivByNameAndSig` against @@ -57,57 +52,48 @@ type private CustomOpResolutionCapturingSink |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange c mi) let tryCapture (m: range) (item: Item) (tpinst: TyparInstantiation) = - match deferredSinksBySyntheticRange.TryGetValue m, item with - | (true, entries), Item.MethodGroup(_, [ mi ], _) -> - for entry in entries do - if matchesCandidate mi entry then - entry.Resolved <- Some(mi, tpinst) + match item with + | Item.MethodGroup(_, [ mi ], _) -> + match deferredSinksBySyntheticRange.TryGetValue m with + | true, entries -> + for entry in entries do + if matchesCandidate mi entry then + entry.Resolved <- Some(mi, tpinst) + | false, _ -> () | _ -> () interface ITypecheckResultsSink with member _.NotifyEnvWithScope(m, nenv, ad) = - match forwardTo with - | Some s -> s.NotifyEnvWithScope(m, nenv, ad) - | None -> () + forwardTo |> Option.iter (fun s -> s.NotifyEnvWithScope(m, nenv, ad)) member _.NotifyExprHasType(ty, nenv, ad, m) = - match forwardTo with - | Some s -> s.NotifyExprHasType(ty, nenv, ad, m) - | None -> () + forwardTo |> Option.iter (fun s -> s.NotifyExprHasType(ty, nenv, ad, m)) member _.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) = - match forwardTo with - | Some s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) - | None -> () + forwardTo + |> Option.iter (fun s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m)) member _.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - match forwardTo with - | Some s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) - | None -> () + forwardTo + |> Option.iter (fun s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace)) member _.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - match forwardTo with - | Some s -> s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) - | None -> () + forwardTo + |> Option.iter (fun s -> + s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace)) member _.NotifyFormatSpecifierLocation(m, numArgs) = - match forwardTo with - | Some s -> s.NotifyFormatSpecifierLocation(m, numArgs) - | None -> () + forwardTo |> Option.iter (fun s -> s.NotifyFormatSpecifierLocation(m, numArgs)) member _.NotifyRelatedSymbolUse(m, item, kind) = - match forwardTo with - | Some s -> s.NotifyRelatedSymbolUse(m, item, kind) - | None -> () + forwardTo |> Option.iter (fun s -> s.NotifyRelatedSymbolUse(m, item, kind)) member _.NotifyOpenDeclaration openDeclaration = - match forwardTo with - | Some s -> s.NotifyOpenDeclaration openDeclaration - | None -> () + forwardTo |> Option.iter (fun s -> s.NotifyOpenDeclaration openDeclaration) member _.CurrentSourceText = forwardTo |> Option.bind (fun s -> s.CurrentSourceText) @@ -159,7 +145,7 @@ let enqueueDeferredCustomOpSink /// Nested CEs: the inner call captures *this* wrapper as its own `forwardTo` via /// `sink.CurrentSink`, so notifications chain outer→inner correctly. Do not forward /// through `sink` directly inside the wrapper or it would recurse. -let withCapturingTcSink +let captureCustomOperationOverloads (g: TcGlobals) (amap: ImportMap) (sink: TcResultsSink) diff --git a/src/Compiler/FSharp.Compiler.Service.fsproj b/src/Compiler/FSharp.Compiler.Service.fsproj index f5038bd88dc..5510af6b3f6 100644 --- a/src/Compiler/FSharp.Compiler.Service.fsproj +++ b/src/Compiler/FSharp.Compiler.Service.fsproj @@ -407,7 +407,7 @@ - + From 109698d1d57700124d3c1828ac35ea786d954b5c Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 12:31:38 +0200 Subject: [PATCH 10/15] R2 review pass: add prior-art comment citing TcMethodItemThen 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> --- .../Expressions/CheckComputationExpressionsCustomOps.fs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs index 253a36a8382..8abcc08e87b 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs @@ -5,6 +5,12 @@ /// (instead of the first-registered overload), so QuickInfo and `GetAllUsesOfAllSymbolsInFile` /// behave the same way they do for regular overloaded method calls. /// +/// The "emit a fallback `Item.X` early then `CallNameResolutionSinkReplacing` once the +/// final resolution is known" idiom is the cross-range generalisation of what +/// `TcMethodItemThen` (in `CheckExpressions.fs`) already does at a single range for +/// type-providers static arguments. Here the early sink lands at the keyword range while +/// overload resolution fires at a different synthetic call range, hence the sink wrapper. +/// /// See https://github.com/dotnet/fsharp/issues/11612 and https://github.com/dotnet/fsharp/issues/15206. module internal FSharp.Compiler.CheckComputationExpressionsCustomOps From bd68ff603c0264b6e3645dae68351d199550ff23 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 12:36:57 +0200 Subject: [PATCH 11/15] R3 review pass: use Range.comparer for Dictionary hashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../Expressions/CheckComputationExpressionsCustomOps.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs index 8abcc08e87b..a5da2aee929 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs @@ -164,7 +164,7 @@ let captureCustomOperationOverloads let oldSinkOpt = sink.CurrentSink let deferredSinksBySyntheticRange = - let d = Dictionary(HashIdentity.Structural) + let d = Dictionary(Range.comparer) for entry in queue do let existing = From 217769a6a235515425d743e918096e175c37d5f8 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 13:32:17 +0200 Subject: [PATCH 12/15] Rewrite CheckComputationExpressionsCustomOps via 5-agent + 3-voter committee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../CheckComputationExpressionsCustomOps.fs | 76 ++++++++----------- 1 file changed, 32 insertions(+), 44 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs index a5da2aee929..38fe0e51407 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs @@ -41,21 +41,17 @@ type DeferredCustomOpSink = mutable Resolved: (MethInfo * TyparInstantiation) option } -/// Sink wrapper that forwards every notification to the supplied `forwardTo` and additionally -/// records the singleton `Item.MethodGroup` resolution that lands at one of the tracked -/// synthetic call ranges. Last-write-wins, validated by `MethInfosEquivByNameAndSig` against -/// the deferred entry's candidates so unrelated notifications at the same range are ignored. -type private CustomOpResolutionCapturingSink - ( - g: TcGlobals, - amap: ImportMap, - forwardTo: ITypecheckResultsSink option, - deferredSinksBySyntheticRange: Dictionary - ) = - - let matchesCandidate (mi: MethInfo) (entry: DeferredCustomOpSink) = - entry.Candidates - |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange c mi) +/// Sink wrapper that forwards every notification to `forwardTo` and additionally records +/// the singleton `Item.MethodGroup` resolution that lands at one of the tracked synthetic +/// call ranges. Last-write-wins, validated by `MethInfosEquivByNameAndSig` against the +/// deferred entry's candidates so unrelated notifications at the same range are ignored. +/// `forwardTo` is non-optional: callers must gate construction on a real outer sink. +let private makeCustomOpResolutionCapturingSink + (g: TcGlobals) + (amap: ImportMap) + (forwardTo: ITypecheckResultsSink) + (deferredSinksBySyntheticRange: Dictionary) + : ITypecheckResultsSink = let tryCapture (m: range) (item: Item) (tpinst: TyparInstantiation) = match item with @@ -63,48 +59,45 @@ type private CustomOpResolutionCapturingSink match deferredSinksBySyntheticRange.TryGetValue m with | true, entries -> for entry in entries do - if matchesCandidate mi entry then + if + entry.Candidates + |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange c mi) + then entry.Resolved <- Some(mi, tpinst) | false, _ -> () | _ -> () - interface ITypecheckResultsSink with + { new ITypecheckResultsSink with member _.NotifyEnvWithScope(m, nenv, ad) = - forwardTo |> Option.iter (fun s -> s.NotifyEnvWithScope(m, nenv, ad)) + forwardTo.NotifyEnvWithScope(m, nenv, ad) member _.NotifyExprHasType(ty, nenv, ad, m) = - forwardTo |> Option.iter (fun s -> s.NotifyExprHasType(ty, nenv, ad, m)) + forwardTo.NotifyExprHasType(ty, nenv, ad, m) member _.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) = - forwardTo - |> Option.iter (fun s -> s.NotifyExprHasTypeSynthetic(ty, nenv, ad, m)) + forwardTo.NotifyExprHasTypeSynthetic(ty, nenv, ad, m) member _.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - - forwardTo - |> Option.iter (fun s -> s.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace)) + forwardTo.NotifyNameResolution(endPos, item, tpinst, occurrenceType, nenv, ad, m, replace) member _.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) = tryCapture m item tpinst - - forwardTo - |> Option.iter (fun s -> - s.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace)) + forwardTo.NotifyMethodGroupNameResolution(endPos, item, itemMethodGroup, tpinst, occurrenceType, nenv, ad, m, replace) member _.NotifyFormatSpecifierLocation(m, numArgs) = - forwardTo |> Option.iter (fun s -> s.NotifyFormatSpecifierLocation(m, numArgs)) + forwardTo.NotifyFormatSpecifierLocation(m, numArgs) member _.NotifyRelatedSymbolUse(m, item, kind) = - forwardTo |> Option.iter (fun s -> s.NotifyRelatedSymbolUse(m, item, kind)) + forwardTo.NotifyRelatedSymbolUse(m, item, kind) member _.NotifyOpenDeclaration openDeclaration = - forwardTo |> Option.iter (fun s -> s.NotifyOpenDeclaration openDeclaration) + forwardTo.NotifyOpenDeclaration openDeclaration - member _.CurrentSourceText = forwardTo |> Option.bind (fun s -> s.CurrentSourceText) + member _.CurrentSourceText = forwardTo.CurrentSourceText - member _.FormatStringCheckContext = - forwardTo |> Option.bind (fun s -> s.FormatStringCheckContext) + member _.FormatStringCheckContext = forwardTo.FormatStringCheckContext + } /// Eagerly sink an `Item.CustomOperation` at the keyword range using the fallback `MethInfo` /// (`opDatas[0]`) and enqueue the entry for later upgrade once overload resolution finishes. @@ -158,11 +151,8 @@ let captureCustomOperationOverloads (queue: ResizeArray) (action: unit -> 'T) : 'T = - if queue.Count = 0 || sink.CurrentSink.IsNone then - action () - else - let oldSinkOpt = sink.CurrentSink - + match sink.CurrentSink with + | Some oldSink when queue.Count > 0 -> let deferredSinksBySyntheticRange = let d = Dictionary(Range.comparer) @@ -177,18 +167,15 @@ let captureCustomOperationOverloads d let captureSink = - CustomOpResolutionCapturingSink(g, amap, oldSinkOpt, deferredSinksBySyntheticRange) :> ITypecheckResultsSink + makeCustomOpResolutionCapturingSink g amap oldSink deferredSinksBySyntheticRange let result = use _holder = WithNewTypecheckResultsSink(captureSink, sink) action () - let isDifferentOverload (entry: DeferredCustomOpSink) (resolved: MethInfo) = - not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) - for entry in queue do match entry.Resolved with - | Some(resolved, tpinst) when isDifferentOverload entry resolved -> + | Some(resolved, tpinst) when not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) -> let item = Item.CustomOperation(entry.OpName, entry.UsageText, Some resolved) CallNameResolutionSinkReplacing @@ -197,3 +184,4 @@ let captureCustomOperationOverloads | _ -> () result + | _ -> action () From 7f5043ec02caa6355ef733da4bae3d759b137b37 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 13:54:35 +0200 Subject: [PATCH 13/15] Drop mutable record field, Candidates list, and MethInfosEquivByNameAndSig hot-path scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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.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> --- .../CheckComputationExpressions.fs | 8 +- .../CheckComputationExpressionsCustomOps.fs | 84 ++++++++----------- 2 files changed, 37 insertions(+), 55 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index 8aa6b0f56ab..00522b11f8c 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -207,10 +207,6 @@ let hasMethInfo nm cenv env mBuilderVal ad builderTy = | [] -> false | _ -> true -/// Project the `MethInfo` out of every `opData` 9-tuple produced by `getCustomOperationMethods` below. -let inline methInfosOfOpDatas opDatas = - opDatas |> List.map (fun (_, _, _, _, _, _, _, _, mi: MethInfo) -> mi) - let getCustomOperationMethods (cenv: TcFileState) (env: TcEnv) ad mBuilderVal builderTy = let allMethInfos = AllMethInfosOfTypeInScope @@ -1145,7 +1141,6 @@ let rec TryTranslateComputationExpression opName (fun () -> customOpUsageText ceenv nm) (mOpCore.MakeSynthetic()) - (methInfosOfOpDatas opDatas) methInfo let mkJoinExpr keySelector1 keySelector2 innerPat e = @@ -2454,7 +2449,6 @@ and ConsumeCustomOpClauses opName (fun () -> customOpUsageText ceenv nm) (mClause.MakeSynthetic()) - (methInfosOfOpDatas opDatas) methInfo if isLikeZip || isLikeJoin || isLikeGroupJoin then @@ -3139,7 +3133,7 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv // custom-operation keyword, then drain the queue once TcExpr has resolved them. // No-op when no custom operations are present, or when no IDE sink is listening. // See `CheckComputationExpressionsCustomOps.captureCustomOperationOverloads` and #11612 / #15206. - captureCustomOperationOverloads cenv.g cenv.amap cenv.tcSink deferredCustomOpSinks (fun () -> + captureCustomOperationOverloads cenv.tcSink deferredCustomOpSinks (fun () -> TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr) // For queries, transfer HasBeenReferenced from compiler-generated varSpace Vals to user Vals diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs index 38fe0e51407..5eb9b3f572b 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs @@ -16,17 +16,17 @@ module internal FSharp.Compiler.CheckComputationExpressionsCustomOps open System.Collections.Generic open FSharp.Compiler.AccessibilityLogic -open FSharp.Compiler.Import open FSharp.Compiler.Infos open FSharp.Compiler.NameResolution open FSharp.Compiler.Syntax -open FSharp.Compiler.TcGlobals open FSharp.Compiler.Text open FSharp.Compiler.TypedTree open FSharp.Compiler.TypedTreeOps /// Information about a `[]` keyword usage whose `Item.CustomOperation` /// sink record needs to be upgraded once normal overload resolution picks an overload. +/// Fully immutable: captured resolutions live in the side dictionary set up by +/// `captureCustomOperationOverloads`, keyed by `SyntheticCallRange`. [] type DeferredCustomOpSink = { @@ -34,37 +34,30 @@ type DeferredCustomOpSink = OpName: string UsageText: unit -> string option SyntheticCallRange: range - Candidates: MethInfo list Fallback: MethInfo NameEnv: NameResolutionEnv AccessRights: AccessorDomain - mutable Resolved: (MethInfo * TyparInstantiation) option } -/// Sink wrapper that forwards every notification to `forwardTo` and additionally records -/// the singleton `Item.MethodGroup` resolution that lands at one of the tracked synthetic -/// call ranges. Last-write-wins, validated by `MethInfosEquivByNameAndSig` against the -/// deferred entry's candidates so unrelated notifications at the same range are ignored. +/// Sink wrapper that forwards every notification to `forwardTo` and additionally records, +/// for every tracked synthetic call range, the singleton `Item.MethodGroup` resolution that +/// lands at it. The synthetic range can collide with outer-comprehension calls (e.g. `For`) +/// at the same range, so we also check the method name matches the expected fallback's +/// `LogicalName` — that's an O(1) string compare, no `MethInfosEquivByNameAndSig` on the +/// hot path. +/// /// `forwardTo` is non-optional: callers must gate construction on a real outer sink. let private makeCustomOpResolutionCapturingSink - (g: TcGlobals) - (amap: ImportMap) (forwardTo: ITypecheckResultsSink) - (deferredSinksBySyntheticRange: Dictionary) + (capturedResolutions: Dictionary) : ITypecheckResultsSink = let tryCapture (m: range) (item: Item) (tpinst: TyparInstantiation) = match item with - | Item.MethodGroup(_, [ mi ], _) -> - match deferredSinksBySyntheticRange.TryGetValue m with - | true, entries -> - for entry in entries do - if - entry.Candidates - |> List.exists (fun c -> MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange c mi) - then - entry.Resolved <- Some(mi, tpinst) - | false, _ -> () + | Item.MethodGroup(name, [ mi ], _) -> + match capturedResolutions.TryGetValue m with + | true, (expectedName, _, _) when name = expectedName -> capturedResolutions[m] <- (expectedName, mi, tpinst) + | _ -> () | _ -> () { new ITypecheckResultsSink with @@ -112,7 +105,6 @@ let enqueueDeferredCustomOpSink opName usageText syntheticCallRange - candidates (fallback: MethInfo) = let fallbackItem = Item.CustomOperation(opName, usageText, Some fallback) @@ -125,17 +117,17 @@ let enqueueDeferredCustomOpSink OpName = opName UsageText = usageText SyntheticCallRange = syntheticCallRange - Candidates = candidates Fallback = fallback NameEnv = nenv AccessRights = ad - Resolved = None } /// Run `action` (typically the `TcExpr` of the desugared CE lambda) with a sink wrapper /// installed that captures the resolved `MethInfo` for each enqueued custom-operation /// keyword. After `action` returns, replace each early sink record whose captured overload -/// is signature-different from the fallback with one carrying the resolved overload. +/// is a *different* method definition from the fallback with one carrying the resolved +/// overload. Single-overload CEs leave the eager fallback record untouched (`Replacing` +/// would reorder it and break `Test Project12 all symbols`). /// /// Short-circuits the wrapping and draining entirely when no custom operations were /// enqueued (most async/task/seq/option/list CEs) or when no IDE sink is listening @@ -144,44 +136,40 @@ let enqueueDeferredCustomOpSink /// Nested CEs: the inner call captures *this* wrapper as its own `forwardTo` via /// `sink.CurrentSink`, so notifications chain outer→inner correctly. Do not forward /// through `sink` directly inside the wrapper or it would recurse. -let captureCustomOperationOverloads - (g: TcGlobals) - (amap: ImportMap) - (sink: TcResultsSink) - (queue: ResizeArray) - (action: unit -> 'T) - : 'T = +let captureCustomOperationOverloads (sink: TcResultsSink) (queue: ResizeArray) (action: unit -> 'T) : 'T = match sink.CurrentSink with | Some oldSink when queue.Count > 0 -> - let deferredSinksBySyntheticRange = - let d = Dictionary(Range.comparer) - - for entry in queue do - let existing = - match d.TryGetValue entry.SyntheticCallRange with - | true, xs -> xs - | false, _ -> [] + // The capture dict serves both as the "tracked ranges" set and the result buffer. + // Each entry is `(expectedMethodName, capturedOrFallback, tpinst)`: + // * expectedMethodName comes from `Fallback.LogicalName` and is fixed for the entry — + // used in the wrapper to filter out unrelated MethodGroup notifications that share + // the synthetic range (e.g. an enclosing `For` call in a join/zip clause). + // * the second/third positions start as the fallback and are overwritten by the + // wrapper when the resolved overload's MethodGroup notification arrives. + // Drain decides whether to call `Replacing` by comparing the captured method against + // the fallback with `MethInfo.MethInfosUseIdenticalDefinitions` (cheap def-equality, + // not the deep `MethInfosEquivByNameAndSig`). + let capturedResolutions = + Dictionary(Range.comparer) - d[entry.SyntheticCallRange] <- entry :: existing - - d + for entry in queue do + capturedResolutions[entry.SyntheticCallRange] <- (entry.Fallback.LogicalName, entry.Fallback, emptyTyparInst) - let captureSink = - makeCustomOpResolutionCapturingSink g amap oldSink deferredSinksBySyntheticRange + let captureSink = makeCustomOpResolutionCapturingSink oldSink capturedResolutions let result = use _holder = WithNewTypecheckResultsSink(captureSink, sink) action () for entry in queue do - match entry.Resolved with - | Some(resolved, tpinst) when not (MethInfosEquivByNameAndSig EraseAll true g amap entry.KeywordRange resolved entry.Fallback) -> + let _, resolved, tpinst = capturedResolutions[entry.SyntheticCallRange] + + if not (MethInfo.MethInfosUseIdenticalDefinitions resolved entry.Fallback) then let item = Item.CustomOperation(entry.OpName, entry.UsageText, Some resolved) CallNameResolutionSinkReplacing sink (entry.KeywordRange, entry.NameEnv, item, tpinst, ItemOccurrence.Use, entry.AccessRights) - | _ -> () result | _ -> action () From 0b6f597927377b21308d68a199535e1bf104bdff Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 14:23:30 +0200 Subject: [PATCH 14/15] Document and test the [mi] vs multi-element MethodGroup case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../CheckComputationExpressionsCustomOps.fs | 18 ++++-- .../TooltipTests.fs | 57 ++++++++++++++++++- 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs index 5eb9b3f572b..11678671005 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs @@ -41,10 +41,20 @@ type DeferredCustomOpSink = /// Sink wrapper that forwards every notification to `forwardTo` and additionally records, /// for every tracked synthetic call range, the singleton `Item.MethodGroup` resolution that -/// lands at it. The synthetic range can collide with outer-comprehension calls (e.g. `For`) -/// at the same range, so we also check the method name matches the expected fallback's -/// `LogicalName` — that's an O(1) string compare, no `MethInfosEquivByNameAndSig` on the -/// hot path. +/// lands at it. +/// +/// We deliberately only match the *singleton* `[ mi ]` pattern (= the refined notification +/// fired by `AfterResolution.RecordResolution`'s `callSinkWithSpecificOverload` once +/// overload resolution has settled, see `NameResolution.fs:4300-4314`). The earlier +/// *unrefined* `Item.MethodGroup(name, [...allCandidates...], _)` notification arrives +/// before overload resolution picks one — capturing it would replay the wrong record. +/// If the refined notification never fires (overload resolution failed on broken user code), +/// the dict slot stays at `Fallback` and the drain's identity check decides to leave the +/// eager fallback sink record in place — graceful degradation rather than blanking the IDE. +/// +/// The synthetic range can also collide with outer-comprehension calls (e.g. `For`) at the +/// same range, so we also check the method name matches the expected fallback's +/// `LogicalName` — an O(1) string compare, no `MethInfosEquivByNameAndSig` on the hot path. /// /// `forwardTo` is non-optional: callers must gate construction on a real outer sink. let private makeCustomOpResolutionCapturingSink diff --git a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs index 80c2c95f5aa..6fc0cc1a422 100644 --- a/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/TooltipTests.fs @@ -4,6 +4,7 @@ #nowarn "57" open FSharp.Compiler.CodeAnalysis +open FSharp.Compiler.Diagnostics open FSharp.Compiler.Service.Tests.Common open FSharp.Compiler.Text open FSharp.Compiler.EditorServices @@ -1052,4 +1053,58 @@ let _ = b { for x in ["a";"b"] do let secondMfv = myzipKeywordUses[1].Symbol :?> FSharpMemberOrFunctionOrValue Assert.Contains("string", firstParamType secondMfv) - Assert.NotEqual(firstParamType firstMfv, firstParamType secondMfv) \ No newline at end of file + Assert.NotEqual(firstParamType firstMfv, firstParamType secondMfv) + +// When `Item.MethodGroup` arrives at the synthetic call range with *more than one* MethInfo +// (the unrefined group fired by `ResolveExprDotLongIdentAndComputeRange` BEFORE +// `AfterResolution.RecordResolution` settles), our wrapper's `[ mi ]` singleton pattern +// must NOT match — capturing the unrefined list would replay the wrong overload (or all of +// them). When overload resolution then fails (broken user code) and the refined singleton +// notification never arrives, the dictionary slot stays at the pre-populated `Fallback`, +// so the drain's `MethInfosUseIdenticalDefinitions` check returns true → no `Replacing` +// → the early-sunk `Item.CustomOperation(opName, _, Some Fallback)` record stays at the +// keyword. This is the right error-recovery behaviour: the user still sees *a* MethInfo +// in QuickInfo / Find-All-References instead of nothing. +[] +let ``Broken overloaded CE custom op call falls back to the eager opDatas[0] sink record`` () = + // The for-loop iterates over a list whose element type matches NEITHER overload's outer + // type, so F# overload resolution cannot pick a single overload. We expect: + // * A type-error diagnostic. + // * The keyword still has an Item.CustomOperation symbol-use record (the fallback) — + // we just don't know which overload was picked, because none was. + let source = """ +module M +type FooBuilder() = + member _.Yield _ = 1 + member _.For(xs, body) = xs |> Seq.iter body + [] + member _.Pick(_, x: int) = x + [] + member _.Pick(_, x: string) = x + +let b = FooBuilder() +let _ = b { for x in [true] do pick true } +""" + let _, checkResults = getParseAndCheckResults source + + // We expect a type-check error from overload resolution failing. + let errors = + checkResults.Diagnostics + |> Array.filter (fun d -> d.Severity = FSharpDiagnosticSeverity.Error) + + Assert.NotEmpty errors + + // But the symbol-use record at the keyword 'pick' must still exist (the eager fallback + // sunk by enqueueDeferredCustomOpSink) — the wrapper just didn't get a chance to upgrade + // it. Without this graceful fallback, IDE features at the keyword would go blank on + // broken CE code. + let pickKeywordUses = + checkResults.GetAllUsesOfAllSymbolsInFile() + |> Seq.filter (fun u -> + match u.Symbol with + | :? FSharpMemberOrFunctionOrValue as mfv -> + mfv.LogicalName = "Pick" && not u.IsFromDefinition + | _ -> false) + |> List.ofSeq + + Assert.Equal(1, pickKeywordUses.Length) \ No newline at end of file From 28ff8839b4817e1a58278a781fb5f7bcb3c144be Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 4 Jun 2026 14:35:27 +0200 Subject: [PATCH 15/15] Strip code comments from product code 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> --- .../CheckComputationExpressions.fs | 13 ---- .../CheckComputationExpressionsCustomOps.fs | 64 +------------------ 2 files changed, 2 insertions(+), 75 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs index 00522b11f8c..3548746ce6c 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressions.fs @@ -62,8 +62,6 @@ type ComputationExpressionContext<'a> = origComp: SynExpr mWhole: range emptyVarSpace: LazyWithContext * TcEnv, range> - /// Queue of custom-operation keyword usages whose `Item.CustomOperation` sink record - /// is upgraded after overload resolution. See `CheckComputationExpressionsCustomOps`. deferredCustomOpSinks: ResizeArray } @@ -1130,8 +1128,6 @@ let rec TryTranslateComputationExpression | Some opDatas -> let opName, _, _, _, _, _, _, _, methInfo = opDatas[0] - // Defer the resolution of the custom operation sink record until after - // overload resolution (https://github.com/dotnet/fsharp/issues/11612, #15206). enqueueDeferredCustomOpSink ceenv.cenv.tcSink ceenv.env.NameEnv @@ -2438,8 +2434,6 @@ and ConsumeCustomOpClauses let isLikeGroupJoin = customOperationIsLikeZip ceenv nm - // Defer the resolution of the custom operation sink record until after overload - // resolution of the synthesized call has picked an overload. See #11612 / #15206. enqueueDeferredCustomOpSink ceenv.cenv.tcSink ceenv.env.NameEnv @@ -3033,9 +3027,6 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv let origComp = comp - /// Queue of `Item.CustomOperation` sink records whose final `MethInfo` depends on - /// the overload resolution that happens inside the `TcExpr` call below. See - /// `enqueueDeferredCustomOpSink` and `DeferredCustomOpSink` in `CheckComputationExpressionsCustomOps`. let deferredCustomOpSinks = ResizeArray() let ceenv = @@ -3129,10 +3120,6 @@ let TcComputationExpression (cenv: TcFileState) env (overallTy: OverallTy) tpenv | _ -> env let lambdaExpr, tpenv = - // Install a sink wrapper to capture the resolved MethInfo for each enqueued - // custom-operation keyword, then drain the queue once TcExpr has resolved them. - // No-op when no custom operations are present, or when no IDE sink is listening. - // See `CheckComputationExpressionsCustomOps.captureCustomOperationOverloads` and #11612 / #15206. captureCustomOperationOverloads cenv.tcSink deferredCustomOpSinks (fun () -> TcExpr cenv (MustEqual(mkFunTy cenv.g builderTy overallTy)) env tpenv lambdaExpr) diff --git a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs index 11678671005..29cbdfcbb8e 100644 --- a/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs +++ b/src/Compiler/Checking/Expressions/CheckComputationExpressionsCustomOps.fs @@ -1,17 +1,7 @@ // Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. -/// Capture-and-replay machinery used by `TcComputationExpression` to sink the *resolved* -/// `MethInfo` for overloaded `[]` keywords at the keyword's source range -/// (instead of the first-registered overload), so QuickInfo and `GetAllUsesOfAllSymbolsInFile` -/// behave the same way they do for regular overloaded method calls. -/// -/// The "emit a fallback `Item.X` early then `CallNameResolutionSinkReplacing` once the -/// final resolution is known" idiom is the cross-range generalisation of what -/// `TcMethodItemThen` (in `CheckExpressions.fs`) already does at a single range for -/// type-providers static arguments. Here the early sink lands at the keyword range while -/// overload resolution fires at a different synthetic call range, hence the sink wrapper. -/// -/// See https://github.com/dotnet/fsharp/issues/11612 and https://github.com/dotnet/fsharp/issues/15206. +/// Sinks the resolved overload's `MethInfo` at the keyword range of an overloaded +/// `[]` usage in a computation expression — fixes #11612 / #15206. module internal FSharp.Compiler.CheckComputationExpressionsCustomOps open System.Collections.Generic @@ -23,10 +13,6 @@ open FSharp.Compiler.Text open FSharp.Compiler.TypedTree open FSharp.Compiler.TypedTreeOps -/// Information about a `[]` keyword usage whose `Item.CustomOperation` -/// sink record needs to be upgraded once normal overload resolution picks an overload. -/// Fully immutable: captured resolutions live in the side dictionary set up by -/// `captureCustomOperationOverloads`, keyed by `SyntheticCallRange`. [] type DeferredCustomOpSink = { @@ -39,24 +25,6 @@ type DeferredCustomOpSink = AccessRights: AccessorDomain } -/// Sink wrapper that forwards every notification to `forwardTo` and additionally records, -/// for every tracked synthetic call range, the singleton `Item.MethodGroup` resolution that -/// lands at it. -/// -/// We deliberately only match the *singleton* `[ mi ]` pattern (= the refined notification -/// fired by `AfterResolution.RecordResolution`'s `callSinkWithSpecificOverload` once -/// overload resolution has settled, see `NameResolution.fs:4300-4314`). The earlier -/// *unrefined* `Item.MethodGroup(name, [...allCandidates...], _)` notification arrives -/// before overload resolution picks one — capturing it would replay the wrong record. -/// If the refined notification never fires (overload resolution failed on broken user code), -/// the dict slot stays at `Fallback` and the drain's identity check decides to leave the -/// eager fallback sink record in place — graceful degradation rather than blanking the IDE. -/// -/// The synthetic range can also collide with outer-comprehension calls (e.g. `For`) at the -/// same range, so we also check the method name matches the expected fallback's -/// `LogicalName` — an O(1) string compare, no `MethInfosEquivByNameAndSig` on the hot path. -/// -/// `forwardTo` is non-optional: callers must gate construction on a real outer sink. let private makeCustomOpResolutionCapturingSink (forwardTo: ITypecheckResultsSink) (capturedResolutions: Dictionary) @@ -102,10 +70,6 @@ let private makeCustomOpResolutionCapturingSink member _.FormatStringCheckContext = forwardTo.FormatStringCheckContext } -/// Eagerly sink an `Item.CustomOperation` at the keyword range using the fallback `MethInfo` -/// (`opDatas[0]`) and enqueue the entry for later upgrade once overload resolution finishes. -/// The early sink preserves the byte-for-byte sink ordering that downstream consumers -/// (e.g. `Test Project12 all symbols`) rely on for single-overload custom operations. let enqueueDeferredCustomOpSink (sink: TcResultsSink) (nenv: NameResolutionEnv) @@ -132,33 +96,9 @@ let enqueueDeferredCustomOpSink AccessRights = ad } -/// Run `action` (typically the `TcExpr` of the desugared CE lambda) with a sink wrapper -/// installed that captures the resolved `MethInfo` for each enqueued custom-operation -/// keyword. After `action` returns, replace each early sink record whose captured overload -/// is a *different* method definition from the fallback with one carrying the resolved -/// overload. Single-overload CEs leave the eager fallback record untouched (`Replacing` -/// would reorder it and break `Test Project12 all symbols`). -/// -/// Short-circuits the wrapping and draining entirely when no custom operations were -/// enqueued (most async/task/seq/option/list CEs) or when no IDE sink is listening -/// (plain `dotnet build`) — in either case the resolved sink record has no consumer. -/// -/// Nested CEs: the inner call captures *this* wrapper as its own `forwardTo` via -/// `sink.CurrentSink`, so notifications chain outer→inner correctly. Do not forward -/// through `sink` directly inside the wrapper or it would recurse. let captureCustomOperationOverloads (sink: TcResultsSink) (queue: ResizeArray) (action: unit -> 'T) : 'T = match sink.CurrentSink with | Some oldSink when queue.Count > 0 -> - // The capture dict serves both as the "tracked ranges" set and the result buffer. - // Each entry is `(expectedMethodName, capturedOrFallback, tpinst)`: - // * expectedMethodName comes from `Fallback.LogicalName` and is fixed for the entry — - // used in the wrapper to filter out unrelated MethodGroup notifications that share - // the synthetic range (e.g. an enclosing `For` call in a join/zip clause). - // * the second/third positions start as the fallback and are overwritten by the - // wrapper when the resolved overload's MethodGroup notification arrives. - // Drain decides whether to call `Replacing` by comparing the captured method against - // the fallback with `MethInfo.MethInfosUseIdenticalDefinitions` (cheap def-equality, - // not the deep `MethInfosEquivByNameAndSig`). let capturedResolutions = Dictionary(Range.comparer)