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.

Sign up for GitHub

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: remove log.Fatal during analysis import #60963

Closed
findleyr opened this issue Jun 23, 2023 · 3 comments
Closed

x/tools/gopls: remove log.Fatal during analysis import #60963

findleyr opened this issue Jun 23, 2023 · 3 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.12.4

Comments

@findleyr
Copy link
Contributor

gopls@v0.12.3 introduced a log.Fatal during analysis, when importing fails. We suspect this is the cause of a number of crash reports with no stack (because VS Code does not extract log.Fatal messages in the way it extracts panic stacks).

While we think we've fixed the root cause of this fatal error, but in case we have not we should remove this log.Fatal before cutting a patch release, either choosing to panic or simply file a bug report and keep going. I lean toward the latter, as I don't feel confident that we have eliminated all sources of import failure.

CC @hyangah @adonovan
@findleyr findleyr added this to the gopls/v0.12.4 milestone Jun 23, 2023
@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 Jun 23, 2023
@gopherbot
Copy link

Change https://go.dev/cl/505575 mentions this issue: gopls/internal/lsp/cache: don't panic when import fails during analysis

@findleyr findleyr mentioned this issue Jun 23, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/505576 mentions this issue: gopls/internal/lsp/cache: don't panic when import fails during analysis

@gopherbot
Copy link

Change https://go.dev/cl/505577 mentions this issue: gopls/internal/lsp/cache: panic, not fatal, for inconsistent packages

gopherbot pushed a commit to golang/tools that referenced this issue Jun 23, 2023
@findleyr
gopls/internal/lsp/cache: don't panic when import fails during analysis
1257328 
We have received a number of empty crash reports for gopls since cutting
the v0.12.3 release. We don't want to auto-populate crash reports with
log.Fatal messages (as unlike panicking stacks they may contain the name
of user packages), so in almost all cases we lose information as to why
gopls crashed (users are unlikely to pre-populate this information).

In general, I don't think failing analysis should crash the process, so
switch this failure mode to a bug report.

Fixes golang/go#60963

Change-Id: I670ee4b1adbe9f37d763c5684b14d4bcb78dcb63
Reviewed-on: https://go-review.googlesource.com/c/tools/+/505575
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fa10359)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/505576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.12.4
Development

No branches or pull requests
2 participants
@gopherbot @findleyr