From ab391d50b528baae03e0dc4f62adde41b71cc307 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 30 Jan 2020 19:24:33 -0500 Subject: [PATCH] internal/lsp: don't show links in hover for test functions source.Identifier previously was used for references and rename, so it needed to take a package policy. Now, it's only used for definition and hover, so it should always be the narrowest package handle. We can use this fact to determine if the identifier is located in its declaring package, and if that package is a test variant, we don't link to the documentation on pkg.go.dev, since it doesn't exist. Change-Id: I5686828858a3feafb8ff2e4c5964b562f66db9fa Reviewed-on: https://go-review.googlesource.com/c/tools/+/217137 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/check.go | 1 + internal/lsp/cache/pkg.go | 12 ++++++--- internal/lsp/definition.go | 4 +-- internal/lsp/hover.go | 2 +- internal/lsp/source/hover.go | 8 ++++++ internal/lsp/source/identifier.go | 4 +-- internal/lsp/source/source_test.go | 4 +-- internal/lsp/source/view.go | 1 + internal/lsp/testdata/godef/a/a_test.go | 8 ++++++ .../lsp/testdata/godef/a/a_test.go.golden | 26 +++++++++++++++++++ internal/lsp/testdata/godef/a/a_x_test.go | 8 ++++++ .../lsp/testdata/godef/a/a_x_test.go.golden | 26 +++++++++++++++++++ internal/lsp/testdata/summary.txt.golden | 2 +- 13 files changed, 94 insertions(+), 12 deletions(-) create mode 100644 internal/lsp/testdata/godef/a/a_test.go create mode 100644 internal/lsp/testdata/godef/a/a_test.go.golden create mode 100644 internal/lsp/testdata/godef/a/a_x_test.go create mode 100644 internal/lsp/testdata/godef/a/a_x_test.go.golden diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 35edec1fb7..45f940bc65 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -256,6 +256,7 @@ func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode sourc Selections: make(map[*ast.SelectorExpr]*types.Selection), Scopes: make(map[ast.Node]*types.Scope), }, + forTest: m.forTest, } var ( files = make([]*ast.File, len(pkg.compiledGoFiles)) diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index f7ae41379a..c706b5503c 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -18,10 +18,10 @@ import ( // pkg contains the type information needed by the source package. type pkg struct { // ID and package path have their own types to avoid being used interchangeably. - id packageID - pkgPath packagePath - mode source.ParseMode - + id packageID + pkgPath packagePath + mode source.ParseMode + forTest packagePath goFiles []source.ParseGoHandle compiledGoFiles []source.ParseGoHandle errors []*source.Error @@ -99,6 +99,10 @@ func (p *pkg) IsIllTyped() bool { return p.types == nil || p.typesInfo == nil || p.typesSizes == nil } +func (p *pkg) ForTest() string { + return string(p.forTest) +} + func (p *pkg) GetImport(pkgPath string) (source.Package, error) { if imp := p.imports[packagePath(pkgPath)]; imp != nil { return imp, nil diff --git a/internal/lsp/definition.go b/internal/lsp/definition.go index e8b8b540d3..bb8fb9005a 100644 --- a/internal/lsp/definition.go +++ b/internal/lsp/definition.go @@ -26,7 +26,7 @@ func (s *Server) definition(ctx context.Context, params *protocol.DefinitionPara if fh.Identity().Kind != source.Go { return nil, nil } - ident, err := source.Identifier(ctx, snapshot, fh, params.Position, source.WidestPackageHandle) + ident, err := source.Identifier(ctx, snapshot, fh, params.Position) if err != nil { return nil, err } @@ -56,7 +56,7 @@ func (s *Server) typeDefinition(ctx context.Context, params *protocol.TypeDefini if fh.Identity().Kind != source.Go { return nil, nil } - ident, err := source.Identifier(ctx, snapshot, fh, params.Position, source.WidestPackageHandle) + ident, err := source.Identifier(ctx, snapshot, fh, params.Position) if err != nil { return nil, err } diff --git a/internal/lsp/hover.go b/internal/lsp/hover.go index 967907ab5f..3305189887 100644 --- a/internal/lsp/hover.go +++ b/internal/lsp/hover.go @@ -26,7 +26,7 @@ func (s *Server) hover(ctx context.Context, params *protocol.HoverParams) (*prot if fh.Identity().Kind != source.Go { return nil, nil } - ident, err := source.Identifier(ctx, snapshot, fh, params.Position, source.WidestPackageHandle) + ident, err := source.Identifier(ctx, snapshot, fh, params.Position) if err != nil { return nil, nil } diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go index 43bcd1bc7f..586e817ecf 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -85,6 +85,14 @@ func (i *IdentifierInfo) linkAndSymbolName() (string, string) { case *types.Builtin: return fmt.Sprintf("builtin#%s", obj.Name()), obj.Name() } + // Check if the identifier is test-only (and is therefore not part of a + // package's API). This is true if the request originated in a test package, + // and if the declaration is also found in the same test package. + if i.pkg != nil && obj.Pkg() != nil && i.pkg.ForTest() != "" { + if _, pkg, _ := findFileInPackage(i.pkg, i.Declaration.URI()); i.pkg == pkg { + return "", "" + } + } // Don't return links for other unexported types. if !obj.Exported() { return "", "" diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 26943049f3..bd3c8f4dc8 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -48,11 +48,11 @@ type Declaration struct { // Identifier returns identifier information for a position // in a file, accounting for a potentially incomplete selector. -func Identifier(ctx context.Context, snapshot Snapshot, fh FileHandle, pos protocol.Position, selectPackage PackagePolicy) (*IdentifierInfo, error) { +func Identifier(ctx context.Context, snapshot Snapshot, fh FileHandle, pos protocol.Position) (*IdentifierInfo, error) { ctx, done := trace.StartSpan(ctx, "source.Identifier") defer done() - pkg, pgh, err := getParsedFile(ctx, snapshot, fh, selectPackage) + pkg, pgh, err := getParsedFile(ctx, snapshot, fh, NarrowestPackageHandle) if err != nil { return nil, fmt.Errorf("getting file for Identifier: %v", err) } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 2c766b87cf..1020a8ef00 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -484,7 +484,7 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) { if err != nil { t.Fatal(err) } - ident, err := source.Identifier(r.ctx, r.view.Snapshot(), fh, srcRng.Start, source.WidestPackageHandle) + ident, err := source.Identifier(r.ctx, r.view.Snapshot(), fh, srcRng.Start) if err != nil { t.Fatalf("failed for %v: %v", d.Src, err) } @@ -768,7 +768,7 @@ func (r *runner) PrepareRename(t *testing.T, src span.Span, want *source.Prepare } return } - if want.Text == "" && item != nil { + if want.Text == "" { t.Errorf("prepare rename failed for %v: expected nil, got %v", src, item) return } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 053ae7999a..a04fb644cb 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -341,6 +341,7 @@ type Package interface { GetTypesInfo() *types.Info GetTypesSizes() types.Sizes IsIllTyped() bool + ForTest() string GetImport(pkgPath string) (Package, error) Imports() []Package } diff --git a/internal/lsp/testdata/godef/a/a_test.go b/internal/lsp/testdata/godef/a/a_test.go new file mode 100644 index 0000000000..77bd633b6c --- /dev/null +++ b/internal/lsp/testdata/godef/a/a_test.go @@ -0,0 +1,8 @@ +package a + +import ( + "testing" +) + +func TestA(t *testing.T) { //@TestA,godef(TestA, TestA) +} diff --git a/internal/lsp/testdata/godef/a/a_test.go.golden b/internal/lsp/testdata/godef/a/a_test.go.golden new file mode 100644 index 0000000000..ac50b90b95 --- /dev/null +++ b/internal/lsp/testdata/godef/a/a_test.go.golden @@ -0,0 +1,26 @@ +-- TestA-definition -- +godef/a/a_test.go:7:6-11: defined here as ```go +func TestA(t *testing.T) +``` +-- TestA-definition-json -- +{ + "span": { + "uri": "file://godef/a/a_test.go", + "start": { + "line": 7, + "column": 6, + "offset": 39 + }, + "end": { + "line": 7, + "column": 11, + "offset": 44 + } + }, + "description": "```go\nfunc TestA(t *testing.T)\n```" +} + +-- TestA-hover -- +```go +func TestA(t *testing.T) +``` diff --git a/internal/lsp/testdata/godef/a/a_x_test.go b/internal/lsp/testdata/godef/a/a_x_test.go new file mode 100644 index 0000000000..85f21cc766 --- /dev/null +++ b/internal/lsp/testdata/godef/a/a_x_test.go @@ -0,0 +1,8 @@ +package a_test + +import ( + "testing" +) + +func TestA2(t *testing.T) { //@TestA2,godef(TestA2, TestA2) +} diff --git a/internal/lsp/testdata/godef/a/a_x_test.go.golden b/internal/lsp/testdata/godef/a/a_x_test.go.golden new file mode 100644 index 0000000000..dd1d740164 --- /dev/null +++ b/internal/lsp/testdata/godef/a/a_x_test.go.golden @@ -0,0 +1,26 @@ +-- TestA2-definition -- +godef/a/a_x_test.go:7:6-12: defined here as ```go +func TestA2(t *testing.T) +``` +-- TestA2-definition-json -- +{ + "span": { + "uri": "file://godef/a/a_x_test.go", + "start": { + "line": 7, + "column": 6, + "offset": 44 + }, + "end": { + "line": 7, + "column": 12, + "offset": 50 + } + }, + "description": "```go\nfunc TestA2(t *testing.T)\n```" +} + +-- TestA2-hover -- +```go +func TestA2(t *testing.T) +``` diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index e85b9f839b..4b010d814f 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/internal/lsp/testdata/summary.txt.golden @@ -11,7 +11,7 @@ FoldingRangesCount = 2 FormatCount = 6 ImportCount = 7 SuggestedFixCount = 1 -DefinitionsCount = 43 +DefinitionsCount = 45 TypeDefinitionsCount = 2 HighlightsCount = 52 ReferencesCount = 8