Skip to content

Commit

Permalink
feat(internal/godocfx): keep some cross links on same domain (#3767)
Browse files Browse the repository at this point in the history
The magic of this PR (that took me way too long to figure out) comes from adding `| packages.NeedDeps` to the `packages.Load` `Config`. That lets us get the `Fset` and `Syntax` of all packages the current package imports. We can then use that information to pull out the anchors for every element we link to.

This also fixes an issue where we were handling some packages that didn't have the Module field set -- we can ignore them. It appears to be a new package gets returned with the ID of the glob you asked for. For example, `cloud.google.com/go/...`. We don't seem to need that.

Fixes #3739.
  • Loading branch information
tbpg authored Mar 3, 2021
1 parent 2ee6bf4 commit 77f76ed
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 22 deletions.
30 changes: 30 additions & 0 deletions internal/godocfx/godocfx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,33 @@ func TestGoldens(t *testing.T) {
}
}
}

func TestHasPrefix(t *testing.T) {
tests := []struct {
s string
prefixes []string
want bool
}{
{
s: "abc",
prefixes: []string{"1", "a"},
want: true,
},
{
s: "abc",
prefixes: []string{"1"},
want: false,
},
{
s: "abc",
prefixes: []string{"1", "2"},
want: false,
},
}

for _, test := range tests {
if got := hasPrefix(test.s, test.prefixes); got != test.want {
t.Errorf("hasPrefix(%q, %q) got %v, want %v", test.s, test.prefixes, got, test.want)
}
}
}
79 changes: 58 additions & 21 deletions internal/godocfx/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,74 +315,93 @@ type linker struct {
// different files.
imports map[string]string

// idToAnchor is a map from an ID to the anchor URL for that ID.
idToAnchor map[string]string
// idToAnchor is a map from package path to a map from ID to the anchor for
// that ID.
idToAnchor map[string]map[string]string

// sameDomainModules is a map from package path to module for every imported
// package that should cross link on the same domain.
sameDomainModules map[string]*packages.Module
}

func newLinker(pi pkgInfo) *linker {
sameDomainPrefixes := []string{"cloud.google.com/go"}

imports := map[string]string{}
sameDomainModules := map[string]*packages.Module{}
idToAnchor := map[string]map[string]string{}

for path, pkg := range pi.pkg.Imports {
name := pkg.Name
if rename := pi.importRenames[path]; rename != "" {
name = rename
}
imports[name] = path

// TODO: Consider documenting internal packages so we don't have to link
// out.
if pkg.Module != nil && hasPrefix(pkg.PkgPath, sameDomainPrefixes) && !strings.Contains(pkg.PkgPath, "internal") {
sameDomainModules[path] = pkg.Module

docPkg, _ := doc.NewFromFiles(pkg.Fset, pkg.Syntax, path)
idToAnchor[path] = buildIDToAnchor(docPkg)
}
}

idToAnchor := buildIDToAnchor(pi)
idToAnchor[""] = buildIDToAnchor(pi.doc)

return &linker{imports: imports, idToAnchor: idToAnchor}
return &linker{imports: imports, idToAnchor: idToAnchor, sameDomainModules: sameDomainModules}
}

// nonWordRegex is based on
// https://github.com/googleapis/doc-templates/blob/70eba5908e7b9aef5525d0f1f24194ae750f267e/third_party/docfx/templates/devsite/common.js#L27-L30.
var nonWordRegex = regexp.MustCompile("\\W")

