internal/lsp: use the correct converter for mapped range offsets
CL 405546 introduced a latent bug in MappedRange, because it naively
used the wrong TokenConverter to convert mapped positions to offsets.
This was detected via related clean-up work in another CL. Fix this by
passing the correct converter from MappedRange.Range. Add a test that
would have demonstrated the breakage.
More cleanup is needed here. It is subtle that MappedRange.Converter
maps the adjusted position for its start and end, and there may be some
places where this invariant has been broken over the years.
Add additional documentation and bug reports.
Change-Id: If7f177894bac1242ddcc1786e79c7559455e9291
Reviewed-on: https://go-review.googlesource.com/c/tools/+/407887
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/internal/lsp/definition.go b/internal/lsp/definition.go
index 599228a..9487c68 100644
--- a/internal/lsp/definition.go
+++ b/internal/lsp/definition.go
@@ -6,6 +6,7 @@
import (
"context"
+ "fmt"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
@@ -54,6 +55,9 @@
if err != nil {
return nil, err
}
+ if ident.Type.Object == nil {
+ return nil, fmt.Errorf("no type definition for %s", ident.Name)
+ }
identRange, err := ident.Type.Range()
if err != nil {
return nil, err
diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go
index 641e463..8e40694 100644
--- a/internal/lsp/source/rename.go
+++ b/internal/lsp/source/rename.go
@@ -198,7 +198,7 @@
return nil, err
}
for _, ref := range r.refs {
- refSpan, err := ref.spanRange.Span()
+ refSpan, err := ref.Span()
if err != nil {
return nil, err
}
diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go
index a76c916..ccade4a 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -18,6 +18,7 @@
"strings"
"golang.org/x/mod/modfile"
+ "golang.org/x/tools/internal/lsp/bug"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/span"
)
@@ -25,25 +26,45 @@
// MappedRange provides mapped protocol.Range for a span.Range, accounting for
// UTF-16 code points.
type MappedRange struct {
- spanRange span.Range
- m *protocol.ColumnMapper
+ spanRange span.Range // the range in the compiled source (package.CompiledGoFiles)
+ m *protocol.ColumnMapper // a mapper of the edited source (package.GoFiles)
// protocolRange is the result of converting the spanRange using the mapper.
- // It is computed on-demand.
+ // It is computed lazily.
protocolRange *protocol.Range
}
// NewMappedRange returns a MappedRange for the given start and end token.Pos.
+//
+// By convention, start and end are assumed to be positions in the compiled (==
+// type checked) source, whereas the column mapper m maps positions in the
+// user-edited source. Note that these may not be the same, as when using CGo:
+// CompiledGoFiles contains generated files, whose positions (via
+// token.File.Position) point to locations in the edited file -- the file
+// containing `import "C"`.
func NewMappedRange(fset *token.FileSet, m *protocol.ColumnMapper, start, end token.Pos) MappedRange {
+ if tf := fset.File(start); tf == nil {
+ bug.Report("nil file", nil)
+ } else {
+ mapped := m.Converter.TokFile.Name()
+ adjusted := tf.PositionFor(start, true) // adjusted position
+ if adjusted.Filename != mapped {
+ bug.Reportf("mapped file %q does not match start position file %q", mapped, adjusted.Filename)
+ }
+ }
return MappedRange{
spanRange: span.NewRange(fset, start, end),
m: m,
}
}
+// Range returns the LSP range in the edited source.
+//
+// See the documentation of NewMappedRange for information on edited vs
+// compiled source.
func (s MappedRange) Range() (protocol.Range, error) {
if s.protocolRange == nil {
- spn, err := s.spanRange.Span()
+ spn, err := s.Span()
if err != nil {
return protocol.Range{}, err
}
@@ -56,14 +77,33 @@
return *s.protocolRange, nil
}
+// Span returns the span corresponding to the mapped range in the edited
+// source.
+//
+// See the documentation of NewMappedRange for information on edited vs
+// compiled source.
func (s MappedRange) Span() (span.Span, error) {
- return s.spanRange.Span()
+ // In the past, some code-paths have relied on Span returning an error if s
+ // is the zero value (i.e. s.m is nil). But this should be treated as a bug:
+ // observe that s.URI() would panic in this case.
+ if s.m == nil {
+ return span.Span{}, bug.Errorf("invalid range")
+ }
+ return span.FileSpan(s.spanRange.TokFile, s.m.Converter, s.spanRange.Start, s.spanRange.End)
}
+// SpanRange returns a span.Range in the compiled source.
+//
+// See the documentation of NewMappedRange for information on edited vs
+// compiled source.
func (s MappedRange) SpanRange() span.Range {
return s.spanRange
}
+// URI returns the URI of the edited file.
+//
+// See the documentation of NewMappedRange for information on edited vs
+// compiled source.
func (s MappedRange) URI() span.URI {
return s.m.URI
}
diff --git a/internal/lsp/source/util_test.go b/internal/lsp/source/util_test.go
new file mode 100644
index 0000000..2920774
--- /dev/null
+++ b/internal/lsp/source/util_test.go
@@ -0,0 +1,83 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+ "bytes"
+ "go/scanner"
+ "go/token"
+ "testing"
+
+ "golang.org/x/tools/internal/lsp/protocol"
+ "golang.org/x/tools/internal/span"
+)
+
+func TestMappedRangeAdjustment(t *testing.T) {
+ // Test that mapped range adjusts positions in compiled files to positions in
+ // the corresponding edited file.
+
+ compiled := []byte(`// Generated. DO NOT EDIT.
+
+package p
+
+//line edited.go:3:1
+const a𐐀b = 42`)
+ edited := []byte(`package p
+
+const a𐐀b = 42`)
+
+ fset := token.NewFileSet()
+ cf := scanFile(fset, "compiled.go", compiled)
+ ef := scanFile(fset, "edited.go", edited)
+ eURI := span.URIFromPath(ef.Name())
+
+ mapper := &protocol.ColumnMapper{
+ URI: eURI,
+ Converter: span.NewTokenConverter(ef),
+ Content: edited,
+ }
+
+ start := cf.Pos(bytes.Index(compiled, []byte("a𐐀b")))
+ end := start + token.Pos(len("a𐐀b"))
+ mr := NewMappedRange(fset, mapper, start, end)
+ gotRange, err := mr.Range()
+ if err != nil {
+ t.Fatal(err)
+ }
+ wantRange := protocol.Range{
+ Start: protocol.Position{Line: 2, Character: 6},
+ End: protocol.Position{Line: 2, Character: 10},
+ }
+ if gotRange != wantRange {
+ t.Errorf("NewMappedRange(...).Range(): got %v, want %v", gotRange, wantRange)
+ }
+
+ // Verify that the mapped span is also in the edited file.
+ gotSpan, err := mr.Span()
+ if err != nil {
+ t.Fatal(err)
+ }
+ if gotURI := gotSpan.URI(); gotURI != eURI {
+ t.Errorf("mr.Span().URI() = %v, want %v", gotURI, eURI)
+ }
+ wantOffset := bytes.Index(edited, []byte("a𐐀b"))
+ if gotOffset := gotSpan.Start().Offset(); gotOffset != wantOffset {
+ t.Errorf("mr.Span().Start().Offset() = %d, want %d", gotOffset, wantOffset)
+ }
+}
+
+// scanFile scans the a file into fset, in order to honor line directives.
+func scanFile(fset *token.FileSet, name string, content []byte) *token.File {
+ f := fset.AddFile(name, -1, len(content))
+ var s scanner.Scanner
+ s.Init(f, content, nil, scanner.ScanComments)
+ for {
+ _, tok, _ := s.Scan()
+ if tok == token.EOF {
+ break
+ }
+ }
+ return f
+}
diff --git a/internal/span/token.go b/internal/span/token.go
index a64e5e8..6e62776 100644
--- a/internal/span/token.go
+++ b/internal/span/token.go
@@ -5,7 +5,6 @@
package span
import (
- "errors"
"fmt"
"go/token"
@@ -18,12 +17,20 @@
type Range struct {
Start token.Pos
End token.Pos
- file *token.File // possibly nil, if Start or End is invalid
+
+ // TokFile may be nil if Start or End is invalid.
+ // TODO: Eventually we should guarantee that it is non-nil.
+ TokFile *token.File
}
// TokenConverter converts between offsets and (col, row) using a token.File.
+//
+// TODO(rfindley): eliminate TokenConverter in favor of just operating on
+// token.File.
type TokenConverter struct {
- file *token.File
+ // TokFile is exported for invariant checking; it may be nil in the case of
+ // an invalid converter.
+ TokFile *token.File
}
// NewRange creates a new Range from a FileSet and two positions.
@@ -34,9 +41,9 @@
bug.Reportf("nil file")
}
return Range{
- Start: start,
- End: end,
- file: file,
+ Start: start,
+ End: end,
+ TokFile: file,
}
}
@@ -46,7 +53,7 @@
if f == nil {
bug.Reportf("nil file")
}
- return &TokenConverter{file: f}
+ return &TokenConverter{TokFile: f}
}
// NewContentConverter returns an implementation of Converter for the
@@ -67,18 +74,22 @@
// It will fill in all the members of the Span, calculating the line and column
// information.
func (r Range) Span() (Span, error) {
- if !r.Start.IsValid() {
- return Span{}, fmt.Errorf("start pos is not valid")
- }
- if r.file == nil {
- return Span{}, errors.New("range missing file association")
- }
- return FileSpan(r.file, NewTokenConverter(r.file), r.Start, r.End)
+ return FileSpan(r.TokFile, NewTokenConverter(r.TokFile), r.Start, r.End)
}
// FileSpan returns a span within tok, using converter to translate between
// offsets and positions.
+//
+// If non-nil, the converter must be a converter for the source file pointed to
+// by start, after accounting for //line directives, as it will be used to
+// compute offsets for the resulting Span.
func FileSpan(tok *token.File, converter *TokenConverter, start, end token.Pos) (Span, error) {
+ if !start.IsValid() {
+ return Span{}, fmt.Errorf("start pos is not valid")
+ }
+ if tok == nil {
+ return Span{}, bug.Errorf("missing file association") // should never get here with a nil file
+ }
var s Span
var err error
var startFilename string
@@ -102,13 +113,13 @@
s.v.Start.clean()
s.v.End.clean()
s.v.clean()
- if converter != nil {
- return s.WithOffset(converter)
+ if converter == nil {
+ converter = &TokenConverter{tok}
}
- if startFilename != tok.Name() {
- return Span{}, fmt.Errorf("must supply Converter for file %q containing lines from %q", tok.Name(), startFilename)
+ if startFilename != converter.TokFile.Name() {
+ return Span{}, bug.Errorf("must supply Converter for file %q containing lines from %q", tok.Name(), startFilename)
}
- return s.WithOffset(&TokenConverter{tok})
+ return s.WithOffset(converter)
}
func position(f *token.File, pos token.Pos) (string, int, int, error) {
@@ -149,7 +160,7 @@
if err != nil {
return Range{}, err
}
- file := converter.file
+ file := converter.TokFile
// go/token will panic if the offset is larger than the file's size,
// so check here to avoid panicking.
if s.Start().Offset() > file.Size() {
@@ -159,14 +170,14 @@
return Range{}, bug.Errorf("end offset %v is past the end of the file %v", s.End(), file.Size())
}
return Range{
- Start: file.Pos(s.Start().Offset()),
- End: file.Pos(s.End().Offset()),
- file: file,
+ Start: file.Pos(s.Start().Offset()),
+ End: file.Pos(s.End().Offset()),
+ TokFile: file,
}, nil
}
func (l *TokenConverter) ToPosition(offset int) (int, int, error) {
- _, line, col, err := positionFromOffset(l.file, offset)
+ _, line, col, err := positionFromOffset(l.TokFile, offset)
return line, col, err
}
@@ -174,7 +185,7 @@
if line < 0 {
return -1, fmt.Errorf("line is not valid")
}
- lineMax := l.file.LineCount() + 1
+ lineMax := l.TokFile.LineCount() + 1
if line > lineMax {
return -1, fmt.Errorf("line is beyond end of file %v", lineMax)
} else if line == lineMax {
@@ -182,14 +193,14 @@
return -1, fmt.Errorf("column is beyond end of file")
}
// at the end of the file, allowing for a trailing eol
- return l.file.Size(), nil
+ return l.TokFile.Size(), nil
}
- pos := l.file.LineStart(line)
+ pos := l.TokFile.LineStart(line)
if !pos.IsValid() {
return -1, fmt.Errorf("line is not in file")
}
// we assume that column is in bytes here, and that the first byte of a
// line is at column 1
pos += token.Pos(col - 1)
- return offset(l.file, pos)
+ return offset(l.TokFile, pos)
}
