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: 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

Comments

@myitcv
Copy link
Member

@myitcv 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

@gopherbot gopherbot added this to the Unreleased milestone Oct 23, 2019
@gopherbot gopherbot added the Tools label Oct 23, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 24, 2019

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

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 25, 2019

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

This comment has been minimized.

Copy link
Member Author

@myitcv 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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Nov 2, 2019

@myitcv myitcv closed this Nov 2, 2019
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
4 participants
You can’t perform that action at this time.