This PR

• Adds moniker output to the lsif-go binary
• Adds a new utility to add read go.mod and go.sum to and rewrite monikers for external use
• Adds support for stdout output for lsif-go (this allows us to stream lsif-go output directly to lsif-gomod)
• Refactors the binary arguments to act like lsif-tsc (now that we have two cmds, this was a good opportunity)

After spending all day debugging, I think I finally understand what is going on here and answered my own issue :) PTAL @efritz

Helps https://github.com/sourcegraph/sourcegraph/issues/19591

The first commit

This prepares the indexer to index textDocument/documentSymbols. The problem here is that indexing symbols involves emitting ranges for the symbol definitions, and so does our existing indexing for indexDefinition. LSIF ranges cannot overlap or be duplicative, so we must effectively make symbol and definition indexing work together. The idea is that symbol indexing will emit ranges for the definitions it encounters, and traditional definition indexing will merely reuse those or emit a new range if needed.

At present, this causes all tests to fail due to the way we pre-emptively mark a range for generation during definition indexing:

--- FAIL: TestIndexer (0.63s)
    --- FAIL: TestIndexer/check_Parallel_function_hover_text (0.00s)
        indexer_test.go:35: could not find target range
    --- FAIL: TestIndexer/check_external_package_hover_text (0.00s)
        indexer_test.go:62: could not find target range
    --- FAIL: TestIndexer/check_errs_definition (0.00s)
        indexer_test.go:94: incorrect definition count. want=1 have=0
    --- FAIL: TestIndexer/check_wg_references (0.00s)
        indexer_test.go:108: incorrect reference count. want=4 have=3
    --- FAIL: TestIndexer/check_NestedB_monikers (0.00s)
        indexer_test.go:122: could not find target range
    --- FAIL: TestIndexer/check_typeswitch (0.00s)
        indexer_test.go:143: could not find target range
    --- FAIL: TestIndexer/check_typealias (0.00s)
        indexer_test.go:209: could not find target range
    --- FAIL: TestIndexer/check_typealias_reference (0.00s)
        indexer_test.go:255: incorrection definition count. want=1 have=0
    --- FAIL: TestIndexer/check_typealias_anonymous_struct (0.00s)
        indexer_test.go:296: could not find target range
FAIL
FAIL	github.com/sourcegraph/lsif-go/internal/indexer	17.393s
FAIL

The second commit

indexer: remove pre-emptive range marking for definitions

Prior to this change, we would pre-emptively "mark" a range for a definition as being actively produced. This appears to have been because indexDefinition would always emit a new range, and this it was necessary to ensure that two parallel definition indexing processes would not emit the same range.

Since my prior commit, however, indexDefinition only emits a new range if one does not exist. So pre-emptive range marking is not required anymore.

This causes all tests to pass.

Code like the following:

switch v := obj.(type) {
  case A: f1(v, v)
  case B: f2(v, v)
}

does not get indexed properly because when processing definitions, the definition for v has a nil associated def object, since there's a different def object for the v in each branch. Then when we encounter each use of v, there is an associated def object but since it's not indexed in our definitions we only output a hover result.

Discussion:

• If there's a way to know that this is why the def object is nil during the definitions pass, that would likely be the right way to solve.
• What LSIF should we actually output here? IMO:
  • Each case branch should have a different hover result cause they have different types.
  • The def result is canonical.
  • I think the reference result should point to all uses in every branch. Kind of weird to have a reference return results with different types, but I think the experience would be weirder if you hover over one of these and only the symbols in that branch highlight. Also the definition would need to have everything as a reference result anyway.
  • Open question: what should go in the hover result when hovering the definition?

Therefore the resulting LSIF structure should be:

vdef -> resultSet1
resultSet1 -> hoverResult1
resultSet1 -> resultSetShared
resultSetShared -> defResult1
resultSetShared -> refResult1
defResult1 -> vdef
refResult1 -> vdef
vref1 -> resultSet2
resultSet2 -> hoverResult2
resultSet2 -> resultSetShared
refResult1 -> vref1
vref2 -> resuiltSet2
refResult1 -> vref2
vref3 -> resultSet3
resultSet3 -> hoverResult3
resultSet3 -> resultSetShared
refResult1 -> vref3
vref4 -> resuiltSet3
refResult1 -> vref4

Thoughts?

Also how does the worker process this graph?

In the dump of gorilla/mux, there is an edge

{
  id: '12458',
  type: 'edge',
  label: 'contains',
  outV: '96',
  inVs: null
}

