proposal: cmd/vet: should warn about broken doc links

[mvdan]

What version of Go are you using (go version)?

$ go version
go version devel go1.21-ba91377454 Sat Jan 21 21:08:30 2023 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mvdan/.cache/go-build"
GOENV="/home/mvdan/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/mvdan/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/mvdan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/mvdan/tip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/mvdan/tip/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.21-ba91377454 Sat Jan 21 21:08:30 2023 +0000"
GCCGO="gccgo"
GOAMD64="v3"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3083007116=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version devel go1.21-ba91377454 Sat Jan 21 21:08:30 2023 +0000 linux/amd64
GOROOT/bin/go tool compile -V: compile version devel go1.21-ba91377454 Sat Jan 21 21:08:30 2023 +0000
uname -sr: Linux 6.1.7-arch1-1
LSB Version:	n/a
Distributor ID:	Arch
Description:	Arch Linux
Release:	rolling
Codename:	n/a
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.36.
gdb --version: GNU gdb (GDB) 12.1

What did you do?

Wrote some godoc to include a doc link referencing a missing name, as well as an example referencing a missing name.

$ git diff
diff --git a/src/archive/zip/example_test.go b/src/archive/zip/example_test.go
index 1eed3040cb..81551609f2 100644
--- a/src/archive/zip/example_test.go
+++ b/src/archive/zip/example_test.go
@@ -75,7 +75,7 @@ func ExampleReader() {
    // This is the source code repository for the Go programming language.
 }
 
-func ExampleWriter_RegisterCompressor() {
+func ExampleWriter_Missing() {
    // Override the default Deflate compressor with a higher compression level.
 
    // Create a buffer to write our archive to.
diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go
index 3e96d0ecc9..917604b826 100644
--- a/src/archive/zip/reader.go
+++ b/src/archive/zip/reader.go
@@ -89,7 +89,7 @@ func OpenReader(name string) (*ReadCloser, error) {
 // have the given size in bytes.
 //
 // If any file inside the archive uses a non-local name
-// (as defined by [filepath.IsLocal]) or a name containing backslashes
+// (as defined by [filepath.Missing]) or a name containing backslashes
 // and the GODEBUG environment variable contains `zipinsecurepath=0`,
 // NewReader returns the reader with an ErrInsecurePath error.
 // A future version of Go may introduce this behavior by default.

What did you expect to see?

go vet should warn about both.

What did you see instead?

go vet only warns about the unknown name in the example name, but not in the doc link:

$ go vet archive/zip
# archive/zip_test
src/archive/zip/example_test.go:78:1: ExampleWriter_Missing refers to unknown field or method: Writer.Missing

I write this bug because I'm improving godocs in a project of mine to use doc links. It's hard to tell if I'm making any mistakes when referencing packages or exported names, because vet isn't helping me at all. I will likely look at the rendered godoc after pushing to a branch, but it's going to be very tricky to verify every link renders and works properly by hand.

I think vet should warn about all forms of broken links, even [Name1] or [pkg.Name1]. I realise that we want to be careful with backwards compatibility; old godocs may contain text which happens to be a broken link. In those cases, I'd want to be warned as well, because I want to either fix the link, or change the godoc so that it's not a link - by using a block quote for example.

[timothy-king]

To make such a check feasible given the current state of vet, what it can check will probably need to be restricted to checking symbols from packages that are directly imported. [example.com/sys.File] probably should not be checked unless the current package imports example.com/sys. Another tool might be happy to talk to pkg.go.dev, but cmd/vet does not seem like it should be doing RPC queries each time it is run. A simple restriction here would just be to only check assumed package names and to not check full package names.

Another this to clarify is whether cmd/vet should consider it a bug to be version inconsistent. If there is a doc link that refers to code that is only available for a newer go version, is that a bug? pkg.IsNewerDef (defined only after say go version 1.40) seems like an okay thing to include in the documentation of a function that is still available in older go versions.

A similar question about the standard library versions. Is it any standard library version? Or the one being used to compile?

[timothy-king]

I am slightly leaning towards this needs to be a proposal.

[mvdan]

A simple restriction here would just be to only check assumed package names and to not check full package names.

That restriction would be fine with me. In my use case, I'd almost always be referencing packages that I already import in my project. Vet could still check packages which are in the module dependency graph, but not directly imported by the current package, but I'm not too fussed about that distinction.

I'd still like for the check to warn me about [typo]. Without slashes, one could assume that it must be either a standard library package or a local name, and if neither exists, that should be an error. New root-level standard library packages are rare enough that this seems fine to me.

Another this to clarify is whether cmd/vet should consider it a bug to be version inconsistent.

Again a valid question. I think "name exists in any version of the referenced package" is probably better, although I'm not sure if pkg.go.dev would actually render links properly if a name doesn't exist in the imported or latest versions of said module (e.g. because it's a v0 and the name was deprecated and removed, or because it's in a development version that isn't yet released).

Thinking about your two points (potentially having to look up extra modules/packages, and potentially having to be aware of many versions at once), what occurs to me is that perhaps it should be pkgsite the one to check against broken doc links. pkg.go.dev already contains practically all open source Go modules and their known versions, so it would be able to perform better checks.

One downside of having the check in pkg.go.dev and not in go vet is that I would have to push the godoc to my VCS and then load the version in pkg.go.dev to see the result of those checks. But at least if any broken links were warned about loudly (for example, as part of the module information in the header), I'd be able to quickly check the entire module for broken links without having to manually check every package with doc links.

cc @golang/pkgsite

I am slightly leaning towards this needs to be a proposal.

Perhaps you're right, though at this point I'm 50/50 on whether this should be a proposal for cmd/vet or x/pkgsite :)

[mvdan]

Perhaps another reason for pkgsite to warn about broken doc links is that it already does the work to render the links, so it might already be aware of which are broken.

[mvdan]

Yet another idea: if broken doc links were to somehow be included as part of a "quality report" produced by pkg.go.dev, it would be nice as a module author to fetch that from time to time and fix any errors. For example, goreleaser could tell me if I'm about to tag a release which contains broken doc links.

[bcmills]

If the package mentioned in the doc comment is provided by some module in the module graph, I would expect the doc link to in general refer to that version of the package. In that case, I think it would be reasonable for vet to flag the broken link.

On the other hand, if the package is not present at all in the build graph — maybe it is talking about a deprecated alternative, or a package that interferes destructively — then the potential for false-positives is much higher.

[adonovan]

Perhaps another reason for pkgsite to warn about broken doc links is that it already does the work to render the links, so it might already be aware of which are broken.

I think we would all like it to be easier to preview pkgsite's rendering of your locally edited packages, and it's something we're considering doing, perhaps integrated into gopls. If we did that, it could be made to visually flag broken links. That would make it easier for someone actively reviewing their doc comments to detect problems, though of course it still wouldn't enforce invariants like a vet check would.

[mvdan]

It seems like gopls is getting some awareness of godoc/comment links via #64648, so perhaps it might also be able to start flagging cases where it knows for sure that a link is wrong.

[adonovan]

See also #63929, which proposes a more general checker for go doc problems.