go/packages/packagestest: break dependency on gopls' span package

The span package is properly part of gopls, and we'd like to move
it into that module. However, the packagestest package unfortunately
depends on span.Span as an alternative notation for Ranges.

This change decouples span.Span from packagestest.Range using a
new (unexported for now) rangeSetter interface, which Span implements.
Neither package depends on the other.

Technically this is a breaking API change:
all the Range methods have gone away, as have the Span, URI,
and Point types and their methods, which were accessible via
Range.Span(). However, clients would not be able to access
these internal types, and I think it is highly unlikely that
anyone depends on it. Nonethless this is a cautionary tale about
the risks from one innocuous-looking type alias declaration.

Change-Id: I8acb03f4acb1f798f304b03648445e37a44f9c45
Reviewed-on: https://go-review.googlesource.com/c/tools/+/439715
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

go/packages/packagestest/expect.go

@@ -16,7 +16,6 @@ import (
 
 	"golang.org/x/tools/go/expect"
 	"golang.org/x/tools/go/packages"
-	"golang.org/x/tools/internal/span"
 )
 
 const (
@@ -124,14 +123,31 @@ func (e *Exported) Expect(methods map[string]interface{}) error {
 	return nil
 }
 
-// Range is a type alias for span.Range for backwards compatibility, prefer
-// using span.Range directly.
-type Range = span.Range
+// A Range represents an interval within a source file in go/token notation.
+type Range struct {
+	TokFile    *token.File // non-nil
+	Start, End token.Pos   // both valid and within range of TokFile
+}
+
+// A rangeSetter abstracts a variable that can be set from a Range value.
+//
+// The parameter conversion machinery will automatically construct a
+// variable of type T and call the SetRange method on its address if
+// *T implements rangeSetter. This allows alternative notations of
+// source ranges to interoperate transparently with this package.
+//
+// This type intentionally does not mention Range itself, to avoid a
+// dependency from the application's range type upon this package.
+//
+// Currently this is a secret back door for use only by gopls.
+type rangeSetter interface {
+	SetRange(file *token.File, start, end token.Pos)
+}
 
 // Mark adds a new marker to the known set.
 func (e *Exported) Mark(name string, r Range) {
 	if e.markers == nil {
-		e.markers = make(map[string]span.Range)
+		e.markers = make(map[string]Range)
 	}
 	e.markers[name] = r
 }
@@ -221,22 +237,22 @@ func (e *Exported) getMarkers() error {
 		return nil
 	}
 	// set markers early so that we don't call getMarkers again from Expect
-	e.markers = make(map[string]span.Range)
+	e.markers = make(map[string]Range)
 	return e.Expect(map[string]interface{}{
 		markMethod: e.Mark,
 	})
 }
 
 var (
-	noteType       = reflect.TypeOf((*expect.Note)(nil))
-	identifierType = reflect.TypeOf(expect.Identifier(""))
-	posType        = reflect.TypeOf(token.Pos(0))
-	positionType   = reflect.TypeOf(token.Position{})
-	rangeType      = reflect.TypeOf(span.Range{})
-	spanType       = reflect.TypeOf(span.Span{})
-	fsetType       = reflect.TypeOf((*token.FileSet)(nil))
-	regexType      = reflect.TypeOf((*regexp.Regexp)(nil))
-	exportedType   = reflect.TypeOf((*Exported)(nil))
+	noteType        = reflect.TypeOf((*expect.Note)(nil))
+	identifierType  = reflect.TypeOf(expect.Identifier(""))
+	posType         = reflect.TypeOf(token.Pos(0))
+	positionType    = reflect.TypeOf(token.Position{})
+	rangeType       = reflect.TypeOf(Range{})
+	rangeSetterType = reflect.TypeOf((*rangeSetter)(nil)).Elem()
+	fsetType        = reflect.TypeOf((*token.FileSet)(nil))
+	regexType       = reflect.TypeOf((*regexp.Regexp)(nil))
+	exportedType    = reflect.TypeOf((*Exported)(nil))
 )
 
 // converter converts from a marker's argument parsed from the comment to
@@ -295,17 +311,16 @@ func (e *Exported) buildConverter(pt reflect.Type) (converter, error) {
 			}
 			return reflect.ValueOf(r), remains, nil
 		}, nil
-	case pt == spanType:
+	case reflect.PtrTo(pt).AssignableTo(rangeSetterType):
+		// (*pt).SetRange method exists: call it.
 		return func(n *expect.Note, args []interface{}) (reflect.Value, []interface{}, error) {
 			r, remains, err := e.rangeConverter(n, args)
 			if err != nil {
 				return reflect.Value{}, nil, err
 			}
-			spn, err := r.Span()
-			if err != nil {
-				return reflect.Value{}, nil, err
-			}
-			return reflect.ValueOf(spn), remains, nil
+			v := reflect.New(pt)
+			v.Interface().(rangeSetter).SetRange(r.TokFile, r.Start, r.End)
+			return v.Elem(), remains, nil
 		}, nil
 	case pt == identifierType:
 		return func(n *expect.Note, args []interface{}) (reflect.Value, []interface{}, error) {
@@ -408,10 +423,10 @@ func (e *Exported) buildConverter(pt reflect.Type) (converter, error) {
 	}
 }
 
-func (e *Exported) rangeConverter(n *expect.Note, args []interface{}) (span.Range, []interface{}, error) {
+func (e *Exported) rangeConverter(n *expect.Note, args []interface{}) (Range, []interface{}, error) {
 	tokFile := e.ExpectFileSet.File(n.Pos)
 	if len(args) < 1 {
-		return span.Range{}, nil, fmt.Errorf("missing argument")
+		return Range{}, nil, fmt.Errorf("missing argument")
 	}
 	arg := args[0]
 	args = args[1:]
@@ -422,34 +437,60 @@ func (e *Exported) rangeConverter(n *expect.Note, args []interface{}) (span.Rang
 		case eofIdentifier:
 			// end of file identifier
 			eof := tokFile.Pos(tokFile.Size())
-			return span.NewRange(tokFile, eof, eof), args, nil
+			return newRange(tokFile, eof, eof), args, nil
 		default:
 			// look up an marker by name
 			mark, ok := e.markers[string(arg)]
 			if !ok {
-				return span.Range{}, nil, fmt.Errorf("cannot find marker %v", arg)
+				return Range{}, nil, fmt.Errorf("cannot find marker %v", arg)
 			}
 			return mark, args, nil
 		}
 	case string:
 		start, end, err := expect.MatchBefore(e.ExpectFileSet, e.FileContents, n.Pos, arg)
 		if err != nil {
-			return span.Range{}, nil, err
+			return Range{}, nil, err
 		}
 		if !start.IsValid() {
-			return span.Range{}, nil, fmt.Errorf("%v: pattern %s did not match", e.ExpectFileSet.Position(n.Pos), arg)
+			return Range{}, nil, fmt.Errorf("%v: pattern %s did not match", e.ExpectFileSet.Position(n.Pos), arg)
 		}
-		return span.NewRange(tokFile, start, end), args, nil
+		return newRange(tokFile, start, end), args, nil
 	case *regexp.Regexp:
 		start, end, err := expect.MatchBefore(e.ExpectFileSet, e.FileContents, n.Pos, arg)
 		if err != nil {
-			return span.Range{}, nil, err
+			return Range{}, nil, err
 		}
 		if !start.IsValid() {
-			return span.Range{}, nil, fmt.Errorf("%v: pattern %s did not match", e.ExpectFileSet.Position(n.Pos), arg)
+			return Range{}, nil, fmt.Errorf("%v: pattern %s did not match", e.ExpectFileSet.Position(n.Pos), arg)
 		}
-		return span.NewRange(tokFile, start, end), args, nil
+		return newRange(tokFile, start, end), args, nil
 	default:
-		return span.Range{}, nil, fmt.Errorf("cannot convert %v to pos", arg)
+		return Range{}, nil, fmt.Errorf("cannot convert %v to pos", arg)
+	}
+}
+
+// newRange creates a new Range from a token.File and two valid positions within it.
+func newRange(file *token.File, start, end token.Pos) Range {
+	fileBase := file.Base()
+	fileEnd := fileBase + file.Size()
+	if !start.IsValid() {
+		panic("invalid start token.Pos")
+	}
+	if !end.IsValid() {
+		panic("invalid end token.Pos")
+	}
+	if int(start) < fileBase || int(start) > fileEnd {
+		panic(fmt.Sprintf("invalid start: %d not in [%d, %d]", start, fileBase, fileEnd))
+	}
+	if int(end) < fileBase || int(end) > fileEnd {
+		panic(fmt.Sprintf("invalid end: %d not in [%d, %d]", end, fileBase, fileEnd))
+	}
+	if start > end {
+		panic("invalid start: greater than end")
+	}
+	return Range{
+		TokFile: file,
+		Start:   start,
+		End:     end,
 	}
 }

go/packages/packagestest/expect_test.go

@@ -10,7 +10,6 @@ import (
 
 	"golang.org/x/tools/go/expect"
 	"golang.org/x/tools/go/packages/packagestest"
-	"golang.org/x/tools/internal/span"
 )
 
 func TestExpect(t *testing.T) {
@@ -43,7 +42,7 @@ func TestExpect(t *testing.T) {
 			}
 		},
 		"directNote": func(n *expect.Note) {},
-		"range": func(r span.Range) {
+		"range": func(r packagestest.Range) {
 			if r.Start == token.NoPos || r.Start == 0 {
 				t.Errorf("Range had no valid starting position")
 			}

go/packages/packagestest/export.go

@@ -79,7 +79,6 @@ import (
 
 	"golang.org/x/tools/go/expect"
 	"golang.org/x/tools/go/packages"
-	"golang.org/x/tools/internal/span"
 	"golang.org/x/tools/internal/testenv"
 )
 
@@ -129,7 +128,7 @@ type Exported struct {
 	primary  string                       // the first non GOROOT module that was exported
 	written  map[string]map[string]string // the full set of exported files
 	notes    []*expect.Note               // The list of expectations extracted from go source files
-	markers  map[string]span.Range        // The set of markers extracted from go source files
+	markers  map[string]Range             // The set of markers extracted from go source files
 }
 
 // Exporter implementations are responsible for converting from the generic description of some

internal/span/span.go

@@ -275,3 +275,13 @@ func (p *point) updateOffset(tf *token.File) error {
 	p.Offset = offset
 	return nil
 }
+
+// SetRange implements packagestest.rangeSetter, allowing
+// gopls' test suites to use Spans instead of Range in parameters.
+func (span *Span) SetRange(file *token.File, start, end token.Pos) {
+	point := func(pos token.Pos) Point {
+		posn := file.Position(pos)
+		return NewPoint(posn.Line, posn.Column, posn.Offset)
+	}
+	*span = New(URIFromPath(file.Name()), point(start), point(end))
+}