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

Jump to bottom

x/tools/gopls: define which LSP features return errors #35694

Closed
myitcv opened this issue Nov 19, 2019 · 2 comments
Closed

x/tools/gopls: define which LSP features return errors #35694

myitcv opened this issue Nov 19, 2019 · 2 comments
Labels
Documentation FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@myitcv
Copy link
Member

myitcv commented Nov 19, 2019

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

$ go version
go version devel +0ac8739ad5 Mon Nov 18 15:11:03 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20191118222007-07fc4c7f2b98
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20191118222007-07fc4c7f2b98

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="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build491500556=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Previously we have discussed what Formatting/CodeAction should do in the presence of parse errors.

Currently, Formatting/CodeAction return an error if the code in question does not parse. However, clients have no way of distinguishing a parse error from a "real" error (i.e. anything else that would point to a more significant problem)

Therefore clients like govim end up simply ignoring return errors, reason being:

  • the user is already aware of parse errors via diagnostics
  • having an error thrown on format-on-save will prevent the user from ever saving the file until the parse error is fixed (which the user might not want to do just yet)

This approach of ignoring all errors has the unfortunate side effect of "real" errors getting missed. In govim we have had ~2/3 instances of non-parse errors getting missed in this way and real bugs slipping through the net.

We have at least two options as previously discussed:

  1. do not return parse errors for either Formatting/CodeAction and instead rely on diagnostics which already tell the user about this
  2. return structured errors so that the user can distinguish and ignore parse errors

Thoughts?

cc @stamblerre
@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Nov 19, 2019
@gopherbot gopherbot added this to the Unreleased milestone Nov 19, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 19, 2019
@myitcv myitcv mentioned this issue Nov 25, 2019
Open
@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 4, 2019
@myitcv myitcv mentioned this issue Dec 6, 2019
Closed
@stamblerre stamblerre changed the title x/tools/gopls: define errors returned by Formatting/CodeAction x/tools/gopls: define which LSP features return errors Jan 10, 2020
@stamblerre
Copy link
Contributor

I'm going to co-opt this issue to continue the general discussion about which LSP requests should return errors, and which should just fail silently. The guiding principle that I'm following right now is: An action triggered by the user should always get a complete response or an error, whereas an action triggered by the editor should get a partial response. As an example:

  • textDocument/documentLink returns a partial response, because the editor triggers it when file contents change.
  • textDocument/references returns a complete response or an error, because it's triggered by the user.

I'd like to complete this list for all of the different features, and formatting and code actions are corner cases, since they can be triggered by the editor automatically or by the user directly.

@myitcv myitcv mentioned this issue Jan 16, 2020
Closed
@stamblerre stamblerre modified the milestones: gopls unplanned, gopls/v1.0.0 Jan 29, 2020
@stamblerre stamblerre added Documentation and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 12, 2020
@stamblerre stamblerre removed this from the gopls/v1.0.0 milestone Jun 24, 2020
@stamblerre
Copy link
Contributor

I think this is mostly resolved. We don't return errors for requests that the client will send frequently (on file change), but we do for user-triggered requests.

@golang golang locked and limited conversation to collaborators Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge 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
Milestone
No milestone
Development

No branches or pull requests
3 participants
@myitcv @stamblerre @gopherbot