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

Jump to bottom

x/tools/gopls: handle formatting on files that do not parse #31291

Closed
stamblerre opened this issue Apr 5, 2019 · 7 comments
Closed

x/tools/gopls: handle formatting on files that do not parse #31291

stamblerre opened this issue Apr 5, 2019 · 7 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone
Unreleased

Comments

@stamblerre
Copy link
Contributor

If we don't check a package's parse errors before formatting, we may encounter a case where we delete code on format.

Repro:

package foo

func _() {
	f(),
	f()
}

To handle this, we check the parse errors before formatting. However, we should confirm that these errors belong to the file in question and lie within the specified range before failing.
@stamblerre stamblerre added the gopls Issues related to the Go language server, gopls. label Apr 5, 2019
@gopherbot gopherbot added this to the Unreleased milestone Apr 5, 2019
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 10, 2019
@myitcv
Copy link
Member

myitcv commented Apr 17, 2019

@stamblerre @ianthehat - should/can we also use this issue to pick up the conversation from https://go-review.googlesource.com/c/tools/+/171019/2#message-f8bec621217d3d2caad7dda408cdc629c71aaed6:

We should have a discussion about overall approach. The source package needs to return errors, because it has no idea about the environment it is running in, the question is whether the lsp package should log or propagate those errors. We were propagating them because we had no wired up way of logging, but I fixed that recently, so now we could choose to log them instead. We need to decide what level of failure should result in an error vs an empty result (does formatting a non existent file count as an actual error?)

Or do you want to capture that discussion about error propagation elsewhere?

@myitcv myitcv mentioned this issue Apr 17, 2019
Open
@ianthehat
Copy link

I think that is a separate issue. In fact, is now issue #31526

@stamblerre stamblerre changed the title x/tools/internal/lsp: handle rangeFormatting on files that do not parse x/tools/internal/lsp: handle formatting on files that do not parse May 20, 2019
@stamblerre stamblerre mentioned this issue Jun 28, 2019
Closed
@stamblerre stamblerre changed the title x/tools/internal/lsp: handle formatting on files that do not parse x/tools/gopls: handle formatting on files that do not parse Jul 2, 2019
@stamblerre stamblerre added the Suggested Issues that may be good for new contributors looking for work to do. label Jul 10, 2019
@natefinch
Copy link
Contributor

This is a huge quality of life issue for me, and probably many others. Not being formatted makes it harder to fix errors so that the code can compile. This literally makes it harder and slower for me to code in Go.

@stamblerre
Copy link
Contributor Author

As a temporary workaround, we could shell out to the gofmt binary when a file has parse errors.

@gopherbot
Copy link

Change https://golang.org/cl/187757 mentions this issue: internal/lsp: format files in packages with errors

gopherbot pushed a commit to golang/tools that referenced this issue Jul 26, 2019
@suzmue
internal/lsp: format files in packages with errors
1bd5602 
Packages with errors may still contain files that can be formatted.
Try to format the source of the files in packages that have errors.
This change will still not format files with parse errors.

Updates golang/go#31291

Change-Id: Ia5168d7908948d201eac7f2ee28534022a2d4eb0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/187757
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/188767 mentions this issue: internal/lsp: format files that parse in packages with parse errors

gopherbot pushed a commit to golang/tools that referenced this issue Aug 5, 2019
@stamblerre
internal/lsp: format files that parse in packages with parse errors
2756c52 
Updates golang/go#31291

Change-Id: Ibbd0b6cef9b9ec588c8a2e0c5e7ee6e3512b8a22
Reviewed-on: https://go-review.googlesource.com/c/tools/+/188767
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre mentioned this issue Aug 5, 2019
Open
@stamblerre
Copy link
Contributor Author

This issue should really be fixed in the go/format.Node function, so I will close this issue in favor of #33479.

@golang golang locked and limited conversation to collaborators Aug 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@ianthehat @natefinch @myitcv @bcmills @stamblerre @gopherbot