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: JSON-RPC spec violation #39719

Closed
uhthomas opened this issue Jun 19, 2020 · 8 comments
Closed

x/tools/gopls: JSON-RPC spec violation #39719

uhthomas opened this issue Jun 19, 2020 · 8 comments
Labels
Milestone

Comments

@uhthomas
Copy link

@uhthomas uhthomas commented Jun 19, 2020

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

$ go version
go version go1.14.4 darwin/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="/Users/thomas/Library/Caches/go-build"
GOENV="/Users/thomas/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY="code.cfops.it"
GONOSUMDB="code.cfops.it"
GOOS="darwin"
GOPATH="/Users/thomas/go"
GOPRIVATE="code.cfops.it"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.4/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/thomas/cloudflare/segments/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/d5/hqd0d3f17h7gmntg94k251z80000gn/T/go-build278475873=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'm using gopls, along with sublimelsp. There was an issue where a single JSON-RPC error response will persist for the duration of the session. During this, it was discovered that gopls violates the JSON-RPC spec by sending a result in an error response.

I have a feeling this is actually a net/rpc/jsonrpc issue, as it's missing ,omitempty.

What did you expect to see?

The response to not contain a result in an error response.

What did you see instead?

{
  "jsonrpc": "2.0",
  "error": {
    "data": null,
    "code": 0,
    "message": "computing fix edits: /Users/me/main.go:9:21: expected ';', found Println (and 2 more errors)"
  },
  "id": 3,
  "result": null
}

gopls

$ gopls version
version master, built in $GOPATH mode
$ git rev-parse HEAD
6023b8da91ed4093207e7f49f7a48e0cde121f9b

Editor

Sublime Text Dev Channel, Build 4074

{
    "lsp_format_on_save": true,
    "lsp_code_actions_on_save": {
        "source.fixAll": true,
        "source.organizeImports": true
    },
    "show_symbol_action_links": true,
    "only_show_lsp_completions": true,
    "clients": {
        "gopls": {
            "enabled": true,
            "settings": {
                "gopls.usePlaceholders": true,
                "gopls.analyses": {
                  "fillreturns": true,
                  "nonewvars": true,
                  "undeclaredname": true,
                  "unusedparams": true
                },
                "gopls.completeUnimported": true,
                "gopls.staticcheck": true
            }
        }
    }
}

See the original issue for more information.

cc @rchl @rwols

@gopherbot gopherbot added the Tools label Jun 19, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 19, 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 Jun 19, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jun 19, 2020
@ianthehat
Copy link

@ianthehat ianthehat commented Jun 19, 2020

I am confused, because gopls does not use net/rpc/jsonrpc, it uses x/tools/internal/jsonrpc2 where the wire structs do have omitempty on them, and I cannot reproduce it with a simple test.
I am wondering if there is a way it has a non nil raw message that still ends up as a null and thus does not trigger the omit empty logic

@uhthomas
Copy link
Author

@uhthomas uhthomas commented Jun 19, 2020

Ah yes, I thought it might be using that internal tools package. I made a quick example of how to reproduce this behaviour.

@uhthomas
Copy link
Author

@uhthomas uhthomas commented Jun 19, 2020

I assume the fix is to check if result is nil in x/tools/internal/jsonrpc2.NewResponse. I can do this now if you like 😄

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 19, 2020

Change https://golang.org/cl/239097 mentions this issue: internal/jsonrpc2: don't set result in response if there is an error

@uhthomas
Copy link
Author

@uhthomas uhthomas commented Jun 19, 2020

Off-topic, but I wonder if it might be a good idea not to marshal the data field for errors? The spec says it's optional, currently null is always encoded.

@ianthehat
Copy link

@ianthehat ianthehat commented Jun 20, 2020

Probably, especially as we never fill it in.

@uhthomas
Copy link
Author

@uhthomas uhthomas commented Jun 21, 2020

Created a new issue #39736

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.