x/tools/gopls: space in package path leads to invalid hover markdown

H0llyW00dzZ commented 2 weeks ago

gopls version

$ gopls -v version
Build info
----------
golang.org/x/tools/gopls v0.16.0-pre.1
    golang.org/x/tools/gopls@v0.16.0-pre.1 h1:EAWgsepx4FJqIUuDtZ+wSeHUQ/oIqYwvNag1DjE50b4=
    github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
    github.com/google/go-cmp@v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
    golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
    golang.org/x/mod@v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
    golang.org/x/sync@v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
    golang.org/x/sys@v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
    golang.org/x/telemetry@v0.0.0-20240607193123-221703e18637 h1:3Wt8mZlbFwG8llny+t18kh7AXxyWePFycXMuVdHxnyM=
    golang.org/x/text@v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
    golang.org/x/tools@v0.22.1-0.20240611204047-7aca9095a0c1 h1:2RQpNDoYkPUoN8eTlSyKYJ6pchoM6n6fwsYevP+k4eY=
    golang.org/x/vuln@v1.0.4 h1:SP0mPeg2PmGCu03V+61EcQiOjmpri2XijexKdzv8Z1I=
    honnef.co/go/tools@v0.4.7 h1:9MDAWxMoSnB6QoSqiVr7P5mtkT9pOc1kSxchzPCnqJs=
    mvdan.cc/gofumpt@v0.6.0 h1:G3QvahNDmpD+Aek/bNOLrFR2XC6ZAdo62dZu65gmwGo=
    mvdan.cc/xurls/v2@v2.5.0 h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8=
go: go1.22.4

go env

$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\user\AppData\Local\go-build
set GOENV=C:\Users\user\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\user\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\user\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.22.4
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=C:\h0llyw00dz\unnammed-project\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\user\AppData\Local\Temp\go-build2907263936=/tmp/go-build -gno-record-gcc-switches

What did you do?

Reference documentation is broken

What did you see happen?

Today

What did you expect to see?

The link to the reference documentation is broken. It should be:

builtin.IntegerType on pkg.go.dev

Editor and settings

No response

Logs

No response

gabyhelp commented 2 weeks ago

Similar Issues

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

mauri870 commented 2 weeks ago

I did not test this on windows but I was unable to reproduce it on Linux. Perhaps the space in the url is causing the markdown to not be rendered. If so seems to be a vscode-go issue.

Either way, these pkg.go.dev urls mixed with the local Go toolchain path will result in a 404.

Edit: I was wrong, gopls formats the markdown links https://github.com/golang/tools/blob/019da39/gopls/internal/golang/hover.go#L1156. We should probably escape urls here.

cc @hyangah

H0llyW00dzZ commented 2 weeks ago

I did not test this on windows but I was unable to reproduce it on Linux. Perhaps the space in the url is causing the markdown to not be rendered. If so seems to be a vscode-go issue.

Either way, these pkg.go.dev urls mixed with the local Go toolchain path will result in a 404.

Edit: I was wrong, gopls formats the markdown links https://github.com/golang/tools/blob/019da39/gopls/internal/golang/hover.go#L1156. We should probably escape urls here.

cc @hyangah

Yes, the other one is correct.

H0llyW00dzZ commented 2 weeks ago

this broken

findleyr commented 1 week ago

Thanks for trying the prerelease, and for the report. We will investigate.

Assigning to @adonovan, who has been working in this area.

adonovan commented 1 week ago

As @mauri870 points out, the problem is indeed that if the package path has a space in it, the markdown is invalid. It's trivial to reproduce this by creating a package in a directory with a pathological name such as foo bar. The bug report shows a more realistic case where the pathological path segment is the operating system's own Program Files directory, which leaks into package paths when gopls synthesizes command-line-arguments packages.

Still thinking about a fix.

adonovan commented 1 week ago

One possibility is to emit angle brackets around the link. Compare these two, while looking at the markdown source:

[angle brackets](<https://a b/>)
[no angle brackets](https://a b/)

But that requires adjusting every place in gopls that puts package paths in markdown links, which is potentially a lot; and it's an unfamiliar syntax. It also seems like a hard invariant to maintain.

Another approach is to URL-encode the URLs, since spaces aren't really safe. But it also seems like a hard invariant to maintain since we're so used to not thinking about URL encoding in markdown.

A third approach is to choose paths that don't contain spaces when synthesizing packages. But that only solves the Program Files problem; genuine (pathological) package paths such as my foo bar example may contain spaces (or other arbitrary syntax such as close paren).

Option 2 seems best to me.

findleyr commented 1 week ago

Thanks. This isn't a regression then, moving out of the v0.16.0 milestone (that should have been obvious to me -- sorry for missing it in my haste).

findleyr commented 1 week ago

So, to summarize, there are two bugs here:

gopls is not properly url-escaping package paths.
gopls is not providing accurate hover information for builtins, when viewing the builtin file.

(1) should be fixed by a relatively straighforward application of url escaping. Assigning to our new team member @h9jiang as this is a good starter exercise for bug fixing and writing a test.

On the other hand, (2) might require a more sweeping refactoring of how we handle the builtin file and unsafe packages, and is just about the polar opposite of a good starter bug: requires familiarity with nitty-gritty details of the codebase. I will do this part.

gopherbot commented 5 days ago

Change https://go.dev/cl/594795 mentions this issue: gopls/internal/golang: fix hovering from the builtin file

golang / go