internal/godoc: smarter import path resolution
simpleImporter is a little too simple to handle imports
of v2+ packages. This reuses the same logic that go/doc
internally uses for its unexported importByName field.
This also deduplicates a few copies of the same function.
Fixes golang/go#60404
Change-Id: I59665b51c8081d7719c65ebc7c5198a26a6a6964
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/728261
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ethan Lee <ethanalee@google.com>
kokoro-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/internal/godoc/dochtml/dochtml_test.go b/internal/godoc/dochtml/dochtml_test.go
index 12f8536..f57cce3 100644
--- a/internal/godoc/dochtml/dochtml_test.go
+++ b/internal/godoc/dochtml/dochtml_test.go
@@ -26,6 +26,7 @@
"github.com/google/safehtml/template"
"golang.org/x/net/html"
"golang.org/x/pkgsite/internal/godoc/dochtml/internal/render"
+ "golang.org/x/pkgsite/internal/godoc/importer"
"golang.org/x/pkgsite/internal/testing/testhelper"
)
@@ -106,6 +107,101 @@
testhelper.CompareWithGolden(t, got, name+".golden", update)
}
+func TestImportLink(t *testing.T) {
+ LoadTemplates(templateFS)
+
+ cases := []struct {
+ name string
+ path string
+ }{{
+ name: "std",
+ path: "math/rand",
+ }, {
+ name: "std-v2",
+ path: "math/rand/v2",
+ }, {
+ name: "regular",
+ path: "example.com/rand",
+ }, {
+ name: "regular-v2",
+ path: "example.com/rand/v2",
+ }, {
+ name: "go-prefix",
+ path: "example.com/go-rand",
+ }, {
+ name: "go-prefix-v2",
+ path: "example.com/go-rand/v2",
+ }}
+
+ // Create a simple package with the "rand" dependency imported from the given path.
+ createPackage := func(importPath, depPath string) (*token.FileSet, *doc.Package) {
+ const codeTpl = `
+package foo
+
+import %q
+
+func F(rng *rand.Rand) {}
+`
+ code := fmt.Sprintf(codeTpl, depPath)
+
+ fset := token.NewFileSet()
+ astFile, _ := parser.ParseFile(fset, "main.go", code, parser.ParseComments)
+ files := []*ast.File{astFile}
+
+ filesMap := map[string]*ast.File{
+ "main.go": astFile,
+ }
+
+ //lint:ignore SA1019 We had a preexisting dependency on ast.Object.
+ ast.NewPackage(fset, filesMap, importer.SimpleImporter, nil)
+
+ astPackage, err := doc.NewFromFiles(fset, files, importPath, doc.AllDecls)
+ if err != nil {
+ panic(err)
+ }
+
+ return fset, astPackage
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ ctx := t.Context()
+
+ // render out the package
+ fset, d := createPackage("example.com/module/pkg", tc.path)
+ parts, err := Render(ctx, fset, d, testRenderOptions)
+ if err != nil {
+ t.Fatal(err)
+ }
+ htmlDoc, err := html.Parse(strings.NewReader(parts.Body.String()))
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ // Walk the rendered output looking for "rand" in the function signature,
+ // and get the location it's linked to.
+ var declarations *html.Node
+ var got string
+ walk(htmlDoc, func(n *html.Node) {
+ if strings.Contains(attr(n, "class"), "Documentation-declaration") {
+ declarations = n
+ }
+ if n.Data == "rand" {
+ got = attr(n.Parent, "href")
+ }
+ })
+
+ want := "/" + tc.path
+ if got != want {
+ var buf bytes.Buffer
+ html.Render(&buf, declarations)
+ t.Errorf("dep link = %q, want = %q\ndeclarations:\n%s",
+ got, want, buf.String())
+ }
+ })
+ }
+}
+
func TestExampleRender(t *testing.T) {
LoadTemplates(templateFS)
ctx := context.Background()
@@ -483,7 +579,7 @@
}
//lint:ignore SA1019 We had a preexisting dependency on ast.Object.
- ast.NewPackage(fset, filesMap, simpleImporter, nil)
+ ast.NewPackage(fset, filesMap, importer.SimpleImporter, nil)
astPackage, err := doc.NewFromFiles(fset, files, path, doc.AllDecls)
if err != nil {
@@ -492,20 +588,3 @@
return fset, astPackage
}
-
-// simpleImporter returns a (dummy) package object named by the last path
-// component of the provided package path (as is the convention for packages).
-// This is sufficient to resolve package identifiers without doing an actual
-// import. It never returns an error.
-//
-//lint:ignore SA1019 We had a preexisting dependency on ast.Object.
-func simpleImporter(imports map[string]*ast.Object, path string) (*ast.Object, error) {
- pkg := imports[path]
- if pkg == nil {
- // note that strings.LastIndex returns -1 if there is no "/"
- pkg = ast.NewObj(ast.Pkg, path[strings.LastIndex(path, "/")+1:])
- pkg.Data = ast.NewScope(nil) // required by ast.NewPackage for dot-import
- imports[path] = pkg
- }
- return pkg, nil
-}
diff --git a/internal/godoc/dochtml/internal/render/render_test.go b/internal/godoc/dochtml/internal/render/render_test.go
index 483423d..fa775d6 100644
--- a/internal/godoc/dochtml/internal/render/render_test.go
+++ b/internal/godoc/dochtml/internal/render/render_test.go
@@ -11,7 +11,8 @@
"go/token"
"os"
"path/filepath"
- "strings"
+
+ "golang.org/x/pkgsite/internal/godoc/importer"
)
//lint:file-ignore SA1019 We only need the syntax tree.
@@ -21,18 +22,6 @@
var pkgTime, fsetTime = mustLoadPackage("time")
func mustLoadPackage(path string) (*doc.Package, *token.FileSet) {
- // simpleImporter is used by ast.NewPackage.
- simpleImporter := func(imports map[string]*ast.Object, pkgPath string) (*ast.Object, error) {
- pkg := imports[pkgPath]
- if pkg == nil {
- pkgName := pkgPath[strings.LastIndex(pkgPath, "/")+1:]
- pkg = ast.NewObj(ast.Pkg, pkgName)
- pkg.Data = ast.NewScope(nil) // required for or dot-imports
- imports[pkgPath] = pkg
- }
- return pkg, nil
- }
-
srcName := filepath.Base(path) + ".go"
code, err := os.ReadFile(filepath.Join("testdata", srcName))
if err != nil {
@@ -43,6 +32,6 @@
pkgFiles := make(map[string]*ast.File)
astFile, _ := parser.ParseFile(fset, srcName, code, parser.ParseComments)
pkgFiles[srcName] = astFile
- astPkg, _ := ast.NewPackage(fset, pkgFiles, simpleImporter, nil)
+ astPkg, _ := ast.NewPackage(fset, pkgFiles, importer.SimpleImporter, nil)
return doc.New(astPkg, path, 0), fset
}
diff --git a/internal/godoc/importer/importer.go b/internal/godoc/importer/importer.go
new file mode 100644
index 0000000..97c7544
--- /dev/null
+++ b/internal/godoc/importer/importer.go
@@ -0,0 +1,57 @@
+// Copyright 2025 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 importer
+
+import (
+ "go/ast"
+ "path"
+ "strconv"
+ "strings"
+ "unicode"
+ "unicode/utf8"
+)
+
+// SimpleImporter returns a (dummy) package object named by the last path element,
+// stripping off any major version suffix or go- prefix.
+// This is sufficient to resolve most package identifiers without doing an actual
+// import. It never returns an error.
+//
+//lint:ignore SA1019 We had a preexisting dependency on ast.Object.
+func SimpleImporter(imports map[string]*ast.Object, path string) (*ast.Object, error) {
+ pkg := imports[path]
+ if pkg == nil {
+ name := importPathToAssumedName(path)
+ pkg = ast.NewObj(ast.Pkg, name)
+ pkg.Data = ast.NewScope(nil) // required by ast.NewPackage for dot-import
+ imports[path] = pkg
+ }
+ return pkg, nil
+}
+
+// importPathToAssumedName is a copy of golang.org/x/tools/internal/imports.ImportPathToAssumedName
+func importPathToAssumedName(importPath string) string {
+ base := path.Base(importPath)
+ if strings.HasPrefix(base, "v") {
+ if _, err := strconv.Atoi(base[1:]); err == nil {
+ dir := path.Dir(importPath)
+ if dir != "." {
+ base = path.Base(dir)
+ }
+ }
+ }
+ base = strings.TrimPrefix(base, "go-")
+ if i := strings.IndexFunc(base, notIdentifier); i >= 0 {
+ base = base[:i]
+ }
+ return base
+}
+
+// notIdentifier reports whether ch is an invalid identifier character.
+func notIdentifier(ch rune) bool {
+ return !('a' <= ch && ch <= 'z' || 'A' <= ch && ch <= 'Z' ||
+ '0' <= ch && ch <= '9' ||
+ ch == '_' ||
+ ch >= utf8.RuneSelf && (unicode.IsLetter(ch) || unicode.IsDigit(ch)))
+}
diff --git a/internal/godoc/importer/importer_test.go b/internal/godoc/importer/importer_test.go
new file mode 100644
index 0000000..04a7d23
--- /dev/null
+++ b/internal/godoc/importer/importer_test.go
@@ -0,0 +1,47 @@
+// Copyright 2025 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 importer
+
+import (
+ "go/ast"
+ "testing"
+)
+
+func TestSimpleImporter(t *testing.T) {
+ cases := []struct {
+ name string
+ path string
+ }{{
+ name: "std",
+ path: "math/rand",
+ }, {
+ name: "std-v2",
+ path: "math/rand/v2",
+ }, {
+ name: "regular",
+ path: "example.com/rand",
+ }, {
+ name: "regular-v2",
+ path: "example.com/rand/v2",
+ }, {
+ name: "go-prefix",
+ path: "example.com/go-rand",
+ }, {
+ name: "go-prefix-v2",
+ path: "example.com/go-rand/v2",
+ }}
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ //lint:ignore SA1019 We had a preexisting dependency on ast.Object.
+ obj, err := SimpleImporter(make(map[string]*ast.Object), tc.path)
+ if err != nil {
+ t.Fatalf("SimpleImporter(%q) returned error: %v", tc.path, err)
+ }
+ if obj.Name != "rand" {
+ t.Errorf("SimpleImporter(%q).Name = %s, want = rand", tc.path, obj.Name)
+ }
+ })
+ }
+}
diff --git a/internal/godoc/render.go b/internal/godoc/render.go
index 4bd8cde..9d21fbb 100644
--- a/internal/godoc/render.go
+++ b/internal/godoc/render.go
@@ -19,6 +19,7 @@
"golang.org/x/pkgsite/internal"
"golang.org/x/pkgsite/internal/derrors"
"golang.org/x/pkgsite/internal/godoc/dochtml"
+ "golang.org/x/pkgsite/internal/godoc/importer"
"golang.org/x/pkgsite/internal/source"
"golang.org/x/pkgsite/internal/stdlib"
)
@@ -125,7 +126,7 @@
// Call ast.NewPackage for side-effects to populate objects. In Go 1.25+
// doc.NewFromFiles will not cause the objects to be populated.
//lint:ignore SA1019 We had a preexisting dependency on ast.Object.
- ast.NewPackage(p.Fset, nonTestFiles, simpleImporter, nil)
+ ast.NewPackage(p.Fset, nonTestFiles, importer.SimpleImporter, nil)
d, err := doc.NewFromFiles(p.Fset, allGoFiles, importPath, m)
if err != nil {
return nil, fmt.Errorf("doc.NewFromFiles: %v", err)
@@ -150,23 +151,6 @@
return d, nil
}
-// simpleImporter returns a (dummy) package object named by the last path
-// component of the provided package path (as is the convention for packages).
-// This is sufficient to resolve package identifiers without doing an actual
-// import. It never returns an error.
-//
-//lint:ignore SA1019 We had a preexisting dependency on ast.Object.
-func simpleImporter(imports map[string]*ast.Object, path string) (*ast.Object, error) {
- pkg := imports[path]
- if pkg == nil {
- // note that strings.LastIndex returns -1 if there is no "/"
- pkg = ast.NewObj(ast.Pkg, path[strings.LastIndex(path, "/")+1:])
- pkg.Data = ast.NewScope(nil) // required by ast.NewPackage for dot-import
- imports[path] = pkg
- }
- return pkg, nil
-}
-
// renderOptions returns a RenderOptions for p.
func (p *Package) renderOptions(innerPath string, sourceInfo *source.Info, modInfo *ModuleInfo,
nameToVersion map[string]string, bc internal.BuildContext) dochtml.RenderOptions {
