x/tools/gopls: improve diagnostics for empty go files

[hyangah]

I propose not to generate diagnostics for the newly created, empty file.

Assume this user workflow:

• mkdir hello; cd hello
• run go mod init example.com/hello
• open the folder from an editor (e.g. vscode with gopls enabled by default)
• create a new file hello.go, using the editor's feature

The first thing the user will notice is a diagnostic error message.

[Trace - 17:35:12.131 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///Users/hakim/hello/hello.go","diagnostics":[{"range":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}},"severity":1,"source":"syntax","message":"expected ';', found 'EOF'"}]}

It's not a critical, major issue, but it's not nice nor useful to present an error message before the user starts typing anything.

[stamblerre]

On one hand, I agree that the error message is not useful for beginners. But on the other hand, it feels like it's the responsibility of gopls to show the errors that the compiler would report for a given file.

Maybe we could modify the error message to be more helpful?

[hyangah]

The message expected ';', found 'EOF' is definitely not very user-friendly - especially go is known for a language that doesn't require ;. But what will be the alternative error message? For existing empty files, marking this as an error is reasonable. For a newly open file (which gopls knows) though, shouldn't it delay its diagnostic notification before the file is edited?

[stamblerre]

@heschik @findleyr what are your thoughts on this?

[heschi]

I think we should be focusing on the underlying problem we're trying to solve, not the symptoms. If the concern is that creating a new package is not user-friendly, let's talk about what to do about it from the beginning. For example, should we fill a package clause in for new files? Prompt the user somehow?

[stamblerre]

For example, should we fill a package clause in for new files?

We've had a few requests for this behavior in VS Code Go (vim-go does this), but it's not clear to me that it's definitely the best approach. For experienced users, it can be annoying, especially if you want something different than the prefilled package declaration.

[heschi]

Shrug. Then we can try to get the error improved upstream for 1.16, I guess.

[windowsrefund]

Are there no work-a rounds or plans to address this?

[findleyr]

@windowsrefund this has not been a high priority issue, since it only affects newly created files. Is this more than just a minor annoyance?

I wonder if it would suffice to simply fix go/parser to improve its error message? By comparison, the compiler says

foo.go:1:1: expected 'package', found 'EOF'

That error looks better to me. What do you think?

[windowsrefund]

It's a new file so obviously there shouldn't be an error reported at all. I can't believe people have just bit whacking the escape key all these years (and still are). I've never seen a single piece of software behave like this against a newly created buffer.

[findleyr]

@windowsrefund we generally try to preserve an invariant that the build succeeds if and only if there are no compiler errors. In this case, an empty file is a compiler error, unlike some other languages. Gopls is built on top of compiler tools, so we'd have to explicitly filter out this error.

Maybe we should mask this error if it is a nuisance, as suggested by our teammate @hyangah when filing this issue, but it hasn't gotten done because (1) it's not entirely trivial, (2) the path forward is not entirely obvious: should we instead be focusing on the new file workflow in VS Code?, and (3) we have limited resources, and until this point we haven't heard from users that this is causing them real problems.

To reiterate, I'm not disagreeing with you, just explaining why the situation is as it is, and why this issue has not been prioritized.

I still think that as a starting point we should improve the error. That seems straightforward, and an unqualified improvement.

[hyangah]

@findleyr and I tested this with the latest release candidate. It looks like we can use this diagnostic to improve the UX, instead of skipping diagnostics completely.

1. improve the parser error message as suggested in x/tools/gopls: improve diagnostics for empty go files #42154 (comment) or rewrite the error message from gopls.
2. use this diagnostic to produce code actions for code generation or suggestion.

[windowsrefund]

should we instead be focusing on the new file workflow in VS Code?

Please do not assume people are using that thing.