internal/span: eliminate Converter and FileConverter
The only real implementation of position conversion was via a
*token.File, so refactor the converter logic to eliminate the Converter
interface, and just use a single converter implementation that uses a
*token.File to convert between offsets and positions.
This change is meant to be a zero-impact refactoring for non-test code.
As such, I abstained from panicking in several places where it would
make sense. In later CLs, once the bug reporting API lands, we can
insert bug reports in these places.
Change-Id: Id2e503acd80d089bc5d73e983215784015471f04
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405546
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/go/packages/packagestest/expect.go b/go/packages/packagestest/expect.go
index 3088d43..4302586 100644
--- a/go/packages/packagestest/expect.go
+++ b/go/packages/packagestest/expect.go
@@ -422,7 +422,7 @@
// end of file identifier, look up the current file
f := e.ExpectFileSet.File(n.Pos)
eof := f.Pos(f.Size())
- return span.Range{FileSet: e.ExpectFileSet, Start: eof, End: token.NoPos}, args, nil
+ return span.NewRange(e.ExpectFileSet, eof, token.NoPos), args, nil
default:
// look up an marker by name
mark, ok := e.markers[string(arg)]
@@ -439,7 +439,7 @@
if start == token.NoPos {
return span.Range{}, nil, fmt.Errorf("%v: pattern %s did not match", e.ExpectFileSet.Position(n.Pos), arg)
}
- return span.Range{FileSet: e.ExpectFileSet, Start: start, End: end}, args, nil
+ return span.NewRange(e.ExpectFileSet, start, end), args, nil
case *regexp.Regexp:
start, end, err := expect.MatchBefore(e.ExpectFileSet, e.FileContents, n.Pos, arg)
if err != nil {
@@ -448,7 +448,7 @@
if start == token.NoPos {
return span.Range{}, nil, fmt.Errorf("%v: pattern %s did not match", e.ExpectFileSet.Position(n.Pos), arg)
}
- return span.Range{FileSet: e.ExpectFileSet, Start: start, End: end}, args, nil
+ return span.NewRange(e.ExpectFileSet, start, end), args, nil
default:
return span.Range{}, nil, fmt.Errorf("cannot convert %v to pos", arg)
}
diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index d6c4bc0..5d751eb 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -324,7 +324,7 @@
Tok: tok,
Mapper: &protocol.ColumnMapper{
URI: fh.URI(),
- Converter: span.NewTokenConverter(fset, tok),
+ Converter: span.NewTokenConverter(tok),
Content: src,
},
ParseErr: parseErr,
diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go
index 05fb005..d8e66ab 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -322,7 +322,7 @@
// ProtocolEditsFromSource converts text edits to LSP edits using the original
// source.
-func ProtocolEditsFromSource(src []byte, edits []diff.TextEdit, converter span.Converter) ([]protocol.TextEdit, error) {
+func ProtocolEditsFromSource(src []byte, edits []diff.TextEdit, converter *span.TokenConverter) ([]protocol.TextEdit, error) {
m := lsppos.NewMapper(src)
var result []protocol.TextEdit
for _, edit := range edits {
diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go
index 768ff8c..a76c916 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -36,13 +36,8 @@
// NewMappedRange returns a MappedRange for the given start and end token.Pos.
func NewMappedRange(fset *token.FileSet, m *protocol.ColumnMapper, start, end token.Pos) MappedRange {
return MappedRange{
- spanRange: span.Range{
- FileSet: fset,
- Start: start,
- End: end,
- Converter: m.Converter,
- },
- m: m,
+ spanRange: span.NewRange(fset, start, end),
+ m: m,
}
}
diff --git a/internal/span/span.go b/internal/span/span.go
index 4d2ad09..fdf0644 100644
--- a/internal/span/span.go
+++ b/internal/span/span.go
@@ -41,15 +41,6 @@
var invalidPoint = Point{v: point{Line: 0, Column: 0, Offset: -1}}
-// Converter is the interface to an object that can convert between line:column
-// and offset forms for a single file.
-type Converter interface {
- //ToPosition converts from an offset to a line:column pair.
- ToPosition(offset int) (int, int, error)
- //ToOffset converts from a line:column pair to an offset.
- ToOffset(line, col int) (int, error)
-}
-
func New(uri URI, start Point, end Point) Span {
s := Span{v: span{URI: uri, Start: start.v, End: end.v}}
s.v.clean()
@@ -217,28 +208,28 @@
}
}
-func (s Span) WithPosition(c Converter) (Span, error) {
+func (s Span) WithPosition(c *TokenConverter) (Span, error) {
if err := s.update(c, true, false); err != nil {
return Span{}, err
}
return s, nil
}
-func (s Span) WithOffset(c Converter) (Span, error) {
+func (s Span) WithOffset(c *TokenConverter) (Span, error) {
if err := s.update(c, false, true); err != nil {
return Span{}, err
}
return s, nil
}
-func (s Span) WithAll(c Converter) (Span, error) {
+func (s Span) WithAll(c *TokenConverter) (Span, error) {
if err := s.update(c, true, true); err != nil {
return Span{}, err
}
return s, nil
}
-func (s *Span) update(c Converter, withPos, withOffset bool) error {
+func (s *Span) update(c *TokenConverter, withPos, withOffset bool) error {
if !s.IsValid() {
return fmt.Errorf("cannot add information to an invalid span")
}
@@ -265,7 +256,7 @@
return nil
}
-func (p *point) updatePosition(c Converter) error {
+func (p *point) updatePosition(c *TokenConverter) error {
line, col, err := c.ToPosition(p.Offset)
if err != nil {
return err
@@ -275,7 +266,7 @@
return nil
}
-func (p *point) updateOffset(c Converter) error {
+func (p *point) updateOffset(c *TokenConverter) error {
offset, err := c.ToOffset(p.Line, p.Column)
if err != nil {
return err
diff --git a/internal/span/span_test.go b/internal/span/span_test.go
index 150ea3f..3956565 100644
--- a/internal/span/span_test.go
+++ b/internal/span/span_test.go
@@ -6,6 +6,7 @@
import (
"fmt"
+ "go/token"
"path/filepath"
"strings"
"testing"
@@ -59,12 +60,15 @@
return filepath.FromSlash(value)
}
-type lines int
-
-func (l lines) ToPosition(offset int) (int, int, error) {
- return (offset / int(l)) + 1, (offset % int(l)) + 1, nil
-}
-
-func (l lines) ToOffset(line, col int) (int, error) {
- return (int(l) * (line - 1)) + (col - 1), nil
+// lines creates a new tokenConverter for a file with 1000 lines, each width
+// bytes wide.
+func lines(width int) *span.TokenConverter {
+ fset := token.NewFileSet()
+ f := fset.AddFile("", -1, 1000*width)
+ var lines []int
+ for i := 0; i < 1000; i++ {
+ lines = append(lines, i*width)
+ }
+ f.SetLines(lines)
+ return span.NewTokenConverter(f)
}
diff --git a/internal/span/token.go b/internal/span/token.go
index 3fd2219..c2b65e7 100644
--- a/internal/span/token.go
+++ b/internal/span/token.go
@@ -5,6 +5,7 @@
package span
import (
+ "errors"
"fmt"
"go/token"
)
@@ -13,38 +14,37 @@
// It also carries the FileSet that produced the positions, so that it is
// self contained.
type Range struct {
- FileSet *token.FileSet
- Start token.Pos
- End token.Pos
- Converter Converter
+ Start token.Pos
+ End token.Pos
+ file *token.File // possibly nil, if Start or End is invalid
}
-type FileConverter struct {
- file *token.File
-}
-
-// TokenConverter is a Converter backed by a token file set and file.
-// It uses the file set methods to work out the conversions, which
-// makes it fast and does not require the file contents.
+// TokenConverter converts between offsets and (col, row) using a token.File.
type TokenConverter struct {
- FileConverter
- fset *token.FileSet
+ file *token.File
}
// NewRange creates a new Range from a FileSet and two positions.
// To represent a point pass a 0 as the end pos.
func NewRange(fset *token.FileSet, start, end token.Pos) Range {
+ file := fset.File(start)
+ if file == nil {
+ // TODO: report a bug here via the bug reporting API, once it is available.
+ }
return Range{
- FileSet: fset,
- Start: start,
- End: end,
+ Start: start,
+ End: end,
+ file: file,
}
}
// NewTokenConverter returns an implementation of Converter backed by a
// token.File.
-func NewTokenConverter(fset *token.FileSet, f *token.File) *TokenConverter {
- return &TokenConverter{fset: fset, FileConverter: FileConverter{file: f}}
+func NewTokenConverter(f *token.File) *TokenConverter {
+ if f == nil {
+ // TODO: report a bug here using the bug reporting API.
+ }
+ return &TokenConverter{file: f}
}
// NewContentConverter returns an implementation of Converter for the
@@ -53,7 +53,7 @@
fset := token.NewFileSet()
f := fset.AddFile(filename, -1, len(content))
f.SetLinesForContent(content)
- return NewTokenConverter(fset, f)
+ return NewTokenConverter(f)
}
// IsPoint returns true if the range represents a single point.
@@ -68,16 +68,15 @@
if !r.Start.IsValid() {
return Span{}, fmt.Errorf("start pos is not valid")
}
- f := r.FileSet.File(r.Start)
- if f == nil {
- return Span{}, fmt.Errorf("file not found in FileSet")
+ if r.file == nil {
+ return Span{}, errors.New("range missing file association")
}
- return FileSpan(f, r.Converter, r.Start, r.End)
+ return FileSpan(r.file, NewTokenConverter(r.file), r.Start, r.End)
}
// FileSpan returns a span within tok, using converter to translate between
// offsets and positions.
-func FileSpan(tok *token.File, converter Converter, start, end token.Pos) (Span, error) {
+func FileSpan(tok *token.File, converter *TokenConverter, start, end token.Pos) (Span, error) {
var s Span
var err error
var startFilename string
@@ -107,7 +106,7 @@
if startFilename != tok.Name() {
return Span{}, fmt.Errorf("must supply Converter for file %q containing lines from %q", tok.Name(), startFilename)
}
- return s.WithOffset(&FileConverter{tok})
+ return s.WithOffset(&TokenConverter{tok})
}
func position(f *token.File, pos token.Pos) (string, int, int, error) {
@@ -136,7 +135,7 @@
// that it does not panic on invalid positions.
func offset(f *token.File, pos token.Pos) (int, error) {
if int(pos) < f.Base() || int(pos) > f.Base()+f.Size() {
- return 0, fmt.Errorf("invalid pos")
+ return 0, fmt.Errorf("invalid pos: %d not in [%d, %d]", pos, f.Base(), f.Base()+f.Size())
}
return int(pos) - f.Base(), nil
}
@@ -148,28 +147,30 @@
if err != nil {
return Range{}, err
}
+ file := converter.file
// go/token will panic if the offset is larger than the file's size,
// so check here to avoid panicking.
- if s.Start().Offset() > converter.file.Size() {
- return Range{}, fmt.Errorf("start offset %v is past the end of the file %v", s.Start(), converter.file.Size())
+ if s.Start().Offset() > file.Size() {
+ // TODO: report a bug here via the future bug reporting API.
+ return Range{}, fmt.Errorf("start offset %v is past the end of the file %v", s.Start(), file.Size())
}
- if s.End().Offset() > converter.file.Size() {
- return Range{}, fmt.Errorf("end offset %v is past the end of the file %v", s.End(), converter.file.Size())
+ if s.End().Offset() > file.Size() {
+ // TODO: report a bug here via the future bug reporting API.
+ return Range{}, fmt.Errorf("end offset %v is past the end of the file %v", s.End(), file.Size())
}
return Range{
- FileSet: converter.fset,
- Start: converter.file.Pos(s.Start().Offset()),
- End: converter.file.Pos(s.End().Offset()),
- Converter: converter,
+ Start: file.Pos(s.Start().Offset()),
+ End: file.Pos(s.End().Offset()),
+ file: file,
}, nil
}
-func (l *FileConverter) ToPosition(offset int) (int, int, error) {
+func (l *TokenConverter) ToPosition(offset int) (int, int, error) {
_, line, col, err := positionFromOffset(l.file, offset)
return line, col, err
}
-func (l *FileConverter) ToOffset(line, col int) (int, error) {
+func (l *TokenConverter) ToOffset(line, col int) (int, error) {
if line < 0 {
return -1, fmt.Errorf("line is not valid")
}
diff --git a/internal/span/token_test.go b/internal/span/token_test.go
index 81b2631..5f48d68 100644
--- a/internal/span/token_test.go
+++ b/internal/span/token_test.go
@@ -48,7 +48,7 @@
}
for _, test := range tokenTests {
f := files[test.URI()]
- c := span.NewTokenConverter(fset, f)
+ c := span.NewTokenConverter(f)
t.Run(path.Base(f.Name()), func(t *testing.T) {
checkToken(t, c, span.New(
test.URI(),
