Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
H0llyW00dzZ opened this issue Jun 16, 2024 · 10 comments
Open

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

H0llyW00dzZ opened this issue Jun 16, 2024 · 10 comments
Assignees
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@H0llyW00dzZ
Copy link

H0llyW00dzZ commented Jun 16, 2024

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

image

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

@H0llyW00dzZ H0llyW00dzZ added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Jun 16, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jun 16, 2024
@gabyhelp
Copy link

Similar Issues

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

@seankhliao seankhliao changed the title x/tools/gopls: Reference documentation is broken x/tools/gopls: broken rendering of links for stdlib Jun 16, 2024
@mauri870
Copy link
Member

mauri870 commented Jun 17, 2024

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
Copy link
Author

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.

image

@H0llyW00dzZ
Copy link
Author

this broken

image

@findleyr
Copy link
Contributor

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

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

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.16.0 Jun 17, 2024
@adonovan adonovan changed the title x/tools/gopls: broken rendering of links for stdlib x/tools/gopls: space in package name leads to invalid hover markdown Jun 17, 2024
@adonovan
Copy link
Member

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 adonovan changed the title x/tools/gopls: space in package name leads to invalid hover markdown x/tools/gopls: space in package path leads to invalid hover markdown Jun 17, 2024
@adonovan
Copy link
Member

adonovan commented Jun 17, 2024

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

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
Copy link
Contributor

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 findleyr modified the milestones: gopls/v0.16.0, gopls/v0.17.0 Jun 17, 2024
@findleyr
Copy link
Contributor

So, to summarize, there are two bugs here:

  1. gopls is not properly url-escaping package paths.
  2. 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
Copy link
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Jun 25, 2024
The builtin file represents a psuedo package for builtin documentation.
When hovering over a builtin declared in the builtin file, its
corresponding hover content should be the same as hovering at the call
site.

Fix this by intercepting hover requests originating from the builtin
file. Along the way, fix a bug that jumping to a definition in the
builtin file went to the declaring node, not declaring identifier.

For golang/go#68026

Change-Id: I070ec701cb7e067dd1cbb14a968c063169bb541c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/594795
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants