@T-Gro — this came out of the review of #20529 and needs an API decision before that PR can land, so raising it here rather than deciding it in a thread.
What the PR does
#20529 publishes the cases of FSharp.Compiler.EditorServices.NavigableContainer, which were [<Sealed>]-hidden before. The payload of Container became a [<Struct>] record so that a caller can read info.NameParts off a value without a pattern match:
[<Struct>]
type NavigableContainerInfo =
{ ContainerType: NavigableContainerType
NameParts: string list
Parent: NavigableContainer }
and NavigableContainer =
| File of fileName: string
| Container of info: NavigableContainerInfo
The problem
The union keeps its default structural comparison, and comparing two Container values reaches the payload through generic comparison, which takes obj. The struct record is therefore boxed on every comparison, once per nesting level:
let chain file =
[1..3] |> List.fold (fun parent _ ->
NavigableContainer.Container { ContainerType = NavigableContainerType.Module; NameParts = ["M"]; Parent = parent })
(NavigableContainer.File file)
let a, b = chain "a.fs", chain "b.fs"
let comparer = Comparer<NavigableContainer>.Default
// 100k comparisons: 12 000 000 bytes allocated, 120 per comparison at depth three.
// Zero with the tuple payload the PR replaces.
Implementing IComparable<NavigableContainerInfo> does not help: generic comparison boxes before it looks for a typed implementation.
Nothing orders containers today — NavigateTo.GetNavigableItems returns an array in traversal order, the editor reads a container only for FullName and Name, and Navigate To orders results by the strings ComputeSecondarySort builds. So this is not a live regression; it is an allocation that the published API would invite, and a reviewer measured it before the API is fixed in place.
The options
[<NoComparison>] on the container. The comparison disappears, so nothing can box. Ordering containers has no meaning anyway. Breaking: NavigableContainer.CompareTo is in the surface-area baseline today, so anyone who put containers in a Set, a Map, or sorted them stops compiling.
- Fields back on the case —
Container of containerType: NavigableContainerType * nameParts: string list * parent: NavigableContainer. One allocation, no boxing, fields still named in a match. What goes is info.NameParts outside a match, and NavigableContainerInfo as a type to pass around.
- Keep the record and write the comparison —
[<CustomEquality; CustomComparison>] on the union with hand-written Equals, GetHashCode and CompareTo. Keeps the record, keeps CompareTo in the baseline, removes the boxing; costs hand-written code on a recursive type that can drift from what the compiler generated.
Preference
I would take 1. Ordering these is meaningless, and removing a comparison nobody should have relied on is more honest than maintaining one by hand or shaping the API around it. The baseline entry is the only argument against, and it is an entry for behaviour that was never useful.
Happy to implement whichever the team prefers — the PR is otherwise ready.
@T-Gro — this came out of the review of #20529 and needs an API decision before that PR can land, so raising it here rather than deciding it in a thread.
What the PR does
#20529 publishes the cases of
FSharp.Compiler.EditorServices.NavigableContainer, which were[<Sealed>]-hidden before. The payload ofContainerbecame a[<Struct>]record so that a caller can readinfo.NamePartsoff a value without a pattern match:The problem
The union keeps its default structural comparison, and comparing two
Containervalues reaches the payload through generic comparison, which takesobj. The struct record is therefore boxed on every comparison, once per nesting level:Implementing
IComparable<NavigableContainerInfo>does not help: generic comparison boxes before it looks for a typed implementation.Nothing orders containers today —
NavigateTo.GetNavigableItemsreturns an array in traversal order, the editor reads a container only forFullNameandName, and Navigate To orders results by the stringsComputeSecondarySortbuilds. So this is not a live regression; it is an allocation that the published API would invite, and a reviewer measured it before the API is fixed in place.The options
[<NoComparison>]on the container. The comparison disappears, so nothing can box. Ordering containers has no meaning anyway. Breaking:NavigableContainer.CompareTois in the surface-area baseline today, so anyone who put containers in aSet, aMap, or sorted them stops compiling.Container of containerType: NavigableContainerType * nameParts: string list * parent: NavigableContainer. One allocation, no boxing, fields still named in a match. What goes isinfo.NamePartsoutside a match, andNavigableContainerInfoas a type to pass around.[<CustomEquality; CustomComparison>]on the union with hand-writtenEquals,GetHashCodeandCompareTo. Keeps the record, keepsCompareToin the baseline, removes the boxing; costs hand-written code on a recursive type that can drift from what the compiler generated.Preference
I would take 1. Ordering these is meaningless, and removing a comparison nobody should have relied on is more honest than maintaining one by hand or shaping the API around it. The baseline entry is the only argument against, and it is an entry for behaviour that was never useful.
Happy to implement whichever the team prefers — the PR is otherwise ready.