this is invalid, inVs should be an empty array or this edge should not be included.

I read some blogs for LSIF protocol recently, but I only found LSIF servers written in typescript. Is there any plan to write a golang version of server to serve the LSIF dumps?

Looking at the images on Docker Hub the latest one is f9c2d9cf3bdf which is running go 1.14.5 but before that image was even made the Dockerfile was updated in the repo to 1.16. Shouldn't the latest images on Docker Hub be running 1.16 as well?

This reduces the image size from 841MB to 346MB, should give a significant boost in startup time. Q: Do we rely on any tools other than git to be installed?

This is pulled out of https://github.com/sourcegraph/lsif-go/pull/129#discussion_r580296179 - but with actual tests for it and more documentation explaining what it's doing and why.

Helps https://github.com/sourcegraph/sourcegraph/issues/19591

Similar to the way we preload hover text (see https://github.com/sourcegraph/lsif-go/commit/c165bfff52e9d4f87891bba497e3b70fea144d89), we can also preload nested container names for moniker paths. This saves us from re-tracing the same AST nodes.

We piggyback on the hover text preload step and keep track of the relevant parent nodes and stash them into a map for later fast retrieval. There's still some small opportunities here for cleaner code which I'll probably tackle later (for example, we keep the entire ast path to the current node but we only look at the last three, and only look at certain node types - we can store fewer things during this traversal).

Reviewers: The logic changes are in the first two commits. The remaining commits are code style changes, a struct rename (HoverLoader to Preloader), and test updates.

The lsif-tsc utility uses either strings or uuid strings, not int-like strings. This change will make our implementation mirror that a bit more closely (with some small space savings during transmission as well).

Hey! We use Please, which means that we don't have a go.mod in the root of our repo. Unfortunately this tool assumes that it can use go list and other go build specific commands during initialisation. Other users might be using Bazel or similar, and will encounter similar problems, where these commands won't necessarily work as expected or at all.

Fortunately, golang.org/x/tools/go/packages supports plugging in a different "go list driver" (example), so in theory, the indexer should be able to glean all the information it needs. This tool likely would need to be re-worked so that it doesn't require as much information up front about the "module" and it's dependencies, as it may not be indexing a module.

On a repo with a private dependency, the command go mod download fails when git prompts for a username + password.

There are a few ways to authenticate:

• Username + password
• GitHub token
• SSH key

Perhaps the info could be passed via environment variables or mounting files in the lsif-go container.

This works: https://sourcegraph.com/github.com/sourcegraph/sourcegraph@1fe3199/-/blob/internal/codeintel/dependencies/internal/store/store.go?L53:17#tab=implementations_go

This doesn't: https://sourcegraph.com/github.com/sourcegraph/sourcegraph@1fe319919c52d420f44e763f622644d4718e5e5f/-/blob/internal/database/global_state.go?L45:28#tab=implementations_go

Both have precise code intel.

It looks like when we have type X = p.X, the hover doc (and hence go to definition etc) end up referring to the X on the LHS instead of p.X. (try it here)

Originally reported by Robert Lin in Slack.

See screen recording below. Right now, it is taking me to https://sourcegraph.com/github.com/golang/go@4607ebc7d897c36687ae4ede3cfd4939c6dd0a54/-/blob/misc/cgo/testplugin/plugin_test.go?L9:3 despite having precise code intel (sorry the 'Precise' label isn't showing in the recording; I was doing it for accessibility).

https://user-images.githubusercontent.com/93103176/167314970-ba50c302-71e7-4cb4-8367-4d695c0e9709.mov

At https://sourcegraph.com/github.com/C-EO/sourcegraph@f6e8b61465c2e0474011b62819898179d2f36a90/-/blob/cmd/server/shared/copy.go?L73, the code intel popover for Writer in io.Writer says that the code intel is "Precise". However, if I click Go to Definition, it takes me to https://sourcegraph.com/github.com/golang/go@ 63b968f4f86f4c23ce92b7ac2feda4fc7ca17c8e/-/blob/src/io/multi_test.go?L84:3&subtree=true, which is not the definition of Go's io.Writer.

Go 1.18 with generics will be released this month (March 2022). We need to add support for generics (e.g. Go to Definition for type parameters in type signatures) so that people can continue using precise code intel when they bump their toolchain version. I'm not entirely sure how much work this will require; if it needs to be broken up, this can be used as an umbrella issue with various sub-issues organized in a TODO list (with - [ ]).