diff --git a/src/cmd/go/internal/doc/doc.go b/src/cmd/go/internal/doc/doc.go index 32f4674ef0..68306867a6 100644 --- a/src/cmd/go/internal/doc/doc.go +++ b/src/cmd/go/internal/doc/doc.go @@ -425,20 +425,11 @@ func parseArgs(ctx context.Context, flagSet *flag.FlagSet, args []string) (pkg * if err == nil { return pkg, arg, args[1], false } - for { - importPath, ok := findNextPackage(arg) - if !ok { - break - } - if version != "" { - if pkg, err = loadVersioned(ctx, loader, importPath, version); err == nil { - return pkg, arg, args[1], true - } - } else { - if pkg, err = loadPackage(ctx, loader, importPath); err == nil { - return pkg, arg, args[1], true - } - } + if p, findErr, ok := findPackage(arg, importPkg); ok { + pkg = p + return pkg, arg, args[1], true + } else if findErr != nil { + err = findErr } if version != "" { log.Fatal(err) @@ -501,18 +492,8 @@ func parseArgs(ctx context.Context, flagSet *flag.FlagSet, args []string) (pkg * // See if we have the basename or tail of a package, as in json for encoding/json // or ivy/value for robpike.io/ivy/value. pkgName := arg[:period] - for { - importPath, ok := findNextPackage(pkgName) - if !ok { - break - } - if version != "" { - if pkg, err = loadVersioned(ctx, loader, importPath, version); err == nil { - return pkg, arg[0:period], symbol, true - } - } else if pkg, err = loadPackage(ctx, loader, importPath); err == nil { - return pkg, arg[0:period], symbol, true - } + if pkg, _, ok := findPackage(pkgName, importPkg); ok { + return pkg, arg[0:period], symbol, true } dirs.Reset() // Next iteration of for loop must scan all the directories again. } @@ -556,6 +537,82 @@ func parseArgs(ctx context.Context, flagSet *flag.FlagSet, args []string) (pkg * return mustLoadPackage(ctx, loader, wd), "", arg, false } +// findPackage returns the first successfully imported package matching the query pkg. +// It updates dirs.offset to the candidate's nextOffset so that subsequent searches +// work across retry loops. +// +// (pkg, nil, true) => imported a package +// (nil, err, false) => all imports failed (along with last error) +// (nil, nil, false) => no matching packages +func findPackage(pkg string, importPkg func(string) (*load.Package, error)) (*load.Package, error, bool) { + var lastErr error + for _, m := range matchingPackages(pkg) { + p, err := importPkg(m.importPath) + if err == nil { + dirs.offset = m.nextOffset + return p, nil, true + } + lastErr = err + } + return nil, lastErr, false +} + +type packageMatch struct { + importPath string + nextOffset int +} + +func matchingPackages(pkg string) []packageMatch { + // TODO(adonovan): once go1.28 tree opens, refactor matchingPackages to use + // iter.Seq[string] to encapsulate iteration state and avoid global vars. + if filepath.IsAbs(pkg) { + if dirs.offset == 0 { + dirs.offset = -1 + return []packageMatch{{importPath: pkg, nextOffset: -1}} + } + return nil + } + if pkg == "" || token.IsExported(pkg) { // Upper case symbol cannot be a package name. + return nil + } + pkg = path.Clean(pkg) + pkgSuffix := "/" + pkg + deferInternal := !hasPathElement(pkg, "internal") + // Prefer non-internal packages unless pkg is itself internal. + // Scanning directories is cheap compared to loading packages, so + // collect all matches and sort internal matches to the end. + var matches []packageMatch + var nonInternal []packageMatch + var internal []packageMatch + for { + d, ok := dirs.Next() + if !ok { + break + } + if d.importPath != pkg && !strings.HasSuffix(d.importPath, pkgSuffix) { + continue + } + m := packageMatch{importPath: d.importPath, nextOffset: dirs.offset} + if !deferInternal { + matches = append(matches, m) + } else if hasPathElement(d.importPath, "internal") { + internal = append(internal, m) + } else { + nonInternal = append(nonInternal, m) + } + } + if !deferInternal { + return matches + } + if len(nonInternal) == 0 { + return internal + } + // If the last non-internal match is returned, later retries should + // not fall through to internal-only matches for the same package path. + nonInternal[len(nonInternal)-1].nextOffset = dirs.offset + return append(nonInternal, internal...) +} + func loadPackage(ctx context.Context, loader *modload.Loader, pattern string) (*load.Package, error) { if !search.NewMatch(pattern).IsLiteral() { return nil, fmt.Errorf("pattern %q does not specify a single package", pattern) @@ -639,30 +696,13 @@ func isExported(name string) bool { return unexported || token.IsExported(name) } -// findNextPackage returns the next import path that matches the -// (perhaps partial) package path pkg. The boolean reports if any match was found. -func findNextPackage(pkg string) (string, bool) { - if filepath.IsAbs(pkg) { - if dirs.offset == 0 { - dirs.offset = -1 - return pkg, true - } - return "", false - } - if pkg == "" || token.IsExported(pkg) { // Upper case symbol cannot be a package name. - return "", false - } - pkg = path.Clean(pkg) - pkgSuffix := "/" + pkg - for { - d, ok := dirs.Next() - if !ok { - return "", false - } - if d.importPath == pkg || strings.HasSuffix(d.importPath, pkgSuffix) { - return d.importPath, true +func hasPathElement(p, elem string) bool { + for part := range strings.SplitSeq(path.Clean(p), "/") { + if part == elem { + return true } } + return false } // splitGopath splits $GOPATH into a list of roots. diff --git a/src/cmd/go/internal/doc/doc_test.go b/src/cmd/go/internal/doc/doc_test.go index cd931e33c0..2feac69080 100644 --- a/src/cmd/go/internal/doc/doc_test.go +++ b/src/cmd/go/internal/doc/doc_test.go @@ -1144,6 +1144,40 @@ func TestMultiplePackages(t *testing.T) { } } +func TestInternalPackagesArePreferredLast(t *testing.T) { + if testing.Short() { + t.Skip("scanning file system takes too long") + } + maybeSkip(t) + var b bytes.Buffer + var flagSet flag.FlagSet + err := do(t.Context(), &b, &flagSet, []string{"synctest.Wait"}) + if err != nil { + t.Fatalf("unexpected error from synctest.Wait: %v", err) + } + out := b.String() + if !strings.Contains(out, `package synctest // import "testing/synctest"`) { + t.Fatalf("synctest.Wait resolved to wrong package:\n%s", out) + } + if strings.Contains(out, `internal/synctest`) { + t.Fatalf("synctest.Wait resolved to internal package:\n%s", out) + } + + b.Reset() + flagSet = flag.FlagSet{} + err = do(t.Context(), &b, &flagSet, []string{"synctest.DoesNotExist"}) + if err == nil { + t.Fatal("expected error from synctest.DoesNotExist") + } + errStr := err.Error() + if !strings.Contains(errStr, "testing/synctest") { + t.Fatalf("error %q should contain testing/synctest", errStr) + } + if strings.Contains(errStr, "internal/synctest") { + t.Fatalf("error %q should not contain internal/synctest", errStr) + } +} + // Test the code to look up packages when given two args. First test case is // // go doc binary BigEndian diff --git a/src/cmd/go/testdata/script/doc_internal.txt b/src/cmd/go/testdata/script/doc_internal.txt new file mode 100644 index 0000000000..a35960662c --- /dev/null +++ b/src/cmd/go/testdata/script/doc_internal.txt @@ -0,0 +1,32 @@ +# go doc prefers non-internal packages for shorthand package paths. + +env GO111MODULE=off +cd example.com/z/docpick + +go doc docpick.Public +stdout 'package docpick // import "example.com/z/docpick"' +stdout 'func Public' +! stdout 'internal/docpick' + +! go doc docpick.DoesNotExist +stderr 'doc: no symbol DoesNotExist in package example.com/z/docpick' +! stderr 'internal/docpick' + +go doc onlyinternal.Only +stdout 'package onlyinternal // import "example.com/internal/onlyinternal"' +stdout 'func Only' + +-- example.com/internal/docpick/doc.go -- +package docpick + +func Internal() {} + +-- example.com/internal/onlyinternal/doc.go -- +package onlyinternal + +func Only() {} + +-- example.com/z/docpick/doc.go -- +package docpick + +func Public() {}