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: span.FromUTF16Column should handle positions beyond end of line #31883

Closed
myitcv opened this issue May 7, 2019 · 11 comments
Closed

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented May 7, 2019

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

$ go version
go version go1.12.5 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20190503185657-3b6f9c0030f7

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
GOARCH="amd64"
GOBIN="/home/myitcv/gostuff/src/github.com/myitcv/govim/cmd/govim/.bin"
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPROXY="https://proxy.golang.org/"
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
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-build053950369=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Edit the following file:

package main

var _ *

Load and observe the diagnostic that comes back:

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:8},
                End:   protocol.Position{Line:2, Character:8},
            },
            Severity:           1,
            Code:               nil,
            Source:             "LSP",
            Message:            "expected ';', found 'EOF'",
            Tags:               nil,
            RelatedInformation: nil,
        },
    },
}

Character 8 (0-based) is beyond the end of line 2 (0-based); 7 is that last possible index for line 2.

What did you expect to see?

An error within the bounds of line 2.

What did you see instead?

An error beyond the end of line 2.


cc @stamblerre @ianthehat

@gopherbot gopherbot added this to the Unreleased milestone May 7, 2019
myitcv added a commit to govim/govim that referenced this issue May 14, 2019
Because of golang/go#31883 (at least) it's
annoying having Vim error when it gets a diagnostic from gopls that it
can't safely handle. Therefore, follow a best-efforts approach to
populating the quickfix window; if we encounter an error, log it and
continue. Hope is now the strategy :) but at least the compiler/similar
will give the definitive answer.

Also use undojoin for squashing the formatting-fixes into the previous
change; prevents an undo block being created for, say, the addition of
imports when saving a file.
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 3, 2019

Perhaps related to this is that all diagnostics come back with Version 0; should this not correspond to the version of the file to which the error pertains?

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 5, 2019

The LSP specification doesn't contain a version (https://microsoft.github.io/language-server-protocol/specification#textDocument_publishDiagnostics), so I don't believe that would be related.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 5, 2019

Per discussion with @ianthehat on Slack.

Notice:

type PublishDiagnosticsParams struct {
	URI string `json:"uri"`
	Version float64 `json:"version,omitempty"`
	Diagnostics []Diagnostic `json:"diagnostics"`
}

Ian's thoughts on this were:

we would have to ask the lsp team, that file is totally automatically generated from their typescript, so it’s not an error, but given that they have not documented it in the human readable spec, we should probably not use it (it might be an experiment that is either still in progress or failed or something)

To my mind, a version is required because responses could be sent/handled out-of-order (much like textDocument/didChange has a version). But also for exactly the situation just described; if a diagnostic is delivered that references a point that is now beyond the end of the file (because the editor has already moved on) then the version field in the diagnostic corresponding to the client's version of the file would eliminate any such mismatch.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 29, 2019

@myitcv: I just tried reproducing this on master and was not able to. I got:

[Trace - 8:47:02 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/rstambler/bob/bob.go",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 2,
                    "character": 7
                },
                "end": {
                    "line": 2,
                    "character": 7
                }
            },
            "severity": 1,
            "source": "LSP",
            "message": "expected ';', found 'EOF'"
        }
    ]
}

Do you still see this error on master?

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 29, 2019

Yes, I'm still seeing this issue using fb37f6ba8261 of both gopls and tools.

Log file: https://gist.github.com/myitcv/89cafa97b2b24b852eb7532f29fb10a4

I started with a main.go of simply:

package main

And then as you can see from the logs made edits, such that the last diagnostic sent to the client is:

{
	"diagnostics": [
		{
			"message": "expected ';', found 'EOF'",
			"range": {
				"end": {
					"character": 8,
					"line": 2
				},
				"start": {
					"character": 8,
					"line": 2
				}
			},
			"severity": 1,
			"source": "LSP"
		}
	],
	"uri": "file:///home/myitcv/gostuff/src/github.com/myitcv/playground/main.go"
}
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jul 1, 2019

Just out of curiosity, can you try reproducing this with VSCode or another editor? I'm wondering if I am maybe trying to reproduce it incorrectly or if this is something specific to govim.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jul 1, 2019

Would it perhaps be easier/more precise to compare the traces? Can you provide a trace from VSCode?

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jul 1, 2019

Sure! It's interesting because I am seeing the 8 as a response from the code action (really from goimports), but not from the diagnostics.

[Trace - 1:46:26 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///usr/local/google/home/rstambler/bob/bob.go","diagnostics":[]}


[Trace - 1:46:27 PM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"uri":"file:///usr/local/google/home/rstambler/bob/bob.go","version":2},"contentChanges":[{"range":{"start":{"line":1,"character":0},"end":{"line":1,"character":0}},"rangeLength":0,"text":"\n"}]}


[Trace - 1:46:27 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///usr/local/google/home/rstambler/bob/bob.go","diagnostics":[]}


[Trace - 1:46:27 PM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"uri":"file:///usr/local/google/home/rstambler/bob/bob.go","version":3},"contentChanges":[{"range":{"start":{"line":2,"character":0},"end":{"line":2,"character":0}},"rangeLength":0,"text":"var _ *"}]}


