Skip to content

NavigableContainer: publishing the cases makes every comparison box the payload #20641

Description

@xperiandri

@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

  1. [<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.
  2. 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.
  3. 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.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions