We don't actually use the non-flattened occurrences yet, but I wanted to maintain it here so we can use that higher-fidelity overlapping set of occurrences once we have a UI that can support that.

There is a small optimization opportunity here; if you interned the symbol strings in a map/set, that would likely help reduce memory usage, especially for large files.

Not sure I'm following. Wouldn't the symbol string of occurrences be unique within a file, meaning interning them would have no effect? Or would I need to do something like trim the common prefix (like I seem to remember we do for storage)?

Occurrences that are definitions/references to the same semantic symbol should have the same symbol string. E.g. if you have:

package abc

func Loop() {
    Loop()
}

You'll end up with two copies of a symbol string like scip-go gomod <pkg> v0.1.0 "github.com/sourcegraph/abc".abc/Loop() (the exact format may be different).

This is how the fast path for locals code navigation in the client code works today (albeit, the data there is coming from Tree-sitter, and not from precise data, so the string lengths are shorter). https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/client/web/src/enterprise/codeintel/useSearchBasedCodeIntel.ts?L122:10-122:44

Ah! Got it -- thanks!

Not exactly part of this PR, but:

1. Is the first sort call inside this function actually needed for the new call-site?
2. It seems a bit odd that this function is calling sort again at the end.
3. Maybe this is covered by unit tests, but I'm having trouble justifying that the logic in this function does what it is supposed to do:

• First, it is sorting the occurrences by range in descending order (sort() followed by reverse())
• It is iterating backwards through the occurrences (perhaps so that it can use pop/push instead of raw index manipulation)
  • While iterating, it seems to hit this in the overlapping case, which seems odd

            if (current.range.isOverlapping(next.range)) {
                pushResult(current.withEndPosition(next.range.start))
                stack.push(current.withStartPosition(next.range.end))

Here, it will end up materializing a fake range with start = next.range.end and end = current.range.start which will be a zero or negative length range, since we expect that current.range.start <= next.range.start <= next.range.end

Thoughts on adding some tests for this to make sure the index logic works correctly in all cases?

Just double checking: I assume this won't suddenly cause 'No hover information' to be shown for e.g. keywords in the React app because er still return early in line 254 if we don't have an occurrence at that position anyway.

Occurrences that are definitions/references to the same semantic symbol should have the same symbol string. E.g. if you have:

package abc

func Loop() {
    Loop()
}

You'll end up with two copies of a symbol string like scip-go gomod <pkg> v0.1.0 "github.com/sourcegraph/abc".abc/Loop() (the exact format may be different).

This is how the fast path for locals code navigation in the client code works today (albeit, the data there is coming from Tree-sitter, and not from precise data, so the string lengths are shorter). https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/client/web/src/enterprise/codeintel/useSearchBasedCodeIntel.ts?L122:10-122:44

How exactly do you want this facet to work? Usually facets enable multiple places to provide input, which is then aggregated somehow (either by default or in a custom way). E.g. if you want to allow input to be provided from multiple sources, I'd change the definition to

export const codeGraphData = Facet.define<CodeGraphData>()

and then provide the input as

$: codeGraph = codeGraphDataFacet.computeN([], () => codeGraphData)

(at least I think that works)

If having static: true is important then we can't use compute I think. In which case, if you still want to allow multiple inputs, we should do

export const codeGraphData = Facet.define<CodeGraphData[], CodeGraphData[]>({
    static: true,
    combine: values => values.flat()
})

Of course if you only want to accept the most high pri input then what you have here is fine, though not obvious without a doc comment.

I don't think having static: true is important, and right now we should only ever have one value for this facet, so I didn't bother making it work for multiple inputs (though we possibly should in the future). Will fixup and add a comment to clarify.