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

go/token: add a (*File).Lines method #57708

Closed
adonovan opened this issue Jan 9, 2023 · 6 comments
Closed

go/token: add a (*File).Lines method #57708

adonovan opened this issue Jan 9, 2023 · 6 comments
Assignees
@griesemer
Labels
Proposal Proposal-Accepted
Milestone
Go1.21

Comments

@adonovan
Copy link
Member

adonovan commented Jan 9, 2023

The token.File type in package go/token has a SetLines method that allows a caller to specify the file content that defines the mapping from offsets to line/column pairs. However, it doesn't have a getter method to retrieve the same information. It should, because a package that wants to export and import position information with complete fidelity (preserving not just line but column and offset too) needs this information, and it is much too expensive to get it by making a sequence of calls for each item. See https://go.dev/cl/461215 for an example.

A GetLines method would not expose any capability, risk, or liability that isn't already exposed by SetLines. Already today, a client can reconstruct all the information in the SetLines slice by a (slow) sequence of calls. A future implementation of File that supports SetLines and GetLines could still choose to encode the information internally in some other representation.

I therefore propose this addition:

package token // go/token

// Lines returns the effective line offset table as described by SetLines.
// It is not necessarily the same array. Callers should not mutate the result.
func (*File) Lines() []int { ... }

(Aside: I don't really understand why File.lines is guarded by a mutex. Has anyone ever written a program that calls SetLines concurrently with any other operation?)

cc @findleyr @griesemer @mdempsky
@gopherbot gopherbot added this to the Proposal milestone Jan 9, 2023
@gopherbot
Copy link

Change https://go.dev/cl/461215 mentions this issue: internal/gcimporter: preserve column and line in shallow iexport

@rsc
Copy link
Contributor

rsc commented Jan 11, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

gopherbot pushed a commit to golang/tools that referenced this issue Jan 17, 2023
@adonovan
internal/gcimporter: preserve column and line in shallow iexport
8aba49b 
Historically, export data has discarded source position information.
Originally not even line numbers were available; at some point
line and column numbers were stored, up to some fixed limit, but
more recently column numbers were again discarded.
Offset information has always been incorrect.

gopls is moving toward greater reliance on incremental operation
using a file-based cache of export data and other serializable
records that are similar in character. It is critical that it
be able to accurately recover file position information for
dependencies.

This change causes the iexport function to encode each object's
token.Pos as a pair (file, offset), where file indicates the
token.File and offset is a byte offset within the file.
The token.File is serialized as a filename and a delta-encoded
line-offset table. (We discard the lineInfos table that supports
//line directives because gopls no longer uses it.)
The iimport function constructs a token.File and calls SetLines,
and then all token.Pos values work exactly as they would with
source.

This causes about a 74% increase in size of the shallow export
data for the standard library: was 564KB, now 982KB.

token.File has a SetLines method but no GetLines. This change
must therefore resort to... unorthodox methods to retrieve the
field. Suggestions welcome.

This alternative encoding is enabled by using "shallow" mode,
which is effectively a license for gopls to do whatever it wants.
Again, suggestions welcome.

Updates golang/go#57708

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

rsc commented Jan 18, 2023

Does anyone object to accepting this proposal?

@rsc
Copy link
Contributor

rsc commented Jan 25, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: add a (*go/token.File).Lines method add a (*go/token.File).Lines method Feb 1, 2023
@rsc rsc modified the milestones: Proposal, Backlog Feb 1, 2023
@gopherbot
Copy link

Change https://go.dev/cl/464515 mentions this issue: go/token: add (*File).Lines method

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@adonovan @johanbrandhorst
go/token: add (*File).Lines method
1654166 
This method returns the array updated by SetLines, for
use in exporter packages.

Fixes golang#57708

Change-Id: I12ed5e7e1bae7517f40cb25e76e51997c25d84f4
Reviewed-on: https://go-review.googlesource.com/c/go/+/464515
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
@mknyszek mknyszek mentioned this issue Jun 1, 2023
Closed
@dmitshur dmitshur changed the title add a (*go/token.File).Lines method go/token: add a (*File).Lines method Jun 4, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
Proposal Proposal-Accepted
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
5 participants
@rsc @dmitshur @adonovan @griesemer @gopherbot