func buildIDToAnchor(pi pkgInfo) map[string]string {
func buildIDToAnchor(pkg *doc.Package) map[string]string {
idToAnchor := map[string]string{}
idToAnchor[pi.doc.ImportPath] = pi.doc.ImportPath
idToAnchor[pkg.ImportPath] = pkg.ImportPath

for _, c := range pi.doc.Consts {
for _, c := range pkg.Consts {
commaID := strings.Join(c.Names, ",")
uid := pi.doc.ImportPath + "." + commaID
uid := pkg.ImportPath + "." + commaID
for _, name := range c.Names {
idToAnchor[name] = uid
}
}
for _, v := range pi.doc.Vars {
for _, v := range pkg.Vars {
commaID := strings.Join(v.Names, ",")
uid := pi.doc.ImportPath + "." + commaID
uid := pkg.ImportPath + "." + commaID
for _, name := range v.Names {
idToAnchor[name] = uid
}
}
for _, f := range pi.doc.Funcs {
uid := pi.doc.ImportPath + "." + f.Name
for _, f := range pkg.Funcs {
uid := pkg.ImportPath + "." + f.Name
idToAnchor[f.Name] = uid
}
for _, t := range pi.doc.Types {
uid := pi.doc.ImportPath + "." + t.Name
for _, t := range pkg.Types {
uid := pkg.ImportPath + "." + t.Name
idToAnchor[t.Name] = uid
for _, c := range t.Consts {
commaID := strings.Join(c.Names, ",")
uid := pi.doc.ImportPath + "." + commaID
uid := pkg.ImportPath + "." + commaID
for _, name := range c.Names {
idToAnchor[name] = uid
}
}
for _, v := range t.Vars {
commaID := strings.Join(v.Names, ",")
uid := pi.doc.ImportPath + "." + commaID
uid := pkg.ImportPath + "." + commaID
for _, name := range v.Names {
idToAnchor[name] = uid
}
}
for _, f := range t.Funcs {
uid := pi.doc.ImportPath + "." + t.Name + "." + f.Name
uid := pkg.ImportPath + "." + t.Name + "." + f.Name
idToAnchor[f.Name] = uid
}
for _, m := range t.Methods {
uid := pi.doc.ImportPath + "." + t.Name + "." + m.Name
uid := pkg.ImportPath + "." + t.Name + "." + m.Name
idToAnchor[m.Name] = uid
}
}
Expand Down Expand Up @@ -436,11 +455,20 @@ func (l *linker) linkify(s string) string {
// pattern.
func (l *linker) toURL(pkg, name string) string {
if pkg == "" {
if anchor := l.idToAnchor[name]; anchor != "" {
if anchor := l.idToAnchor[""][name]; anchor != "" {
name = anchor
}
return fmt.Sprintf("#%s", name)
}
if mod, ok := l.sameDomainModules[pkg]; ok {
pkgRemainder := pkg[len(mod.Path)+1:] // +1 to skip slash.
// Note: we always link to latest. One day, we'll link to mod.Version.
baseURL := fmt.Sprintf("/go/docs/reference/%v/latest/%v", mod.Path, pkgRemainder)
if anchor := l.idToAnchor[pkg][name]; anchor != "" {
return fmt.Sprintf("%s#%s", baseURL, anchor)
}
return baseURL
}
baseURL := "https://pkg.go.dev"
if name == "" {
return fmt.Sprintf("%s/%s", baseURL, pkg)
Expand Down Expand Up @@ -558,7 +586,7 @@ type pkgInfo struct {

func loadPackages(glob, workingDir string) ([]pkgInfo, error) {
config := &packages.Config{
Mode: packages.NeedName | packages.NeedSyntax | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedModule | packages.NeedImports,
Mode: packages.NeedName | packages.NeedSyntax | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedModule | packages.NeedImports | packages.NeedDeps,
Tests: true,
Dir: workingDir,
}
Expand Down Expand Up @@ -594,7 +622,7 @@ func loadPackages(glob, workingDir string) ([]pkgInfo, error) {
}
if strings.Contains(id, "_test") {
id = id[0:strings.Index(id, "_test [")]
} else {
} else if pkg.Module != nil {
idToPkg[pkg.PkgPath] = pkg
pkgNames = append(pkgNames, pkg.PkgPath)
// The test package doesn't have Module set.
Expand Down Expand Up @@ -674,3 +702,12 @@ func loadPackages(glob, workingDir string) ([]pkgInfo, error) {

return result, nil
}

func hasPrefix(s string, prefixes []string) bool {
for _, prefix := range prefixes {
if strings.HasPrefix(s, prefix) {
return true
}
}
return false
}
3 changes: 2 additions & 1 deletion internal/godocfx/testdata/golden/index.yml
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,8 @@ items:
- go
syntax:
content: func (b *<a href="#cloud_google_com_go_storage_BucketHandle">BucketHandle</a>)
IAM() *<a href="https://pkg.go.dev/cloud.google.com/go/iam">iam</a>.<a href="https://pkg.go.dev/cloud.google.com/go/iam#Handle">Handle</a>
IAM() *<a href="/go/docs/reference/cloud.google.com/go/latest/iam">iam</a>.<a
href="/go/docs/reference/cloud.google.com/go/latest/iam#cloud_google_com_go_iam_Handle">Handle</a>
- uid: cloud.google.com/go/storage.BucketHandle.If
name: |
func (*BucketHandle) If
Expand Down

0 comments on commit 77f76ed

Please sign in to comment.