From d926bd178cf0635076e574bc8f3f244ada2f4116 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Thu, 13 Aug 2020 12:48:39 -0400 Subject: [PATCH] internal/lsp/cache: always type check in default mode Every package has a default type checking mode dictated by whether it's in the workspace or not. Some features force full rather than exported type checking, but AFAICT that ends up being more harm than good. For example, let's say we want to Find References on fmt.Printf in the stdlib. Before this CL, we'd force a new type check of the fmt package, then find no references because nothing else would have been checked against that new version. While there may be some features that work better in the current regime, I can't think of any, and we have no test coverage for them. So I'd rather start with what makes sense, and if we want to change it maybe let's write some tests. Change-Id: Iea589efb4b4374fd2a54451c868b6e2bd5484e20 Reviewed-on: https://go-review.googlesource.com/c/tools/+/248380 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/check.go | 12 ++++---- internal/lsp/cache/load.go | 2 +- internal/lsp/cache/snapshot.go | 26 ++++++---------- internal/lsp/regtest/references_test.go | 41 +++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 24 deletions(-) create mode 100644 internal/lsp/regtest/references_test.go diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 62067d4efe..3727eadcd6 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -56,7 +56,11 @@ type packageData struct { } // buildPackageHandle returns a packageHandle for a given package and mode. -func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) { +func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID) (*packageHandle, error) { + mode := source.ParseExported + if _, ok := s.isWorkspacePackage(id); ok { + mode = source.ParseFull + } if ph := s.getPackage(id, mode); ph != nil { return ph, nil } @@ -141,11 +145,7 @@ 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 []packageHandleKey for _, depID := range depList { - mode := source.ParseExported - if _, ok := s.isWorkspacePackage(depID); ok { - mode = source.ParseFull - } - depHandle, err := s.buildPackageHandle(ctx, depID, mode) + depHandle, err := s.buildPackageHandle(ctx, depID) if err != nil { event.Error(ctx, "no dep handle", err, tag.Package.Of(string(depID))) if ctx.Err() != nil { diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index c0eb5d01b3..2e82e6ea4c 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -152,7 +152,7 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { if err != nil { return err } - if _, err := s.buildPackageHandle(ctx, m.id, source.ParseFull); err != nil { + if _, err := s.buildPackageHandle(ctx, m.id); err != nil { return err } } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 2f65ad2257..3cb0ac4f10 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -283,7 +283,7 @@ func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI) ([]source. // Get the list of IDs from the snapshot again, in case it has changed. var pkgs []source.Package for _, id := range s.getIDsForURI(uri) { - pkg, err := s.checkedPackage(ctx, id, source.ParseFull) + pkg, err := s.checkedPackage(ctx, id) if err != nil { return nil, err } @@ -292,8 +292,8 @@ func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI) ([]source. return pkgs, nil } -func (s *snapshot) checkedPackage(ctx context.Context, id packageID, mode source.ParseMode) (*pkg, error) { - ph, err := s.buildPackageHandle(ctx, id, mode) +func (s *snapshot) checkedPackage(ctx context.Context, id packageID) (*pkg, error) { + ph, err := s.buildPackageHandle(ctx, id) if err != nil { return nil, err } @@ -312,7 +312,7 @@ func (s *snapshot) GetReverseDependencies(ctx context.Context, id string) ([]sou var pkgs []source.Package for id := range ids { - pkg, err := s.checkedPackage(ctx, id, source.ParseFull) + pkg, err := s.checkedPackage(ctx, id) if err != nil { return nil, err } @@ -448,7 +448,7 @@ func (s *snapshot) WorkspacePackages(ctx context.Context) ([]source.Package, err } var pkgs []source.Package for _, pkgID := range s.workspacePackageIDs() { - pkg, err := s.checkedPackage(ctx, pkgID, source.ParseFull) + pkg, err := s.checkedPackage(ctx, pkgID) if err != nil { return nil, err } @@ -464,27 +464,19 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.Package, error) // The WorkspaceSymbols implementation relies on this function returning // workspace packages first. - wsPackages := s.workspacePackageIDs() - var otherPackages []packageID + ids := s.workspacePackageIDs() s.mu.Lock() for id := range s.metadata { if _, ok := s.workspacePackages[id]; ok { continue } - otherPackages = append(otherPackages, id) + ids = append(ids, id) } s.mu.Unlock() var pkgs []source.Package - for _, id := range wsPackages { - pkg, err := s.checkedPackage(ctx, id, source.ParseFull) - if err != nil { - return nil, err - } - pkgs = append(pkgs, pkg) - } - for _, id := range otherPackages { - pkg, err := s.checkedPackage(ctx, id, source.ParseExported) + for _, id := range ids { + pkg, err := s.checkedPackage(ctx, id) if err != nil { return nil, err } diff --git a/internal/lsp/regtest/references_test.go b/internal/lsp/regtest/references_test.go new file mode 100644 index 0000000000..2ef577ecc0 --- /dev/null +++ b/internal/lsp/regtest/references_test.go @@ -0,0 +1,41 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package regtest + +import ( + "testing" +) + +func TestNonWorkspaceReferences(t *testing.T) { + const files = ` +-- go.mod -- +module mod.com + +-- main.go -- +package main + +import "fmt" + +func main() { + fmt.Print() +} +` + + run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + file, pos := env.GoToDefinition("main.go", env.RegexpSearch("main.go", `fmt.(Print)`)) + refs, err := env.Editor.References(env.Ctx, file, pos) + if err != nil { + t.Fatal(err) + } + if len(refs) != 2 { + t.Fatalf("got %v reference(s), want 2", len(refs)) + } + // The first reference is guaranteed to be the definition. + if got, want := refs[1].URI, env.Sandbox.Workdir.URI("main.go"); got != want { + t.Errorf("found reference in %v, wanted %v", got, want) + } + }) +}