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 <[email protected]> Run-TryBot: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
golang · May 17, 2022 · facb0d3 · facb0d3
1 parent 814e0d7
commit facb0d3
Show file tree

Hide file tree

Showing 8 changed files with 63 additions and 72 deletions.
diff --git a/go/packages/packagestest/expect.go b/go/packages/packagestest/expect.go
@@ -422,7 +422,7 @@ func (e *Exported) rangeConverter(n *expect.Note, args []interface{}) (span.Rang
 			// 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 @@ func (e *Exported) rangeConverter(n *expect.Note, args []interface{}) (span.Rang
 		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 @@ func (e *Exported) rangeConverter(n *expect.Note, args []interface{}) (span.Rang
 		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
@@ -324,7 +324,7 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod
 			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
@@ -322,7 +322,7 @@ func computeTextEdits(ctx context.Context, snapshot Snapshot, pgf *ParsedGoFile,
 
 // 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
@@ -36,13 +36,8 @@ type MappedRange struct {
 // 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
@@ -41,15 +41,6 @@ var Invalid = Span{v: span{Start: invalidPoint.v, End: invalidPoint.v}}
 
 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) Format(f fmt.State, c rune) {
 	}
 }
 
-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 @@ func (s *Span) update(c Converter, withPos, withOffset bool) error {
 	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 @@ func (p *point) updatePosition(c Converter) error {
 	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
@@ -6,6 +6,7 @@ package span_test
 
 import (
 	"fmt"
+	"go/token"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -59,12 +60,15 @@ func toPath(value string) string {
 	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
@@ -5,6 +5,7 @@
 package span
 
 import (
+	"errors"
 	"fmt"
 	"go/token"
 )
@@ -13,38 +14,37 @@ import (
 // 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 @@ func NewContentConverter(filename string, content []byte) *TokenConverter {
 	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 @@ func (r Range) Span() (Span, error) {
 	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 @@ func FileSpan(tok *token.File, converter Converter, start, end token.Pos) (Span,
 	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 @@ func positionFromOffset(f *token.File, offset int) (string, int, int, error) {
 // 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 @@ func (s Span) Range(converter *TokenConverter) (Range, error) {
 	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
@@ -48,7 +48,7 @@ func TestToken(t *testing.T) {
 	}
 	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(),