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: fix handling of end of file in internal/span #41029

Closed
dandua98 opened this issue Aug 25, 2020 · 5 comments
Closed

x/tools/gopls: fix handling of end of file in internal/span #41029

dandua98 opened this issue Aug 25, 2020 · 5 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
gopls/backlog

Comments

@dandua98
Copy link

Package span currently treats end of file as start of a new line (https://github.com/golang/tools/blob/c024452afbcdebb4a0fbe1bb0eaea0d2dbff835b/internal/span/token.go#L117), which is inaccurate for files with end of file not being a newline. This affects completion on end of file line and we're currently considering a temporary fix there (https://go-review.googlesource.com/c/tools/+/249706). A better fix should come from span itself and the package should check if a final new line is present before converting end of file to start of new line. Just removing this check breaks diff tests and is not a viable fix currently.
@dandua98 dandua98 added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Aug 25, 2020
@gopherbot gopherbot added this to the Unreleased milestone Aug 25, 2020
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v.0.5.0 Aug 25, 2020
@stamblerre
Copy link
Contributor

I think that, rather than changing the span package, we should do our best to work-around this in the cases where it comes up (so far, completion, and perhaps other features that insert edits like code actions). The work-around added in CL 249706 is not ideal because it is untested, so once we have support for completion in package declarations, we should see if we can improve and test it.

/cc @findleyr since we discussed this, and it's relevant to #37702.

@stamblerre stamblerre modified the milestones: gopls/v0.5.0, gopls/v1.0.0 Aug 27, 2020
@stamblerre stamblerre added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 27, 2020
@stamblerre
Copy link
Contributor

Given that we have a reasonable work-around for this in completion, removing from 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
@mjlbach mjlbach mentioned this issue Jan 19, 2021
Closed
@findleyr
Copy link
Contributor

I don't understand how this isn't just a bug. Presumably the limitation is driven by the behavior of token.File, which IIRC ignores a trailing newline.

@gopherbot
Copy link

Change https://go.dev/cl/459736 mentions this issue: gopls/internal/lsp/safetoken: funnel more calls through this package

gopherbot pushed a commit to golang/tools that referenced this issue Dec 29, 2022
@adonovan @gopherbot
gopls/internal/lsp/safetoken: funnel more calls through this package
81e741e 
This change expands the dominion of safetoken to include calls to
token.File{,Set}.Position{,For}, since all need workarounds similar
to that in Offset. As a side benefit, we now have a centralized place
to implement the workaround for other bugs such as golang/go#41029,
the newline at EOF problem).

Unfortunately the former callers of FileSet.Position must stipulate
whether the Pos is a start or an end, as the same value may denote
the position 1 beyond the end of one file, or the start of the
following file in the file set. Hence the two different functions,
{Start,End}Position.

The static check has been expanded, as have the tests.

Of course, this approach is not foolproof: gopls has many dependencies
that call methods of File and FileSet directly.

Updates golang/go#41029
Updates golang/go#57484
Updates golang/go#57490

Change-Id: Ia727e3f55ca2703dd17ff2cac05e786793ca38eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459736
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
@findleyr
Copy link
Contributor

I believe this is a non-issue now that we've switched to the new protocol.Mapper.

@findleyr findleyr modified the milestones: gopls/unplanned, gopls/later Aug 16, 2023
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. 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
None yet
Milestone
gopls/backlog
Development

No branches or pull requests
4 participants
@stamblerre @gopherbot @dandua98 @findleyr