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?

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.

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.

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

Yes, I'm still seeing this issue using fb37f6b 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"
}

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.

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

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

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 😄

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