Skip to content

Commit

Permalink
internal/lsp: always ParseFull in-workspace dependencies
Browse files Browse the repository at this point in the history
When searching for implementations we look at all packages in the
workspace. We do a full parse since we need to look for non-exported
types and look in functions for type declarations. However, we always
type check a package's dependencies in export-only mode to save work.
This leads to what I call the "two world" syndrome where you have both
the export-only and full-parse versions of a package in play at once.
This is problematic because mirror objects in each version do not
compare equal.

For example:

-- a/a.go --
package a

type Breed int
const Mutt Breed = 0

type Dog interface{ Breed() Breed }

-- b/b.go --
package b

import "a"

type dog struct{}
func (dog) Breed() a.Breed { return a.Mutt }

---

In this situation, the problem is "b" loads its dependency "a" in
export only mode so it gets one version of the "a.Breed" type. The
user opens package "a" directly so it gets fully type checked and has
a second version of "a.Breed". The user searches for "a.Dog"
implementations, but "b.dog" does not implement the fully-loaded
"a.Dog" because it returns the export-only version of the "a.Breed"
type.

Fix it by always loading in-workspace dependencies in full parse mode.
We need to load them in full parse mode anyway if the user does find
references or find implementations.

In writing a test I fixed an incorrect import in the testdata. This
uncovered an unrelated bug which made a different implementation test
very flaky. I disabled it for now since I couldn't see a fix simple
enough to slip into this commit.

Fixes golang/go#35857.

Change-Id: I01509f57d54d593e62c895c7ecb93eb5f780bec7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209759
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
  • Loading branch information
muirdm authored and stamblerre committed Dec 4, 2019
1 parent 0d967ef commit d79e56d
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 3 deletions.
6 changes: 5 additions & 1 deletion internal/lsp/cache/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,11 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
// Begin computing the key by getting the depKeys for all dependencies.
var depKeys [][]byte
for _, depID := range depList {
depHandle, err := s.packageHandle(ctx, depID, source.ParseExported)
mode := source.ParseExported
if s.workspacePackages[depID] {
mode = source.ParseFull
}
depHandle, err := s.packageHandle(ctx, depID, mode)
if err != nil {
log.Error(ctx, "no dep handle", err, telemetry.Package.Of(depID))

Expand Down
8 changes: 6 additions & 2 deletions internal/lsp/testdata/implementation/implementation.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package implementation

import "implementation/other"
import "golang.org/x/tools/internal/lsp/implementation/other"

type ImpP struct{} //@ImpP

Expand All @@ -25,5 +25,9 @@ type Foo struct {
}

type U interface {
U() //@mark(IntU, "U"),implementations("U", ImpU),
U() //TODO: fix flaky @implementations("U", ImpU)
}

type cryer int

func (cryer) Cry(other.CryType) {} //@mark(CryImpl, "Cry")
8 changes: 8 additions & 0 deletions internal/lsp/testdata/implementation/other/other.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,11 @@ type Foo struct {

func (Foo) U() { //@mark(ImpU, "U")
}

type CryType int

const Sob CryType = 1

type Cryer interface {
Cry(CryType) //@implementations("Cry", CryImpl)
}

0 comments on commit d79e56d

Please sign in to comment.