Add support for find implementations #199

chrismwendt · 2021-09-09T21:55:34Z

This makes lsif-go emit interface/type implementation data necessary to support find-implementations.

List all the interfaces and concrete types for the stdlib and current project (but not other dependencies - we keep hitting an error "missing go.sum entry")
Emit a textDocument/implementation for each pair (interface, concrete type) that matches
Emit "interface monikers" (new term) for cross-repo find implementations
Also list other dependencies
Emit package information
Emit textDocumentation/implementations for methods + cross repo

Benchmarks

Indexing sourcegraph/sourcegraph:

lsif-go@master: took 20s wall 45s user 336MB dump
lsif-go@implementations: took 29s wall 69s user 339MB dump (8s to load deps, 0.5s to index implementations)

Indexing aws/aws-sdk-go:

lsif-go@master: took 38s wall 106s user 2.347GB dump
lsif-go@implementations: took 45s wall 123s user 2.386GB dump (1.5s to load deps, 2.7s to index implementations)

Conclusion: ~25% slower, ~1% bigger dumps

efritz · 2021-09-09T22:16:12Z

internal/indexer/indexer.go

+		name := dep.Name
+		name = strings.TrimPrefix(name, "https://")
+		name = strings.TrimPrefix(name, "https:/")
+		pkgs, err := packages.Load(config, name)


lol @tjdevries you were right

of course, but about what? 😆 We saw this while we were testing stuff out today and figured we had to do something like this.

I gave you crap about us only calling package.Load once and future proofing with that cache 😃

internal/indexer/helpers_test.go

internal/indexer/indexer.go

tjdevries · 2021-09-21T13:50:35Z

internal/indexer/indexer.go

+
+	start1 := time.Now()
+	localInterfaces, localConcreteTypes := extractTypes(i.packages)
+	remoteInterfaces, remoteConcreteTypes := extractTypes(deps)


i'd still be somewhat interested in us seeing if we can calculate the implementations on a per-dependency basis (and then parallelize that via go funcs) and see if we get any noticeable performance improvements on large repos.

tjdevries · 2021-09-21T13:57:02Z

internal/indexer/indexer.go

+			if shouldInvert {
+				relation = invert(relation)
+				leftDefs, rightDefs = rightDefs, leftDefs
+			}


I have been wondering a bit about whether we should teach the backend that this language is reciprocal or whether we should actually emit both ways.

For example, if we say "struct X implements interface Y", is it always true that "interface Y is implemented by X"?

If so, then we do not need to emit everything for both directions. We can simply have:

X --textDocument/implement --> implementationResult --item--> Y

And the backend must handle the face that the reverse relationship is true as well.

cc @efritz so we can chat about this.

The relationship should be symmetric (imo, just by intuition). I was chatting with @chrismwendt about this and I think it might have some tendrils in the fact that we need to emit relationships for both:

interfaces defined in a package and a conforming struct defined in a dependency, and

structs defined in a package and a descriptive interface of that struct defined in a dependency

Are defs/refs symmetric, too? (I think so) If so, should indexers only emit one direction and the backend fill in the other? If not, there are probably some practical or principled reason(s) why that can help us decide whether or not implementations should follow the same model.

This question applies to implementation relationships within an LSIF dump, but not across dumps because cross-dump find-implementations uses monikers rather than textDocument/implementation et al.

internal/indexer/implementation.go

efritz · 2021-10-27T14:23:50Z

internal/indexer/indexer.go

