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: hoverKind FullDocumentation is incorrectly escaping various characters #38656

Closed
jsravn opened this issue Apr 25, 2020 · 7 comments
Closed

Comments

@jsravn
Copy link

@jsravn jsravn commented Apr 25, 2020

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

❯ gopls version                          
golang.org/x/tools/gopls v0.3.4
    golang.org/x/tools/gopls@v0.3.4 h1:4GC7q/pXQ/tsxHBGVdsMdlB4gCxVC06m/7rIXg1Px4E=

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/james/.cache/go-build"
GOENV="/home/james/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/james/go:/home/james/godev"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build613753881=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I have set "hoverKind": "FullDocumentation" as added in #32561. This works, but it inserts a lot of escaped characters into the markdown description that it doesn't need to. I'm not quite sure why.

Here is an example for fmt#Errorf:

[`fmt.Errorf` on pkg.go.dev](https://pkg.go.dev/fmt#Errorf)

Errorf formats according to a format specifier and returns the string as a
value that satisfies error\\.

If the format specifier includes a \\%w verb with an error operand,
the returned error will implement an Unwrap method returning the operand\\. It is
invalid to include more than one \\%w verb or to supply it with an operand
that does not implement the error interface\\. The \\%w verb is otherwise
a synonym for \\%v\\.

Rendering this via markdown (inside Emacs via lsp-mode) produces this:

image

I'm also seeing html entities being escaped:

image

In the raw JSON this looks like \u0026nbsp;.

What did you expect to see?

Full documentation without any escape characters.

What did you see instead?

Full documentation with lots of embedded escape characters.

@gopherbot gopherbot added this to the Unreleased milestone Apr 25, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 25, 2020

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@gopherbot gopherbot added the gopls label Apr 25, 2020
@jsravn jsravn changed the title x/tools/gopls: hoverKind FullDocumentation is incorrectly escaping itself x/tools/gopls: hoverKind FullDocumentation is incorrectly escaping various characters Apr 25, 2020
@jsravn
Copy link
Author

@jsravn jsravn commented Apr 25, 2020

Actually I think the   is a bug on the lsp-mode/emacs side. But I'm not sure why gopls is returning e.g. \\. for punctuation. I'd expect \..

@jsravn
Copy link
Author

@jsravn jsravn commented Apr 25, 2020

^ Nevermind, I confirm gopls is indeed returning escaped &, that's why I see   in the render.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 25, 2020

This is probably because lsp-mode is requesting its hover results to be formatted in Markdown. There is an option on the language client's end to request plaintext results, but a lot of clients choose Markdown because users may have other plugins installed that display Markdown nicely. I would recommend opening up an issue to offer a configuration in lsp-mode or seeing if you can install a plugin to format the hover.

@jsravn
Copy link
Author

@jsravn jsravn commented Apr 25, 2020

I see this is expected based on the LSP spec. So I'll dig further on the lsp-mode side.

@jsravn jsravn closed this Apr 25, 2020
@jsravn
Copy link
Author

@jsravn jsravn commented Apr 25, 2020

lsp-mode change to handle this at emacs-lsp/lsp-mode#1614.

@stamblerre do you know why gopls returns so much whitespace around indents though?

For example here is the original godoc:

// Expect wraps an actual value allowing assertions to be made on it:
//    Expect("foo").To(Equal("foo"))

And the MarkupContent inserts 2 lines between the sentence and the following indent:

  "contents": {
    "value": "```go\nfunc ..Expect(actual interface{}, extra ...interface{}) ..Assertion\n```\n\n[`gomega.Expect` on pkg.go.dev](https://pkg.go.dev/github.com/onsi/gomega@v1.5.0#Expect)\n\nExpect wraps an actual value allowing assertions to be made on it\\:\n\n\n    Expect(\"foo\").To(Equal(\"foo\"))\n\n\nIf Expect is passed more than one argument it will pass the \\*first\\* argument to the matcher\\.\nAll subsequent arguments will be required to be nil\\/zero\\.\n\nThis is convenient if you want to make an assertion on a method\\/function that returns\na value and an error \\- a common patter in Go\\.\n\nFor example, given a function with signature\\:\n\n\n    func MyAmazingThing() (int, error)\n\n\nThen\\:\n\n\n    Expect(MyAmazingThing()).Should(Equal(3))\n\n\nWill succeed only if \\`MyAmazingThing\\(\\)\\` returns \\`\\(3, nil\\)\\`\n\nExpect and Ω are identical\n",
    "kind": "markdown"
  }
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 26, 2020

I believe that this is done to make the indent render properly in the Markdown. I'm not an expert, but this is the code that does the conversion: https://github.com/golang/tools/blob/f3a5411a4c3bfd7b6429abf664039dc43b37d985/internal/lsp/source/comment.go#L40.

I would imagine that the extra whitespace will not be there in the plaintext mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.