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: improve diagnostics for empty go files #42154

Open
hyangah opened this issue Oct 22, 2020 · 12 comments
Open

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

hyangah opened this issue Oct 22, 2020 · 12 comments
Labels
gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Oct 22, 2020

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.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Oct 22, 2020
@gopherbot gopherbot added this to the Unreleased milestone Oct 22, 2020
@stamblerre stamblerre added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 23, 2020
@stamblerre
Copy link
Contributor

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
Copy link
Contributor Author

hyangah commented Oct 23, 2020

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
Copy link
Contributor

@heschik @findleyr what are your thoughts on this?

@heschi
Copy link
Contributor

heschi commented Oct 28, 2020

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
Copy link
Contributor

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
Copy link
Contributor

heschi commented Oct 30, 2020

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

@windowsrefund
Copy link

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

@findleyr
Copy link
Contributor

findleyr commented Jan 8, 2024

@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
Copy link

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
Copy link
Contributor

findleyr commented Jan 9, 2024

@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
Copy link
Contributor Author

hyangah commented Jan 9, 2024

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

@hyangah hyangah changed the title x/tools/gopls: skip diagnostics for an empty file if it's the only go file x/tools/gopls: improve diagnostics for empty go files Jan 9, 2024
@windowsrefund
Copy link

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

Please do not assume people are using that thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
No open projects
Development

No branches or pull requests

6 participants