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 line directives to .y files #34433

Open
renthraysk opened this issue Sep 20, 2019 · 6 comments
Open

x/tools/gopls: handle line directives to .y files #34433

renthraysk opened this issue Sep 20, 2019 · 6 comments

Comments

@renthraysk
Copy link

@renthraysk renthraysk commented Sep 20, 2019

What version of Go are you using (go version)?

go version go1.13 linux/amd64

And gopls...
golang.org/x/tools/gopls v0.1.7
golang.org/x/tools/gopls@v0.1.7 h1:YwKf8t9h69++qCtVmc2q6fVuetFXmmu9LKoPMYLZid4=

Does this issue reproduce with the latest release?

Yes

What did you do?

Open https://play.golang.org/p/-QfVqDKxsFI in VSCode.

What did you expect to see?

No crashing.

What did you see instead?

"The gopls server crashed 5 times in the last 3 minutes. The server will not be restarted."

@gopherbot gopherbot added this to the Unreleased milestone Sep 20, 2019
@stamblerre stamblerre changed the title x/tools/gopls: crashes with goyacc generated code x/tools/gopls: crashes with line directives Sep 20, 2019
@golang golang deleted a comment from gopherbot Sep 20, 2019
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 20, 2019

Thanks for reporting this. Looks like we aren't correctly handling code with line directives. This will require some work, but we can at least prevent the crash from occurring.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 20, 2019

Change https://golang.org/cl/196662 mentions this issue: internal/span: handle invalid column values to avoid crashing

gopherbot pushed a commit to golang/tools that referenced this issue Sep 23, 2019
This might not be necessary after we fix handling for line directives,
but it's always better to avoid the panic here.

Updates golang/go#34433

Change-Id: Ica4fb571dff6753fb15bf8d397c55f713284aa27
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196662
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
clintjedwards added a commit to clintjedwards/tools that referenced this issue Sep 25, 2019
This might not be necessary after we fix handling for line directives,
but it's always better to avoid the panic here.

Updates golang/go#34433

Change-Id: Ica4fb571dff6753fb15bf8d397c55f713284aa27
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196662
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@stamblerre stamblerre changed the title x/tools/gopls: crashes with line directives x/tools/gopls: handle line directives Sep 25, 2019
@heschi
Copy link
Contributor

@heschi heschi commented Nov 22, 2019

The work I'm doing for #35720 is a good start on this one, but we're not tracking .y files so I don't think it'll actually fix it. We'd have to be able to get a ColumnMapper for an arbitrary non-Go file.

@stamblerre stamblerre removed this from the Unreleased milestone Dec 4, 2019
@stamblerre stamblerre added this to the gopls v1.0 milestone Dec 4, 2019
@stamblerre stamblerre removed this from the gopls/v1.0.0 milestone Mar 12, 2020
@stamblerre stamblerre added this to the gopls/v0.6.0 milestone Mar 12, 2020
@stamblerre stamblerre removed this from the gopls/v0.6.0 milestone Apr 2, 2020
@stamblerre stamblerre added this to the gopls/v0.7.0 milestone Apr 2, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 9, 2020

Change https://golang.org/cl/227770 mentions this issue: internal/lsp: add an extra bounds check to avoid nil pointers

gopherbot pushed a commit to golang/tools that referenced this issue Apr 10, 2020
A nil pointer was reported to the golang-tools group (see
https://groups.google.com/g/golang-tools/c/JrNTz8I6ifo/m/tcJRpek-AAAJ).
I think this bounds check should address it.

Updates golang/go#34433

Change-Id: I87352c269c65c844c86ebe9ee3fd2d041cc49ee9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227770
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@stamblerre stamblerre removed this from the gopls/v0.7.0 milestone Jun 26, 2020
@stamblerre stamblerre added this to the gopls/v0.5.0 milestone Jun 26, 2020
@stamblerre stamblerre removed this from the gopls/v0.5.0 milestone Jul 22, 2020
@stamblerre stamblerre added this to the gopls/v1.0.0 milestone Jul 22, 2020
@pjweinb
Copy link

@pjweinb pjweinb commented Sep 6, 2020

This no longer crashes, instead writing an error message

Failed to compute document links: must supply Converter for file ".../foof.go" containing lines from ".../expr.y"

My personal opinion is that it will be almost impossible to get //line processing right, although maybe a few special cases could be done (after 1.0 is released).

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 9, 2020

I agree that handling line directives is not a critical priority for v1.0. Moving this out of the 1.0 milestone.

@stamblerre stamblerre removed this from the gopls/v1.0.0 milestone Sep 9, 2020
@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
@heschi heschi changed the title x/tools/gopls: handle line directives x/tools/gopls: handle line directives to .y files Nov 13, 2020
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
5 participants