Publish NavigableContainer's cases - #20529
Open
xperiandri wants to merge 2 commits into
Open
xperiandri wants to merge 2 commits into
xperiandri wants to merge 2 commits into
Conversation
`NavigateTo.GetNavigableItems` hands back items whose `Container` can be read but not constructed: the signature seals the type and hides the `File` and `Container` cases. That is enough to display a result and not enough to carry one across a process boundary, which is what caching navigable items on disk needs — the reader has to rebuild the container it deserialized. Publish both cases. While the shape is still private, replace `Container`'s three-element tuple with a named record so the parts have names at the point of use, and make it a struct: a struct record is laid out inside the case exactly as the tuple was, so this costs nothing. Building a file plus three nested containers a million times allocates 144 bytes per chain either way, where a reference record would take 216. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
🔍 Tooling Safety Check — Affects-Design-Time
|
T-Gro
reviewed
Sep 14, 2026
T-Gro
left a comment
Member
There was a problem hiding this comment.
🤖 🕵️ AI review — verify independently.
| and NavigableContainer = | ||
| | File of fileName: string | ||
| | Container of containerType: NavigableContainerType * nameParts: string list * parent: NavigableContainer | ||
| | Container of info: NavigableContainerInfo |
Member
There was a problem hiding this comment.
🤖 🕵️ [P2] Ordered-container comparisons now box each NavigableContainerInfo — 120 bytes per comparison at depth three, versus 0 with the previous representation.
open System
open System.Collections.Generic
open FSharp.Compiler.EditorServices
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
for _ in 1..10000 do comparer.Compare(a, b) |> ignore
let allocated =
let before = GC.GetAllocatedBytesForCurrentThread()
for _ in 1..100000 do comparer.Compare(a, b) |> ignore
GC.GetAllocatedBytesForCurrentThread() - before
printfn "%d bytes" allocated // 12000000 bytes
T-Gro
self-requested a review
September 14, 2026 14:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NavigateTo.GetNavigableItemsreturns items a consumer can read but not rebuild: the signature sealsNavigableContainerand hides itsFileandContainercases. That is enough to display a result, and notenough to carry one out of the process that produced it. Visual Studio's Navigate To is to keep each file's
navigable items on disk between sessions, as Roslyn keeps its syntax index, and reading them back means
constructing containers.
The change
NavigableContainerpublishesFile of fileName: stringandContainer of info: NavigableContainerInfo. Itsmembers
Type,FullNameandNameare unchanged.Containercarried a three-element tuple,containerType * nameParts * parent. Published as it was, the tuplewould be fixed into the API, so it becomes a record with named fields first:
NavigableContainerInfo { ContainerType; NameParts; Parent }.The record is a
[<Struct>]. A struct record lies inside the case exactly as the tuple's fields did — oneallocation per container — where a reference record adds an object per container and an indirection on every
access. Building a file and three nested containers a million times (x64, .NET 11,
fsi --optimize+):Containerpayload[<Struct>]recordA struct union is not an option:
Parentis recursive.The change is additive: the surface-area baseline only gains lines.
NavigableContainerkeeps its structuralequality and comparison, which the struct record derives as well.
SurfaceAreaTestpasses against the updatedFSharp.Compiler.Service.SurfaceArea.netstandard20.bsl. Theconsumer, a persistent cache for Navigate To in
FSharp.Editor, is a separate PR that builds on this one.🤖 Generated with Claude Code