From 9bbfb19312a062d689d0dec4d805a609996def85 Mon Sep 17 00:00:00 2001 From: Sean Liao Date: Mon, 8 Dec 2025 22:37:25 +0000 Subject: 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 Reviewed-by: Ethan Lee kokoro-CI: kokoro Reviewed-by: Jonathan Amsterdam --- internal/godoc/dochtml/dochtml_test.go | 115 +++++++++++++++++---- .../godoc/dochtml/internal/render/render_test.go | 17 +-- internal/godoc/importer/importer.go | 57 ++++++++++ internal/godoc/importer/importer_test.go | 47 +++++++++ internal/godoc/render.go | 20 +--- 5 files changed, 206 insertions(+), 50 deletions(-) create mode 100644 internal/godoc/importer/importer.go create mode 100644 internal/godoc/importer/importer_test.go diff --git a/internal/godoc/dochtml/dochtml_test.go b/internal/godoc/dochtml/dochtml_test.go index 12f85365..f57cce32 100644 --- a/internal/godoc/dochtml/dochtml_test.go +++ b/internal/godoc/dochtml/dochtml_test.go @@ -26,6 +26,7 @@ import ( "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 @@ func compareWithGolden(t *testing.T, parts *Parts, name string, update bool) { 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 @@ func mustLoadPackage(path string) (*token.FileSet, *doc.Package) { } //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 @@ func mustLoadPackage(path string) (*token.FileSet, *doc.Package) { 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 483423db..fa775d6e 100644 --- a/internal/godoc/dochtml/internal/render/render_test.go +++ b/internal/godoc/dochtml/internal/render/render_test.go @@ -11,7 +11,8 @@ import ( "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 @@ import ( 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 @@ func mustLoadPackage(path string) (*doc.Package, *token.FileSet) { 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 00000000..97c7544b --- /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 00000000..04a7d23b --- /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 4bd8cdea..9d21fbb1 100644 --- a/internal/godoc/render.go +++ b/internal/godoc/render.go @@ -19,6 +19,7 @@ import ( "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 @@ func (p *Package) DocPackage(innerPath string, modInfo *ModuleInfo) (_ *doc.Pack // 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 @@ func (p *Package) DocPackage(innerPath string, modInfo *ModuleInfo) (_ *doc.Pack 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 { -- cgit v1.3