-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
go/token: add a (*File).Lines method #57708
Comments
Change https://go.dev/cl/461215 mentions this issue: |
This proposal has been added to the active column of the proposals project |
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>
Does anyone object to accepting this proposal? |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
Change https://go.dev/cl/464515 mentions this issue: |
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>
The
token.File
type in packagego/token
has aSetLines
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:
(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
The text was updated successfully, but these errors were encountered: