gopls/internal/span: some cleanups
This change eliminates these redundant helper functions
- cache.rangeFromPositions
- source.LineToRange
- source.ByteOffsetsToRange
and makes various other simplifications and documentation
improvements.
Change-Id: Ic820ab560d71b88bde00b005e3a919334a5d1856
Reviewed-on: https://go-review.googlesource.com/c/tools/+/442015
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 0b37135..a186a81 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -729,7 +729,7 @@
if reference == nil {
continue
}
- rng, err := rangeFromPositions(pm.Mapper, reference.Start, reference.End)
+ rng, err := pm.Mapper.OffsetRange(reference.Start.Byte, reference.End.Byte)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index 3ef5301..c33561f 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -392,8 +392,8 @@
}
case source.Mod:
if pmf, err := s.ParseMod(ctx, fh); err == nil {
- if pmf.File.Module != nil && pmf.File.Module.Syntax != nil {
- rng, _ = rangeFromPositions(pmf.Mapper, pmf.File.Module.Syntax.Start, pmf.File.Module.Syntax.End)
+ if mod := pmf.File.Module; mod != nil && mod.Syntax != nil {
+ rng, _ = pmf.Mapper.OffsetRange(mod.Syntax.Start.Byte, mod.Syntax.End.Byte)
}
}
}
diff --git a/gopls/internal/lsp/cache/mod.go b/gopls/internal/lsp/cache/mod.go
index f8404e7..97bdb2f 100644
--- a/gopls/internal/lsp/cache/mod.go
+++ b/gopls/internal/lsp/cache/mod.go
@@ -79,7 +79,7 @@
return nil, fmt.Errorf("unexpected parse error type %v", parseErr)
}
for _, mfErr := range mfErrList {
- rng, err := rangeFromPositions(m, mfErr.Pos, mfErr.Pos)
+ rng, err := m.OffsetRange(mfErr.Pos.Byte, mfErr.Pos.Byte)
if err != nil {
return nil, err
}
@@ -155,7 +155,7 @@
return nil, fmt.Errorf("unexpected parse error type %v", parseErr)
}
for _, mfErr := range mfErrList {
- rng, err := rangeFromPositions(m, mfErr.Pos, mfErr.Pos)
+ rng, err := m.OffsetRange(mfErr.Pos.Byte, mfErr.Pos.Byte)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/mod_tidy.go b/gopls/internal/lsp/cache/mod_tidy.go
index 63346dc..8e4e060 100644
--- a/gopls/internal/lsp/cache/mod_tidy.go
+++ b/gopls/internal/lsp/cache/mod_tidy.go
@@ -287,7 +287,7 @@
// unusedDiagnostic returns a source.Diagnostic for an unused require.
func unusedDiagnostic(m *protocol.ColumnMapper, req *modfile.Require, onlyDiagnostic bool) (*source.Diagnostic, error) {
- rng, err := rangeFromPositions(m, req.Syntax.Start, req.Syntax.End)
+ rng, err := m.OffsetRange(req.Syntax.Start.Byte, req.Syntax.End.Byte)
if err != nil {
return nil, err
}
@@ -313,7 +313,7 @@
// directnessDiagnostic extracts errors when a dependency is labeled indirect when
// it should be direct and vice versa.
func directnessDiagnostic(m *protocol.ColumnMapper, req *modfile.Require, computeEdits source.DiffFunction) (*source.Diagnostic, error) {
- rng, err := rangeFromPositions(m, req.Syntax.Start, req.Syntax.End)
+ rng, err := m.OffsetRange(req.Syntax.Start.Byte, req.Syntax.End.Byte)
if err != nil {
return nil, err
}
@@ -325,8 +325,8 @@
if comments := req.Syntax.Comment(); comments != nil && len(comments.Suffix) > 0 {
end := comments.Suffix[0].Start
end.LineRune += len(comments.Suffix[0].Token)
- end.Byte += len([]byte(comments.Suffix[0].Token))
- rng, err = rangeFromPositions(m, comments.Suffix[0].Start, end)
+ end.Byte += len(comments.Suffix[0].Token)
+ rng, err = m.OffsetRange(comments.Suffix[0].Start.Byte, end.Byte)
if err != nil {
return nil, err
}
@@ -359,7 +359,7 @@
if pm.File != nil && pm.File.Module != nil && pm.File.Module.Syntax != nil {
start, end := pm.File.Module.Syntax.Span()
var err error
- rng, err = rangeFromPositions(pm.Mapper, start, end)
+ rng, err = pm.Mapper.OffsetRange(start.Byte, end.Byte)
if err != nil {
return nil, err
}
@@ -429,11 +429,7 @@
if req.Syntax == nil {
return nil, fmt.Errorf("no syntax for %v", req)
}
- spn, err := span.NewRange(file, imp.Path.Pos(), imp.Path.End()).Span()
- if err != nil {
- return nil, err
- }
- rng, err := m.Range(spn)
+ rng, err := m.PosRange(imp.Path.Pos(), imp.Path.End())
if err != nil {
return nil, err
}
@@ -447,14 +443,6 @@
}, nil
}
-func rangeFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (protocol.Range, error) {
- spn, err := spanFromPositions(m, s, e)
- if err != nil {
- return protocol.Range{}, err
- }
- return m.Range(spn)
-}
-
func spanFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (span.Span, error) {
toPoint := func(offset int) (span.Point, error) {
l, c, err := span.ToPosition(m.TokFile, offset)
diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go
index f6f9f79..5a47626 100644
--- a/gopls/internal/lsp/diagnostics.go
+++ b/gopls/internal/lsp/diagnostics.go
@@ -545,11 +545,7 @@
if !pgf.File.Name.Pos().IsValid() {
return nil
}
- spn, err := span.NewRange(pgf.Tok, pgf.File.Name.Pos(), pgf.File.Name.End()).Span()
- if err != nil {
- return nil
- }
- rng, err := pgf.Mapper.Range(spn)
+ rng, err := pgf.Mapper.PosRange(pgf.File.Name.Pos(), pgf.File.Name.End())
if err != nil {
return nil
}
diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go
index 3969e88..d966d94 100644
--- a/gopls/internal/lsp/lsp_test.go
+++ b/gopls/internal/lsp/lsp_test.go
@@ -360,26 +360,18 @@
res := contents
// Apply the edits from the end of the file forward
// to preserve the offsets
+ // TODO(adonovan): factor to use diff.ApplyEdits, which validates the input.
for i := len(ranges) - 1; i >= 0; i-- {
- fRange := ranges[i]
- spn, err := m.RangeSpan(protocol.Range{
- Start: protocol.Position{
- Line: fRange.StartLine,
- Character: fRange.StartCharacter,
- },
- End: protocol.Position{
- Line: fRange.EndLine,
- Character: fRange.EndCharacter,
- },
- })
+ r := ranges[i]
+ start, err := m.Point(protocol.Position{r.StartLine, r.StartCharacter})
if err != nil {
return "", err
}
- start := spn.Start().Offset()
- end := spn.End().Offset()
-
- tmp := res[0:start] + foldedText
- res = tmp + res[end:]
+ end, err := m.Point(protocol.Position{r.EndLine, r.EndCharacter})
+ if err != nil {
+ return "", err
+ }
+ res = res[:start.Offset()] + foldedText + res[end.Offset():]
}
return res, nil
}
diff --git a/gopls/internal/lsp/mod/code_lens.go b/gopls/internal/lsp/mod/code_lens.go
index daafee0..01d75d9 100644
--- a/gopls/internal/lsp/mod/code_lens.go
+++ b/gopls/internal/lsp/mod/code_lens.go
@@ -134,7 +134,7 @@
return protocol.Range{}, fmt.Errorf("no module statement in %s", fh.URI())
}
syntax := pm.File.Module.Syntax
- return source.LineToRange(pm.Mapper, fh.URI(), syntax.Start, syntax.End)
+ return pm.Mapper.OffsetRange(syntax.Start.Byte, syntax.End.Byte)
}
// firstRequireRange returns the range for the first "require" in the given
@@ -155,7 +155,7 @@
if start.Byte == 0 || firstRequire.Start.Byte < start.Byte {
start, end = firstRequire.Start, firstRequire.End
}
- return source.LineToRange(pm.Mapper, fh.URI(), start, end)
+ return pm.Mapper.OffsetRange(start.Byte, end.Byte)
}
func vulncheckLenses(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle) ([]protocol.CodeLens, error) {
diff --git a/gopls/internal/lsp/mod/diagnostics.go b/gopls/internal/lsp/mod/diagnostics.go
index d794424..4b4f421 100644
--- a/gopls/internal/lsp/mod/diagnostics.go
+++ b/gopls/internal/lsp/mod/diagnostics.go
@@ -136,7 +136,7 @@
if !ok || req.Mod.Version == ver {
continue
}
- rng, err := source.LineToRange(pm.Mapper, fh.URI(), req.Syntax.Start, req.Syntax.End)
+ rng, err := pm.Mapper.OffsetRange(req.Syntax.Start.Byte, req.Syntax.End.Byte)
if err != nil {
return nil, err
}
@@ -205,7 +205,7 @@
if !ok {
continue
}
- rng, err := source.LineToRange(pm.Mapper, fh.URI(), req.Syntax.Start, req.Syntax.End)
+ rng, err := pm.Mapper.OffsetRange(req.Syntax.Start.Byte, req.Syntax.End.Byte)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/mod/hover.go b/gopls/internal/lsp/mod/hover.go
index 0f3dfc4..2981274 100644
--- a/gopls/internal/lsp/mod/hover.go
+++ b/gopls/internal/lsp/mod/hover.go
@@ -11,9 +11,9 @@
"strings"
"golang.org/x/mod/modfile"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
)
func Hover(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, position protocol.Position) (*protocol.Hover, error) {
@@ -78,7 +78,7 @@
}
// Get the range to highlight for the hover.
- rng, err := source.ByteOffsetsToRange(pm.Mapper, fh.URI(), startPos, endPos)
+ rng, err := pm.Mapper.OffsetRange(startPos, endPos)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/protocol/span.go b/gopls/internal/lsp/protocol/span.go
index bfa2f81..2d87c08 100644
--- a/gopls/internal/lsp/protocol/span.go
+++ b/gopls/internal/lsp/protocol/span.go
@@ -4,6 +4,34 @@
// this file contains protocol<->span converters
+// Here's a handy guide for your tour of the location zoo:
+//
+// Imports: source --> lsppos --> protocol --> span --> token
+//
+// source.MappedRange = (span.Range, protocol.ColumnMapper)
+//
+// lsppos.TokenMapper = (token.File, lsppos.Mapper)
+// lsppos.Mapper = (line offset table, content)
+//
+// protocol.ColumnMapper = (URI, token.File, content)
+// protocol.Location = (URI, protocol.Range)
+// protocol.Range = (start, end Position)
+// protocol.Position = (line, char uint32) 0-based UTF-16
+//
+// span.Point = (line?, col?, offset?) 1-based UTF-8
+// span.Span = (uri URI, start, end span.Point)
+// span.Range = (file token.File, start, end token.Pos)
+//
+// token.Pos
+// token.FileSet
+// offset int
+//
+// TODO(adonovan): simplify this picture. Eliminate the optionality of
+// span.{Span,Point}'s position and offset fields: work internally in
+// terms of offsets (like span.Range), and require a mapper to convert
+// them to protocol (UTF-16) line/col form. Stop honoring //line
+// directives.
+
package protocol
import (
@@ -12,6 +40,7 @@
"go/token"
"unicode/utf8"
+ "golang.org/x/tools/gopls/internal/lsp/safetoken"
"golang.org/x/tools/gopls/internal/span"
)
@@ -67,7 +96,7 @@
if span.CompareURI(m.URI, s.URI()) != 0 {
return Range{}, fmt.Errorf("column mapper is for file %q instead of %q", m.URI, s.URI())
}
- s, err := s.WithAll(m.TokFile)
+ s, err := s.WithOffset(m.TokFile)
if err != nil {
return Range{}, err
}
@@ -97,6 +126,19 @@
return Range{Start: startPosition, End: endPosition}, nil
}
+// PosRange returns a protocol Range for the token.Pos interval Content[start:end].
+func (m *ColumnMapper) PosRange(start, end token.Pos) (Range, error) {
+ startOffset, err := safetoken.Offset(m.TokFile, start)
+ if err != nil {
+ return Range{}, fmt.Errorf("start: %v", err)
+ }
+ endOffset, err := safetoken.Offset(m.TokFile, end)
+ if err != nil {
+ return Range{}, fmt.Errorf("end: %v", err)
+ }
+ return m.OffsetRange(startOffset, endOffset)
+}
+
// Position returns the protocol position for the specified point,
// which must have a byte offset.
func (m *ColumnMapper) Position(p span.Point) (Position, error) {
@@ -163,6 +205,8 @@
return m.RangeSpan(l.Range)
}
+// RangeSpan converts a UTF-16 range to a Span with both the
+// position (line/col) and offset fields populated.
func (m *ColumnMapper) RangeSpan(r Range) (span.Span, error) {
start, err := m.Point(r.Start)
if err != nil {
@@ -183,22 +227,13 @@
return spn.Range(m.TokFile)
}
-// Pos returns the token.Pos of p within the mapped file.
+// Pos returns the token.Pos of protocol position p within the mapped file.
func (m *ColumnMapper) Pos(p Position) (token.Pos, error) {
start, err := m.Point(p)
if err != nil {
return token.NoPos, err
}
- // TODO: refactor the span package to avoid creating this unnecessary end position.
- spn, err := span.New(m.URI, start, start).WithAll(m.TokFile)
- if err != nil {
- return token.NoPos, err
- }
- rng, err := spn.Range(m.TokFile)
- if err != nil {
- return token.NoPos, err
- }
- return rng.Start, nil
+ return safetoken.Pos(m.TokFile, start.Offset())
}
// Offset returns the utf-8 byte offset of p within the mapped file.
@@ -210,10 +245,12 @@
return start.Offset(), nil
}
-// Point returns a span.Point for p within the mapped file. The resulting point
-// always has an Offset.
+// Point returns a span.Point for the protocol position p within the mapped file.
+// The resulting point has a valid Position and Offset.
func (m *ColumnMapper) Point(p Position) (span.Point, error) {
line := int(p.Line) + 1
+
+ // Find byte offset of start of containing line.
offset, err := span.ToOffset(m.TokFile, line, 1)
if err != nil {
return span.Point{}, err
diff --git a/gopls/internal/lsp/source/fix.go b/gopls/internal/lsp/source/fix.go
index 1ce6d75..a4c6676 100644
--- a/gopls/internal/lsp/source/fix.go
+++ b/gopls/internal/lsp/source/fix.go
@@ -93,15 +93,11 @@
if !end.IsValid() {
end = edit.Pos
}
- spn, err := span.NewRange(tokFile, edit.Pos, end).Span()
+ fh, err := snapshot.GetVersionedFile(ctx, span.URIFromPath(tokFile.Name()))
if err != nil {
return nil, err
}
- fh, err := snapshot.GetVersionedFile(ctx, spn.URI())
- if err != nil {
- return nil, err
- }
- te, ok := editsPerFile[spn.URI()]
+ te, ok := editsPerFile[fh.URI()]
if !ok {
te = &protocol.TextDocumentEdit{
TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{
@@ -111,13 +107,13 @@
},
},
}
- editsPerFile[spn.URI()] = te
+ editsPerFile[fh.URI()] = te
}
_, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage)
if err != nil {
return nil, err
}
- rng, err := pgf.Mapper.Range(spn)
+ rng, err := pgf.Mapper.PosRange(edit.Pos, end)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go
index b06992e..c933fba 100644
--- a/gopls/internal/lsp/source/util.go
+++ b/gopls/internal/lsp/source/util.go
@@ -17,7 +17,6 @@
"strconv"
"strings"
- "golang.org/x/mod/modfile"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/span"
"golang.org/x/tools/internal/bug"
@@ -26,6 +25,9 @@
// MappedRange provides mapped protocol.Range for a span.Range, accounting for
// UTF-16 code points.
+//
+// TOOD(adonovan): stop treating //line directives specially, and
+// eliminate this type. All callers need either m, or a protocol.Range.
type MappedRange struct {
spanRange span.Range // the range in the compiled source (package.CompiledGoFiles)
m *protocol.ColumnMapper // a mapper of the edited source (package.GoFiles)
@@ -555,26 +557,6 @@
return strings.Contains(s, "command-line-arguments")
}
-// LineToRange creates a Range spanning start and end.
-func LineToRange(m *protocol.ColumnMapper, uri span.URI, start, end modfile.Position) (protocol.Range, error) {
- return ByteOffsetsToRange(m, uri, start.Byte, end.Byte)
-}
-
-// ByteOffsetsToRange creates a range spanning start and end.
-func ByteOffsetsToRange(m *protocol.ColumnMapper, uri span.URI, start, end int) (protocol.Range, error) {
- line, col, err := span.ToPosition(m.TokFile, start)
- if err != nil {
- return protocol.Range{}, err
- }
- s := span.NewPoint(line, col, start)
- line, col, err = span.ToPosition(m.TokFile, end)
- if err != nil {
- return protocol.Range{}, err
- }
- e := span.NewPoint(line, col, end)
- return m.Range(span.New(uri, s, e))
-}
-
// RecvIdent returns the type identifier of a method receiver.
// e.g. A for all of A, *A, A[T], *A[T], etc.
func RecvIdent(recv *ast.FieldList) *ast.Ident {
diff --git a/gopls/internal/lsp/text_synchronization.go b/gopls/internal/lsp/text_synchronization.go
index 9687528..63bc0e8 100644
--- a/gopls/internal/lsp/text_synchronization.go
+++ b/gopls/internal/lsp/text_synchronization.go
@@ -356,6 +356,9 @@
return nil, fmt.Errorf("%w: file not found (%v)", jsonrpc2.ErrInternal, err)
}
for _, change := range changes {
+ // TODO(adonovan): refactor to use diff.Apply, which is robust w.r.t.
+ // out-of-order or overlapping changes---and much more efficient.
+
// Make sure to update column mapper along with the content.
m := protocol.NewColumnMapper(uri, content)
if change.Range == nil {
@@ -365,9 +368,6 @@
if err != nil {
return nil, err
}
- if !spn.HasOffset() {
- return nil, fmt.Errorf("%w: invalid range for content change", jsonrpc2.ErrInternal)
- }
start, end := spn.Start().Offset(), spn.End().Offset()
if end < start {
return nil, fmt.Errorf("%w: invalid range for content change", jsonrpc2.ErrInternal)
diff --git a/gopls/internal/lsp/work/diagnostics.go b/gopls/internal/lsp/work/diagnostics.go
index 5d3a328..0d0f4eb 100644
--- a/gopls/internal/lsp/work/diagnostics.go
+++ b/gopls/internal/lsp/work/diagnostics.go
@@ -59,7 +59,7 @@
// Add diagnostic if a directory does not contain a module.
var diagnostics []*source.Diagnostic
for _, use := range pw.File.Use {
- rng, err := source.LineToRange(pw.Mapper, fh.URI(), use.Syntax.Start, use.Syntax.End)
+ rng, err := pw.Mapper.OffsetRange(use.Syntax.Start.Byte, use.Syntax.End.Byte)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/work/hover.go b/gopls/internal/lsp/work/hover.go
index ea4c1e5..641028b 100644
--- a/gopls/internal/lsp/work/hover.go
+++ b/gopls/internal/lsp/work/hover.go
@@ -11,9 +11,9 @@
"go/token"
"golang.org/x/mod/modfile"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
)
func Hover(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, position protocol.Position) (*protocol.Hover, error) {
@@ -56,7 +56,7 @@
mod := pm.File.Module.Mod
// Get the range to highlight for the hover.
- rng, err := source.ByteOffsetsToRange(pw.Mapper, fh.URI(), pathStart, pathEnd)
+ rng, err := pw.Mapper.OffsetRange(pathStart, pathEnd)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/span/span.go b/gopls/internal/span/span.go
index 5b8d31e..333048a 100644
--- a/gopls/internal/span/span.go
+++ b/gopls/internal/span/span.go
@@ -209,7 +209,8 @@
}
}
-func (s Span) WithPosition(tf *token.File) (Span, error) {
+// (Currently unused, but we gain little yet by deleting it.)
+func (s Span) withPosition(tf *token.File) (Span, error) {
if err := s.update(tf, true, false); err != nil {
return Span{}, err
}
diff --git a/gopls/internal/span/utf16.go b/gopls/internal/span/utf16.go
index f4c93a6..0f8e1bc 100644
--- a/gopls/internal/span/utf16.go
+++ b/gopls/internal/span/utf16.go
@@ -13,6 +13,9 @@
// supplied file contents.
// This is used to convert from the native (always in bytes) column
// representation and the utf16 counts used by some editors.
+//
+// TODO(adonovan): this function is unused except by its test. Delete,
+// or consolidate with (*protocol.ColumnMapper).utf16Column.
func ToUTF16Column(p Point, content []byte) (int, error) {
if !p.HasPosition() {
return -1, fmt.Errorf("ToUTF16Column: point is missing position")
