internal/lsp: honor GOPRIVATE in documentLinks and go.mod hovers
Several fixes related to GOPRIVATE handling and links:
+ In Go source, fix links matching GOPRIVATE for external modules.
Previously, in these cases we'd try to match <mod>@v1.2.3/<suffix>,
which wasn't the correct input into the GOPRIVATE matching algorithm.
+ Similarly check GOPRIVATE for go.mod require statement hovers.
+ Likewise, for documentLink requests (both mod and source).
+ Move the existing hover regtest to link_test.go, and expand to cover
all these cases.
Along the way, I encountered a couple apparent bugs, which I fixed:
+ Correctly handle the case where there is only one require in a go.mod
file. This was exercised by the regtest, so took some debugging.
+ Only format links [like](this) if the requested format is actually
markdown.
Fixes golang/go#36998
Change-Id: I92011821f646f2a7449dcca619483f83bdeb54b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/238029
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go
index 9ac6e97..d8d618f 100644
--- a/internal/lsp/cache/mod.go
+++ b/internal/lsp/cache/mod.go
@@ -200,7 +200,7 @@
return err
}
whyList := strings.Split(stdout.String(), "\n\n")
- if len(whyList) <= 1 || len(whyList) != len(data.parsed.Require) {
+ if len(whyList) != len(data.parsed.Require) {
return nil
}
data.why = make(map[string]string, len(data.parsed.Require))
diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go
index 5060131..96a94cc 100644
--- a/internal/lsp/fake/editor.go
+++ b/internal/lsp/fake/editor.go
@@ -760,3 +760,12 @@
}
return &resp.Contents, fromProtocolPosition(resp.Range.Start), nil
}
+
+func (e *Editor) DocumentLink(ctx context.Context, path string) ([]protocol.DocumentLink, error) {
+ if e.Server == nil {
+ return nil, nil
+ }
+ params := &protocol.DocumentLinkParams{}
+ params.TextDocument.URI = e.sandbox.Workdir.URI(path)
+ return e.Server.DocumentLink(ctx, params)
+}
diff --git a/internal/lsp/link.go b/internal/lsp/link.go
index ea09d76..98dc3c5 100644
--- a/internal/lsp/link.go
+++ b/internal/lsp/link.go
@@ -56,6 +56,10 @@
}
var links []protocol.DocumentLink
for _, req := range file.Require {
+ // See golang/go#36998: don't link to modules matching GOPRIVATE.
+ if snapshot.View().IsGoPrivatePath(req.Mod.Path) {
+ continue
+ }
dep := []byte(req.Mod.Path)
s, e := req.Syntax.Start.Byte, req.Syntax.End.Byte
i := bytes.Index(m.Content[s:e], dep)
@@ -132,6 +136,10 @@
if err != nil {
continue
}
+ // See golang/go#36998: don't link to modules matching GOPRIVATE.
+ if view.IsGoPrivatePath(target) {
+ continue
+ }
if mod, version, ok := moduleAtVersion(ctx, target, ph); ok && strings.ToLower(view.Options().LinkTarget) == "pkg.go.dev" {
target = strings.Replace(target, mod, mod+"@"+version, 1)
}
diff --git a/internal/lsp/mod/hover.go b/internal/lsp/mod/hover.go
index 642d1bb..2775dea 100644
--- a/internal/lsp/mod/hover.go
+++ b/internal/lsp/mod/hover.go
@@ -26,20 +26,20 @@
mh, err := snapshot.ModHandle(ctx, fh)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("getting modfile handle: %w", err)
}
file, m, why, err := mh.Why(ctx)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("running go mod why: %w", err)
}
// Get the position of the cursor.
spn, err := m.PointSpan(position)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("computing cursor position: %w", err)
}
hoverRng, err := spn.Range(m.Converter)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("computing hover range: %w", err)
}
var req *modfile.Require
@@ -85,7 +85,8 @@
return nil, err
}
options := snapshot.View().Options()
- explanation = formatExplanation(explanation, req, options)
+ isPrivate := snapshot.View().IsGoPrivatePath(req.Mod.Path)
+ explanation = formatExplanation(explanation, req, options, isPrivate)
return &protocol.Hover{
Contents: protocol.MarkupContent{
Kind: options.PreferredContentFormat,
@@ -95,7 +96,7 @@
}, nil
}
-func formatExplanation(text string, req *modfile.Require, options source.Options) string {
+func formatExplanation(text string, req *modfile.Require, options source.Options, isPrivate bool) string {
text = strings.TrimSuffix(text, "\n")
splt := strings.Split(text, "\n")
length := len(splt)
@@ -117,16 +118,17 @@
return b.String()
}
- imp := splt[length-1]
- target := imp
- if strings.ToLower(options.LinkTarget) == "pkg.go.dev" {
- target = strings.Replace(target, req.Mod.Path, req.Mod.String(), 1)
+ imp := splt[length-1] // import path
+ reference := imp
+ // See golang/go#36998: don't link to modules matching GOPRIVATE.
+ if !isPrivate && options.PreferredContentFormat == protocol.Markdown {
+ target := imp
+ if strings.ToLower(options.LinkTarget) == "pkg.go.dev" {
+ target = strings.Replace(target, req.Mod.Path, req.Mod.String(), 1)
+ }
+ reference = fmt.Sprintf("[%s](https://%s/%s)", imp, options.LinkTarget, target)
}
- target = fmt.Sprintf("https://%s/%s", options.LinkTarget, target)
-
- b.WriteString("This module is necessary because ")
- msg := fmt.Sprintf("[%s](%s) is imported in", imp, target)
- b.WriteString(msg)
+ b.WriteString("This module is necessary because " + reference + " is imported in")
// If the explanation is 3 lines, then it is of the form:
// # golang.org/x/tools
diff --git a/internal/lsp/regtest/hover_test.go b/internal/lsp/regtest/hover_test.go
deleted file mode 100644
index 1281f4d..0000000
--- a/internal/lsp/regtest/hover_test.go
+++ /dev/null
@@ -1,54 +0,0 @@
-// 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 (
- "strings"
- "testing"
-
- "golang.org/x/tools/internal/lsp/fake"
-)
-
-const simpleProgram = `
--- go.mod --
-module gopls.test
-
-go 1.12
--- lib/lib.go --
-package lib
-
-const Hello = "Hello"
--- main.go --
-package main
-
-import (
- "fmt"
- "gopls.test/lib"
-)
-
-func main() {
- fmt.Println(lib.Hello)
-}`
-
-func TestHover(t *testing.T) {
- runner.Run(t, simpleProgram, func(t *testing.T, env *Env) {
- // Hover on an empty line.
- env.OpenFile("main.go")
- content, pos := env.Hover("main.go", fake.Pos{Line: 3, Column: 0})
- if content != nil {
- t.Errorf("got non-empty response for empty hover: %v: %v", pos, *content)
- }
- content, pos = env.Hover("main.go", env.RegexpSearch("main.go", "lib.Hello"))
- link := "pkg.go.dev/gopls.test/lib"
- if content == nil || !strings.Contains(content.Value, link) {
- t.Errorf("got hover: %v, want contains %q", content, link)
- }
- env.ChangeEnv("GOPRIVATE=gopls.test")
- content, pos = env.Hover("main.go", env.RegexpSearch("main.go", "lib.Hello"))
- if content == nil || strings.Contains(content.Value, link) {
- t.Errorf("got hover: %v, want non-empty hover without %q", content, link)
- }
- })
-}
diff --git a/internal/lsp/regtest/link_test.go b/internal/lsp/regtest/link_test.go
new file mode 100644
index 0000000..913abf4
--- /dev/null
+++ b/internal/lsp/regtest/link_test.go
@@ -0,0 +1,85 @@
+// 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 (
+ "strings"
+ "testing"
+)
+
+func TestHoverAndDocumentLink(t *testing.T) {
+ const program = `
+-- go.mod --
+module mod.test
+
+go 1.12
+
+require import.test v1.2.3
+-- main.go --
+package main
+
+import "import.test/pkg"
+
+func main() {
+ println(pkg.Hello)
+}`
+
+ const proxy = `
+-- import.test@v1.2.3/go.mod --
+module import.test
+
+go 1.12
+-- import.test@v1.2.3/pkg/const.go --
+package pkg
+
+const Hello = "Hello"
+`
+ runner.Run(t, program, func(t *testing.T, env *Env) {
+ env.OpenFile("main.go")
+ env.OpenFile("go.mod")
+
+ modLink := "https://pkg.go.dev/mod/import.test@v1.2.3"
+ pkgLink := "https://pkg.go.dev/import.test@v1.2.3/pkg"
+
+ // First, check that we get the expected links via hover and documentLink.
+ content, _ := env.Hover("main.go", env.RegexpSearch("main.go", "pkg.Hello"))
+ if content == nil || !strings.Contains(content.Value, pkgLink) {
+ t.Errorf("hover: got %v in main.go, want contains %q", content, pkgLink)
+ }
+ content, _ = env.Hover("go.mod", env.RegexpSearch("go.mod", "import.test"))
+ if content == nil || !strings.Contains(content.Value, pkgLink) {
+ t.Errorf("hover: got %v in go.mod, want contains %q", content, pkgLink)
+ }
+ links := env.DocumentLink("main.go")
+ if len(links) != 1 || links[0].Target != pkgLink {
+ t.Errorf("documentLink: got %v for main.go, want link to %q", links, pkgLink)
+ }
+ links = env.DocumentLink("go.mod")
+ if len(links) != 1 || links[0].Target != modLink {
+ t.Errorf("documentLink: got %v for go.mod, want link to %q", links, modLink)
+ }
+
+ // Then change the environment to make these links private.
+ env.ChangeEnv("GOPRIVATE=import.test")
+
+ // Finally, verify that the links are gone.
+ content, _ = env.Hover("main.go", env.RegexpSearch("main.go", "pkg.Hello"))
+ if content == nil || strings.Contains(content.Value, pkgLink) {
+ t.Errorf("hover: got %v in main.go, want non-empty hover without %q", content, pkgLink)
+ }
+ content, _ = env.Hover("go.mod", env.RegexpSearch("go.mod", "import.test"))
+ if content == nil || strings.Contains(content.Value, modLink) {
+ t.Errorf("hover: got %v in go.mod, want contains %q", content, modLink)
+ }
+ links = env.DocumentLink("main.go")
+ if len(links) != 0 {
+ t.Errorf("documentLink: got %d document links for main.go, want 0\nlinks: %v", len(links), links)
+ }
+ links = env.DocumentLink("go.mod")
+ if len(links) != 0 {
+ t.Errorf("documentLink: got %d document links for go.mod, want 0\nlinks: %v", len(links), links)
+ }
+ }, WithProxy(proxy))
+}
diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go
index e69e7ca..f1c7502 100644
--- a/internal/lsp/regtest/wrappers.go
+++ b/internal/lsp/regtest/wrappers.go
@@ -164,6 +164,15 @@
return c, p
}
+func (e *Env) DocumentLink(name string) []protocol.DocumentLink {
+ e.T.Helper()
+ links, err := e.Editor.DocumentLink(e.Ctx, name)
+ if err != nil {
+ e.T.Fatal(err)
+ }
+ return links
+}
+
func checkIsFatal(t *testing.T, err error) {
t.Helper()
if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrClosedPipe) {
diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go
index 95cd014..ffa85be 100644
--- a/internal/lsp/source/completion_format.go
+++ b/internal/lsp/source/completion_format.go
@@ -195,7 +195,7 @@
if err != nil {
return item, nil
}
- hover, err := ident.Hover(ctx)
+ hover, err := HoverIdentifier(ctx, ident)
if err != nil {
event.Error(ctx, "failed to find Hover", err, tag.URI.Of(uri))
return item, nil
diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go
index d3d26c7..8fc6974 100644
--- a/internal/lsp/source/hover.go
+++ b/internal/lsp/source/hover.go
@@ -34,6 +34,9 @@
// FullDocumentation is the symbol's full documentation.
FullDocumentation string `json:"fullDocumentation"`
+ // ImportPath is the import path for the package containing the given symbol.
+ ImportPath string
+
// Link is the pkg.go.dev anchor for the given symbol.
// For example, "go/ast#Node".
Link string `json:"link"`
@@ -50,7 +53,7 @@
if err != nil {
return nil, nil
}
- h, err := ident.Hover(ctx)
+ h, err := HoverIdentifier(ctx, ident)
if err != nil {
return nil, err
}
@@ -58,8 +61,11 @@
if err != nil {
return nil, err
}
- isPrivate := h.Link != "" && snapshot.View().IsGoPrivatePath(h.Link)
- hover, err := FormatHover(h, snapshot.View().Options(), isPrivate)
+ // See golang/go#36998: don't link to modules matching GOPRIVATE.
+ if snapshot.View().IsGoPrivatePath(h.ImportPath) {
+ h.Link = ""
+ }
+ hover, err := FormatHover(h, snapshot.View().Options())
if err != nil {
return nil, err
}
@@ -72,7 +78,7 @@
}, nil
}
-func (i *IdentifierInfo) Hover(ctx context.Context) (*HoverInformation, error) {
+func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverInformation, error) {
ctx, done := event.Start(ctx, "source.Hover")
defer done()
@@ -95,7 +101,7 @@
if obj := i.Declaration.obj; obj != nil {
h.SingleLine = objectString(obj, i.qf)
}
- h.Link, h.SymbolName = i.linkAndSymbolName()
+ h.ImportPath, h.Link, h.SymbolName = pathLinkAndSymbolName(i)
if h.comment != nil {
h.FullDocumentation = h.comment.Text()
h.Synopsis = doc.Synopsis(h.FullDocumentation)
@@ -103,32 +109,33 @@
return h, nil
}
-func (i *IdentifierInfo) linkAndSymbolName() (string, string) {
+func pathLinkAndSymbolName(i *IdentifierInfo) (string, string, string) {
obj := i.Declaration.obj
if obj == nil {
- return "", ""
+ return "", "", ""
}
switch obj := obj.(type) {
case *types.PkgName:
path := obj.Imported().Path()
+ link := path
if mod, version, ok := moduleAtVersion(path, i); ok {
- path = strings.Replace(path, mod, mod+"@"+version, 1)
+ link = strings.Replace(path, mod, mod+"@"+version, 1)
}
- return path, obj.Name()
+ return path, link, obj.Name()
case *types.Builtin:
- return fmt.Sprintf("builtin#%s", obj.Name()), obj.Name()
+ return "builtin", 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.MappedRange[0].URI()); i.pkg == pkg {
- return "", ""
+ return "", "", ""
}
}
// Don't return links for other unexported types.
if !obj.Exported() {
- return "", ""
+ return "", "", ""
}
var rTypeName string
switch obj := obj.(type) {
@@ -144,7 +151,7 @@
case *types.Func:
typ, ok := obj.Type().(*types.Signature)
if !ok {
- return "", ""
+ return "", "", ""
}
if r := typ.Recv(); r != nil {
switch rtyp := deref(r.Type()).(type) {
@@ -154,7 +161,7 @@
if named, ok := i.enclosing.(*types.Named); ok {
rTypeName = named.Obj().Name()
} else if !rtyp.Obj().Exported() {
- return "", ""
+ return "", "", ""
} else {
rTypeName = rtyp.Obj().Name()
}
@@ -162,16 +169,19 @@
}
}
path := obj.Pkg().Path()
+ link := path
if mod, version, ok := moduleAtVersion(path, i); ok {
- path = strings.Replace(path, mod, mod+"@"+version, 1)
+ link = strings.Replace(path, mod, mod+"@"+version, 1)
}
if rTypeName != "" {
- link := fmt.Sprintf("%s#%s.%s", path, rTypeName, obj.Name())
+ link = fmt.Sprintf("%s#%s.%s", link, rTypeName, obj.Name())
symbol := fmt.Sprintf("(%s.%s).%s", obj.Pkg().Name(), rTypeName, obj.Name())
- return link, symbol
+ return path, link, symbol
}
// For most cases, the link is "package/path#symbol".
- return fmt.Sprintf("%s#%s", path, obj.Name()), fmt.Sprintf("%s.%s", obj.Pkg().Name(), obj.Name())
+ link = fmt.Sprintf("%s#%s", link, obj.Name())
+ symbolName := fmt.Sprintf("%s.%s", obj.Pkg().Name(), obj.Name())
+ return path, link, symbolName
}
func moduleAtVersion(path string, i *IdentifierInfo) (string, string, bool) {
@@ -331,7 +341,7 @@
return &HoverInformation{source: obj, comment: decl.Doc}
}
-func FormatHover(h *HoverInformation, options Options, isPrivate bool) (string, error) {
+func FormatHover(h *HoverInformation, options Options) (string, error) {
signature := h.Signature
if signature != "" && options.PreferredContentFormat == protocol.Markdown {
signature = fmt.Sprintf("```go\n%s\n```", signature)
@@ -349,10 +359,7 @@
}
return string(b), nil
}
- var link string
- if !isPrivate {
- link = formatLink(h, options)
- }
+ link := formatLink(h, options)
switch options.HoverKind {
case SynopsisDocumentation:
doc := formatDoc(h.Synopsis, options)
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 84c6ab0..e08260e 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -489,11 +489,11 @@
if err != nil {
t.Fatalf("failed for %v: %v", d.Src, err)
}
- h, err := ident.Hover(r.ctx)
+ h, err := source.HoverIdentifier(r.ctx, ident)
if err != nil {
t.Fatalf("failed for %v: %v", d.Src, err)
}
- hover, err := source.FormatHover(h, r.view.Options(), false)
+ hover, err := source.FormatHover(h, r.view.Options())
if err != nil {
t.Fatal(err)
}