-			if err != nil {
-				errs <- errors.Wrap(err, "packages.Load")
-				return
+		load := func(cache map[string][]*packages.Package, patterns []string) ([]*packages.Package, bool) {


Suggested change

load := func(cache map[string][]*packages.Package, patterns []string) ([]*packages.Package, bool) {

load := func(cache map[string][]*packages.Package, patterns ...string) ([]*packages.Package, bool) {

this would be way cooler 😎

internal/indexer/indexer.go

efritz · 2021-10-27T14:26:10Z

internal/indexer/indexer.go

+
+// listProjectDependencies finds any packages from "$ go list all" that are NOT declared
+// as part of the current project.
+func (i *Indexer) listProjectDependencies() ([]string, error) {


Couple comments related to this method (but not exactly about this method):

this might be able to go away if dependencies fits your use-case here (it should)

this should be called pre-indexing if it stays because all shell commands should happen in main; indexer shouldn't have to know much about the OS

Did anything here change regarding the dependencies thing from Chris's other comment?

I can move this to the pre-indexing stage regardless

Yeah so it looks like we shouldn't use -m for these in particular, but it would be cool to have this moved into the gomod package and called from main and passed to the indexer instead.

efritz · 2021-10-27T14:27:06Z

internal/indexer/moniker.go

+	// universeParent := toMethod.Obj().(*types.Func).Type().(*types.Signature).Recv().Type().String()
+
+	pkg := obj.Pkg()
+	if pkg == nil {


Comments here would be real neat :)

efritz · 2021-10-27T14:28:15Z

internal/indexer/indexer.go

+			errs <- errors.Wrap(err, "failed to list dependencies")
+			return
+		}
+		i.depPackages, hasError = load(cachedDepPackages, deps)


Isn't this implicitly done by loading ./...?

efritz · 2021-10-27T14:32:13Z

internal/indexer/implementation.go

+
+// filterToExported removes any nonExported types or identifiers from a list of []implDef
+func filterToExported(defs []implDef) []implDef {
+	filtered := []implDef{}


Suggested change

filtered := []implDef{}

filtered := defs[:0]

If this can be done in place:

internal/indexer/implementation.go

efritz · 2021-10-27T14:36:06Z

internal/indexer/implementation.go

+	localTypesToRemoteInterfaces := i.buildImplementationRelation(localConcreteTypes, filterToExported(remoteInterfaces))
+	localTypesToRemoteInterfaces.forEachImplementation(i.emitRemoteImplementation)
+
+	localInterfacesToRemoteTypes := invert(i.buildImplementationRelation(filterToExported(remoteConcreteTypes), localInterfaces))


Can each of these blocks have a short comment?

efritz · 2021-10-27T14:40:24Z

internal/indexer/implementation.go

+		// Find all the concrete types that implement this interface.
+		// Types that implement this interface are the intersection
+		// of all sets of receivers of all methods in this interface.
+		candidateTypes := &intsets.Sparse{}
+
+		if initialReceivers, ok := methodToReceivers[canonical(methods[0])]; !ok {
+			continue
+		} else {
+			candidateTypes.Copy(initialReceivers)
+		}
+
+		for _, method := range methods[1:] {
+			receivers, ok := methodToReceivers[canonical(method)]
+			if !ok {
+				continue interfaceLoop
+			}
+
+			candidateTypes.IntersectionWith(receivers)
+			if candidateTypes.IsEmpty() {
+				continue interfaceLoop
+			}
+		}


I wish there was a cleaner way to write this as an obvious fold. I think the underlying logic here is really beautiful but the syntax makes it a bit messy and really obscures the intent and simplicity behind it 😞

tjdevries · 2021-11-02T16:49:38Z

So this is one thing I'm not sure about -- putting here to talk about w/ @efritz and/or @chrismwendt at some point.

tjdevries · 2021-11-04T19:08:51Z

This PR creates the issue: #208

efritz reviewed Sep 9, 2021

View reviewed changes

chrismwendt mentioned this pull request Sep 10, 2021

Find implementations for Go sourcegraph/sourcegraph-public-snapshot#24479

Closed

8 tasks

tjdevries reviewed Sep 21, 2021

View reviewed changes

internal/indexer/helpers_test.go Show resolved Hide resolved

tjdevries reviewed Sep 21, 2021

View reviewed changes

internal/indexer/indexer.go Outdated Show resolved Hide resolved

tjdevries reviewed Sep 21, 2021

View reviewed changes

internal/indexer/indexer.go Outdated Show resolved Hide resolved

tjdevries reviewed Sep 21, 2021

View reviewed changes

chrismwendt force-pushed the implementations branch from 58a2d9f to a17e464 Compare September 21, 2021 21:26

chrismwendt mentioned this pull request Sep 25, 2021

Uncommitted API docs changes when running tests #202

Closed

chrismwendt and others added 21 commits September 25, 2021 18:32

WIP

64747f0

wip

f6379d2

wip

d6b0d22

add implementation tests

5480263

update sourcegraph lsif protocol dep

b2c65da

go faster

675f99a

[WIP]: Wed 15 Sep 2021 12:42:30 PM EDT

b3d6c4b

wip on comparing both approaches for correctness

ae10e89

emoji on failure

08c82e3

use combinedMethodSet

85bace5

fix bugs

3258a84

fix segfault

2ad054a

[WIP]: Wed 15 Sep 2021 05:25:58 PM EDT

10c2ac4

fix more bugs

a131104

refactor

2e63670

fix dep load bug (had to pass packages, not modules, to packages.Load())

78b5ec9

refactor

08753b2

remove duplicates in the list of dependencies

b3aea23

refactor

0e2270c

first iteration of methodlen stuff

5ff4e31

sent to emit

4b7bd94

continue cleanup

4def68b

tjdevries reviewed Oct 25, 2021

View reviewed changes

internal/indexer/implementation.go Outdated Show resolved Hide resolved

tjdevries added 4 commits October 25, 2021 15:25

attempt to be more DRY

5c253dc

attempt to simplify logic for shared impls

24c8ab2

update namings

cb6c6a0

[WIP]: Tue 26 Oct 2021 02:56:15 PM EDT

1e8e7ba

efritz reviewed Oct 27, 2021

View reviewed changes

internal/indexer/indexer.go Outdated Show resolved Hide resolved

efritz reviewed Oct 27, 2021

View reviewed changes

internal/indexer/implementation.go Show resolved Hide resolved

efritz reviewed Oct 27, 2021

View reviewed changes

tjdevries added 8 commits October 27, 2021 10:44

cleanup and fix tests

2371dc0

I think this is less confusing

d497bc7

even more cleanups

e57c21f

[WIP]: Wed 27 Oct 2021 04:56:21 PM EDT

53330dc

wow stars are hard

62058d4

answer eric comments

82dd187

fixup: golint-ci fixups (could be placed in separate PR)

fe687d6

fixup: move project deps into pre-indexing

f98a20f

fixup: some comments

4f553c0

tjdevries merged commit f8617a7 into master Nov 4, 2021

tjdevries deleted the implementations branch November 4, 2021 19:10

This was referenced Nov 6, 2021

Why are some files missing from package.Syntax? #211

Open

lsif-go OOMing in CI sourcegraph/sourcegraph-public-snapshot#27211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for find implementations #199

Add support for find implementations #199

chrismwendt commented Sep 9, 2021 •

edited

Loading

efritz Sep 9, 2021

tjdevries Sep 10, 2021

efritz Sep 10, 2021

tjdevries Sep 21, 2021 •

edited by chrismwendt

Loading

tjdevries Sep 21, 2021

efritz Sep 21, 2021

chrismwendt Sep 21, 2021

efritz Oct 27, 2021

efritz Oct 27, 2021

tjdevries Oct 28, 2021

efritz Oct 28, 2021

efritz Oct 27, 2021

efritz Oct 27, 2021

efritz Oct 27, 2021

efritz Oct 27, 2021

efritz Oct 27, 2021

tjdevries commented Nov 2, 2021 •

edited

Loading

tjdevries commented Nov 4, 2021

	load := func(cache map[string][]packages.Package, patterns []string) ([]packages.Package, bool) {
	load := func(cache map[string][]packages.Package, patterns ...string) ([]packages.Package, bool) {

Add support for find implementations #199

Add support for find implementations #199

Conversation

chrismwendt commented Sep 9, 2021 • edited Loading

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjdevries Sep 21, 2021 • edited by chrismwendt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjdevries commented Nov 2, 2021 • edited Loading

tjdevries commented Nov 4, 2021

chrismwendt commented Sep 9, 2021 •

edited

Loading

tjdevries Sep 21, 2021 •

edited by chrismwendt

Loading

tjdevries commented Nov 2, 2021 •

edited

Loading