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: off-by-one in diagnostic results? #35116

Closed
myitcv opened this issue Oct 23, 2019 · 4 comments
Closed

x/tools/gopls: off-by-one in diagnostic results? #35116

myitcv opened this issue Oct 23, 2019 · 4 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.4.0

Comments

@myitcv
Copy link
Member

myitcv commented Oct 23, 2019

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

$ go version
go version devel +03bb3e9ad1 Wed Oct 16 06:29:51 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20191023143423-ff611c50cd12
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20191023143423-ff611c50cd12

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"
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-build585329765=/tmp/go-build -gno-record-gcc-switches"

What did you do?

These might be intentional changes, in which case these issues are just to clarify the changes.

The following file:

package main

var _ *

results in the following diagnostic:

PublishDiagnostics callback: &protocol.PublishDiagnosticsParams{
    URI:         "file:///home/myitcv/gostuff/src/github.com/myitcv/playground/main.go",
    Version:     0,
    Diagnostics: {
        {
            Range: protocol.Range{
                Start: protocol.Position{Line:3, Character:0},
                End:   protocol.Position{Line:3, Character:0},
            },
            Severity:           1,
            Code:               nil,
            Source:             "syntax",
            Message:            "expected ';', found 'EOF'",
            Tags:               nil,
            RelatedInformation: nil,
        },
    },
}

That's the LSP line reference, which human terms is line 4, position 1. But there is no line 4.

Also the following file:

package main

func Hello() string {}

results in the following diagnostic:

PublishDiagnostics callback: &protocol.PublishDiagnosticsParams{
    URI:         "file:///home/myitcv/gostuff/src/github.com/myitcv/playground/main.go",
    Version:     0,
    Diagnostics: {
        {
            Range: protocol.Range{
                Start: protocol.Position{Line:2, Character:20},
                End:   protocol.Position{Line:2, Character:22},
            },
            Severity:           1,
            Code:               nil,
            Source:             "compiler",
            Message:            "missing return",
            Tags:               nil,
            RelatedInformation: nil,
        },
    },
}

This diagnostic has a start position of the { of the function, which I don't think is right? Should this be a zero length diagnostic at the closing }?

What did you expect to see?

Per above

What did you see instead?

Per above

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 Oct 23, 2019
@gopherbot gopherbot added this to the Unreleased milestone Oct 23, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Oct 23, 2019
@stamblerre
Copy link
Contributor

This is likely caused by CL 202542, which probably needs some more review. I will take a look at this soon.

@stamblerre
Copy link
Contributor

The second concern will be addressed in the fix for #35139.

The issue with the var _ * diagnostic happens to be a different error. It seems that span.NewRange(fset, pos, pos).Span() and span.Parse(fset.Position(pos)).String() have different results in this case. I will write a test case to investigate this in the internal/span library.

@myitcv
Copy link
Member Author

myitcv commented Oct 31, 2019

With reference to the var _ * diagnostic, CL 202542 caused the position of the returned error to change the returned range from:

Range: protocol.Range{
    Start: protocol.Position{Line:2, Character:8},
    End:   protocol.Position{Line:2, Character:8},
},

to:

Range: protocol.Range{
    Start: protocol.Position{Line:3, Character:0},
    End:   protocol.Position{Line:3, Character:0},
},

FWIW the compiler reports:

./main.go:4:1: syntax error: unexpected EOF, expecting type

So it seems that previously gopls adjusted a position that is beyond the end of the file to the final position in the file. I think what it did before is correct.

Regarding the function example, I mentioned in CL 203287:

  • consistency with the compiler
  • having the error position be somewhere the error can be fixed
    is what sways it for me.

i.e. that the error diagnostic should (in this case) be a zero length diagnostic with a start position where the user can do something to fix it, i.e. just before the final }. Yes, they can fix this by placing a return statement elsewhere, but we should at least be starting them in a place where they can do something about it.

Regarding the following:

package main

x

CL 202542 caused the position of the returned error to change the returned range from:

Range: protocol.Range{
    Start: protocol.Position{Line:2, Character:0},
    End:   protocol.Position{Line:2, Character:0},
},

to:

Range: protocol.Range{
    Start: protocol.Position{Line:2, Character:0},
    End:   protocol.Position{Line:3, Character:0},
},

But I think the intent of that CL was to in effect "select" the x, which means the range should be:

Range: protocol.Range{
    Start: protocol.Position{Line:2, Character:0},
    End:   protocol.Position{Line:2, Character:1},
},

@ianthehat ianthehat modified the milestones: Unreleased, gopls v1.0 Oct 31, 2019
@myitcv
Copy link
Member Author

myitcv commented Nov 2, 2019

This was fixed in https://go-review.googlesource.com/c/tools/+/204561

@myitcv myitcv closed this as completed Nov 2, 2019
@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Jul 22, 2020
@golang golang locked and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.4.0
Development

No branches or pull requests
4 participants
@ianthehat @myitcv @stamblerre @gopherbot