[Trace - 1:46:27 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///usr/local/google/home/rstambler/bob/bob.go","diagnostics":[{"range":{"start":{"line":2,"character":7},"end":{"line":2,"character":7}},"severity":1,"source":"LSP","message":"expected ';', found 'EOF'"}]}


[Trace - 1:46:28 PM] Sending request 'textDocument/codeAction - (2)'.
Params: {"textDocument":{"uri":"file:///usr/local/google/home/rstambler/bob/bob.go"},"range":{"start":{"line":2,"character":7},"end":{"line":2,"character":7}},"context":{"diagnostics":[{"range":{"start":{"line":2,"character":7},"end":{"line":2,"character":7}},"message":"expected ';', found 'EOF'","severity":1,"source":"LSP"}]}}


[Error - 1:46:28 PM] send textDocument/codeAction#2 /usr/local/google/home/rstambler/bob/bob.go:3:8: expected type, found 'EOF'


[Error - 1:46:28 PM] Request textDocument/codeAction failed.
  Message: /usr/local/google/home/rstambler/bob/bob.go:3:8: expected type, found 'EOF'
  Code: 0 
@stamblerre stamblerre changed the title x/tools/cmd/gopls: diagnostic returns position beyond end of line x/tools/gopls: diagnostic returns position beyond end of line Jul 2, 2019
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jul 3, 2019

@stamblerre thanks very much for that trace. Much appreciated.

That's already helped to narrow this down to something in govim. If you notice, my trace is sending a \n at the end of the file.... which means the offset is correct.

I'm going to close this for now to reduce the noise 😄

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jul 4, 2019

Actually, I'm now back to my original position: what govim is sending here is "right", at least based on the following reasoning.

The file contents following the edits are, in quoted form:

"package main\n\nvar _ *\n"

Line 2 (in LSP terms) is:

"var _ *"

which is 7 bytes long (not including the newline, because they defines the end of the line)

Therefore the last valid position is 7 (in LSP terms).

The error is, however reported at position 8.

I actually think go/types might be at fault here. Consider the following:

package main

import (
	"fmt"
	"go/ast"
	"go/importer"
	"go/parser"
	"go/token"
	"go/types"
	"log"
)

const hello = `package main

var _ *
`

func main() {
	fset := token.NewFileSet()

	fmt.Printf(">> %q\n", hello)

	f, err := parser.ParseFile(fset, "main.go", hello, 0)
	if err != nil {
		log.Fatalf("failed to parse main.go: %v", err)
	}

	conf := types.Config{Importer: importer.Default()}

	_, err = conf.Check("cmd/hello", fset, []*ast.File{f}, nil)
	if err != nil {
		log.Fatalf("failed to type check main.go: %v", err)
	}

	fmt.Printf("Successfully type-checked")
}

This gives:

>> "package main\n\nvar _ *\n"
2019/07/04 08:58:55 failed to parse main.go: main.go:3:9: expected type, found 'EOF'

Column references here are 1-indexed, but here too the reference is beyond the end of the line.

That said, perhaps this is intentional. Because despite there being a newline at the end of the file, it would never make sense to have this error message be reported in terms of line 4 (1-indexed). From the user's perspective, line 4 does not exist).

So we conclude that go/types is not at fault 😄

That said, I don't think it would be correct for go/types to report the error as being main.go:3.8, because the following is valid:

package main

var _ *
string

func main() {}

Instead, I think we need to fallback to the LSP spec here:

interface Position {
	/**
	 * Line position in a document (zero-based).
	 */
	line: number;

	/**
	 * Character offset on a line in a document (zero-based). Assuming that the line is
	 * represented as a string, the `character` value represents the gap between the
	 * `character` and `character + 1`.
	 *
	 * If the character value is greater than the line length it defaults back to the
	 * line length.
	 */
	character: number;
}

Specifically, in this case, the character value is great than the line length, so we should drop back to the line length. Which would give us a valid position.

So, very long story short: I think span.FromUTF16Column needs to be updated to handle this case. I'll see if I can find time to make submit a CL later today, but please do just go ahead and create a CL if you have time 😄

@myitcv myitcv reopened this Jul 4, 2019
@myitcv myitcv changed the title x/tools/gopls: diagnostic returns position beyond end of line x/tools/gopls: span.FromUTF16Column should handle positions beyond end of line Jul 4, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 4, 2019

Change https://golang.org/cl/185058 mentions this issue: internal/span: handle character values beyond end of line in FromUTF16Column

movie-travel-code added a commit to movie-travel-code/tools that referenced this issue Jul 11, 2019
…6Column

On the assumption these implementations are designed to
support/implement the LSP spec, FromUTF16Column should handle the case
where a character value is beyond the end of the line.

https://github.com/Microsoft/language-server-protocol/blob/gh-pages/specification.md#position:

> * If the character value is greater than the line length it defaults back to the
> * line length.

Fixes golang/go#31883

Change-Id: I370845b7f2f046d8e84048a26bae5b23e9c27d06
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185058
Run-TryBot: Paul Jolly <paul@myitcv.org.uk>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
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.