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

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 5, 2019

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 label Apr 5, 2019
@gopherbot gopherbot added this to the Unreleased milestone Apr 5, 2019
@bcmills bcmills added the NeedsFix label Apr 10, 2019
@myitcv

This comment has been minimized.

Copy link
Member

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

@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

@natefinch natefinch commented Jul 24, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Jul 24, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 26, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Aug 5, 2019

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

@stamblerre stamblerre closed this Aug 5, 2019
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
6 participants
You can’t perform that action at this time.