gopls/internal/lsp/safetoken: funnel more calls through this package

This change expands the dominion of safetoken to include calls to
token.File{,Set}.Position{,For}, since all need workarounds similar
to that in Offset. As a side benefit, we now have a centralized place
to implement the workaround for other bugs such as golang/go#41029,
the newline at EOF problem).

Unfortunately the former callers of FileSet.Position must stipulate
whether the Pos is a start or an end, as the same value may denote
the position 1 beyond the end of one file, or the start of the
following file in the file set. Hence the two different functions,
{Start,End}Position.

The static check has been expanded, as have the tests.

Of course, this approach is not foolproof: gopls has many dependencies
that call methods of File and FileSet directly.

Updates golang/go#41029
Updates golang/go#57484
Updates golang/go#57490

Change-Id: Ia727e3f55ca2703dd17ff2cac05e786793ca38eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459736
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>

Author: adonovan

gopls/doc/generate.go

@@ -36,6 +36,7 @@ import (
 	"golang.org/x/tools/gopls/internal/lsp/command"
 	"golang.org/x/tools/gopls/internal/lsp/command/commandmeta"
 	"golang.org/x/tools/gopls/internal/lsp/mod"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 )
 
@@ -555,7 +556,7 @@ func upperFirst(x string) string {
 func fileForPos(pkg *packages.Package, pos token.Pos) (*ast.File, error) {
 	fset := pkg.Fset
 	for _, f := range pkg.Syntax {
-		if fset.PositionFor(f.Pos(), false).Filename == fset.PositionFor(pos, false).Filename {
+		if safetoken.StartPosition(fset, f.Pos()).Filename == safetoken.StartPosition(fset, pos).Filename {
 			return f, nil
 		}
 	}

gopls/internal/lsp/analysis/fillstruct/fillstruct.go

@@ -26,6 +26,7 @@ import (
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/span"
 	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/fuzzy"
@@ -271,8 +272,8 @@ func SuggestedFix(fset *token.FileSet, rng span.Range, content []byte, file *ast
 
 	// Find the line on which the composite literal is declared.
 	split := bytes.Split(content, []byte("\n"))
-	lineNumber := fset.PositionFor(expr.Lbrace, false).Line // ignore line directives
-	firstLine := split[lineNumber-1]                        // lines are 1-indexed
+	lineNumber := safetoken.StartPosition(fset, expr.Lbrace).Line
+	firstLine := split[lineNumber-1] // lines are 1-indexed
 
 	// Trim the whitespace from the left of the line, and use the index
 	// to get the amount of whitespace on the left.

gopls/internal/lsp/analysis/undeclaredname/undeclared.go

@@ -18,6 +18,7 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/ast/astutil"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/span"
 	"golang.org/x/tools/internal/analysisinternal"
 )
@@ -112,8 +113,8 @@ func runForError(pass *analysis.Pass, err types.Error) {
 	if tok == nil {
 		return
 	}
-	offset := pass.Fset.PositionFor(err.Pos, false).Offset
-	end := tok.Pos(offset + len(name))
+	offset := safetoken.StartPosition(pass.Fset, err.Pos).Offset
+	end := tok.Pos(offset + len(name)) // TODO(adonovan): dubious! err.Pos + len(name)??
 	pass.Report(analysis.Diagnostic{
 		Pos:     err.Pos,
 		End:     end,
@@ -146,7 +147,7 @@ func SuggestedFix(fset *token.FileSet, rng span.Range, content []byte, file *ast
 		return nil, fmt.Errorf("could not locate insertion point")
 	}
 
-	insertBefore := fset.PositionFor(insertBeforeStmt.Pos(), false).Offset // ignore line directives
+	insertBefore := safetoken.StartPosition(fset, insertBeforeStmt.Pos()).Offset
 
 	// Get the indent to add on the line after the new statement.
 	// Since this will have a parse error, we can not use format.Source().

gopls/internal/lsp/cache/errors.go

@@ -20,6 +20,7 @@ import (
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/gopls/internal/lsp/command"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/gopls/internal/span"
 	"golang.org/x/tools/internal/analysisinternal"
@@ -315,7 +316,7 @@ func typeErrorData(pkg *pkg, terr types.Error) (typesinternal.ErrorCode, span.Sp
 	if fset != terr.Fset {
 		return 0, span.Span{}, bug.Errorf("wrong FileSet for type error")
 	}
-	posn := fset.PositionFor(start, false) // ignore line directives
+	posn := safetoken.StartPosition(fset, start)
 	if !posn.IsValid() {
 		return 0, span.Span{}, fmt.Errorf("position %d of type error %q (code %q) not found in FileSet", start, start, terr)
 	}

gopls/internal/lsp/safetoken/safetoken.go

@@ -3,8 +3,12 @@
 // license that can be found in the LICENSE file.
 
 // Package safetoken provides wrappers around methods in go/token,
-// that return errors rather than panicking. It also provides a
-// central place for workarounds in the underlying packages.
+// that return errors rather than panicking.
+//
+// It also provides a central place for workarounds in the underlying
+// packages. The use of this package's functions instead of methods of
+// token.File (such as Offset, Position, and PositionFor) is mandatory
+// throughout the gopls codebase and enforced by a static check.
 package safetoken
 
 import (
@@ -22,9 +26,6 @@ import (
 // token.File.Offset to panic. The workaround is that this function
 // accepts a Pos that is exactly 1 byte beyond EOF and maps it to the
 // EOF offset.
-//
-// The use of this function instead of (*token.File).Offset is
-// mandatory in the gopls codebase; this is enforced by static check.
 func Offset(f *token.File, pos token.Pos) (int, error) {
 	if !inRange(f, pos) {
 		// Accept a Pos that is 1 byte beyond EOF,
@@ -57,3 +58,52 @@ func Pos(f *token.File, offset int) (token.Pos, error) {
 func inRange(f *token.File, pos token.Pos) bool {
 	return token.Pos(f.Base()) <= pos && pos <= token.Pos(f.Base()+f.Size())
 }
+
+// Position returns the Position for the pos value in the given file.
+//
+// p must be NoPos, a valid Pos in the range of f, or exactly 1 byte
+// beyond the end of f. (See [Offset] for explanation.)
+// Any other value causes a panic.
+//
+// Line directives (//line comments) are ignored.
+func Position(f *token.File, pos token.Pos) token.Position {
+	// Work around issue #57490.
+	if int(pos) == f.Base()+f.Size()+1 {
+		pos--
+	}
+
+	// TODO(adonovan): centralize the workaround for
+	// golang/go#41029 (newline at EOF) here too.
+
+	return f.PositionFor(pos, false)
+}
+
+// StartPosition converts a start Pos in the FileSet into a Position.
+//
+// Call this function only if start represents the start of a token or
+// parse tree, such as the result of Node.Pos().  If start is the end of
+// an interval, such as Node.End(), call EndPosition instead, as it
+// may need the correction described at [Position].
+func StartPosition(fset *token.FileSet, start token.Pos) (_ token.Position) {
+	if f := fset.File(start); f != nil {
+		return Position(f, start)
+	}
+	return
+}
+
+// EndPosition converts an end Pos in the FileSet into a Position.
+//
+// Call this function only if pos represents the end of
+// a non-empty interval, such as the result of Node.End().
+func EndPosition(fset *token.FileSet, end token.Pos) (_ token.Position) {
+	if f := fset.File(end); f != nil && int(end) > f.Base() {
+		return Position(f, end)
+	}
+
+	// Work around issue #57490.
+	if f := fset.File(end - 1); f != nil {
+		return Position(f, end)
+	}
+
+	return
+}

gopls/internal/lsp/safetoken/safetoken_test.go

@@ -5,9 +5,11 @@
 package safetoken_test
 
 import (
+	"fmt"
 	"go/parser"
 	"go/token"
 	"go/types"
+	"os"
 	"testing"
 
 	"golang.org/x/tools/go/packages"
@@ -21,10 +23,20 @@ func TestWorkaroundIssue57490(t *testing.T) {
 	// syntax nodes, computed as Rbrace+len("}"), to be beyond EOF.
 	src := `package p; func f() { var x struct`
 	fset := token.NewFileSet()
-	file, _ := parser.ParseFile(fset, "", src, 0)
+	file, _ := parser.ParseFile(fset, "a.go", src, 0)
 	tf := fset.File(file.Pos())
+
+	// Add another file to the FileSet.
+	file2, _ := parser.ParseFile(fset, "b.go", "package q", 0)
+
+	// This is the ambiguity of #57490...
+	if file.End() != file2.Pos() {
+		t.Errorf("file.End() %d != %d file2.Pos()", file.End(), file2.Pos())
+	}
+	// ...which causes these statements to panic.
 	if false {
-		tf.Offset(file.End()) // panic: invalid Pos value 36 (should be in [1, 35])
+		tf.Offset(file.End())   // panic: invalid Pos value 36 (should be in [1, 35])
+		tf.Position(file.End()) // panic: invalid Pos value 36 (should be in [1, 35])
 	}
 
 	// The offset of the EOF position is the file size.
@@ -39,57 +51,71 @@ func TestWorkaroundIssue57490(t *testing.T) {
 	if err != nil || offset != tf.Size() {
 		t.Errorf("Offset(ast.File.End()) = (%d, %v), want token.File.Size %d", offset, err, tf.Size())
 	}
+
+	if got, want := safetoken.Position(tf, file.End()).String(), "a.go:1:35"; got != want {
+		t.Errorf("Position(ast.File.End()) = %s, want %s", got, want)
+	}
+
+	if got, want := safetoken.EndPosition(fset, file.End()).String(), "a.go:1:35"; got != want {
+		t.Errorf("EndPosition(ast.File.End()) = %s, want %s", got, want)
+	}
+
+	// Note that calling StartPosition on an end may yield the wrong file:
+	if got, want := safetoken.StartPosition(fset, file.End()).String(), "b.go:1:1"; got != want {
+		t.Errorf("StartPosition(ast.File.End()) = %s, want %s", got, want)
+	}
 }
 
-// This test reports any unexpected uses of (*go/token.File).Offset within
-// the gopls codebase to ensure that we don't check in more code that is prone
-// to panicking. All calls to (*go/token.File).Offset should be replaced with
-// calls to safetoken.Offset.
-func TestGoplsSourceDoesNotCallTokenFileOffset(t *testing.T) {
+// To reduce the risk of panic, or bugs for which this package
+// provides a workaround, this test statically reports references to
+// forbidden methods of token.File or FileSet throughout gopls and
+// suggests alternatives.
+func TestGoplsSourceDoesNotCallTokenFileMethods(t *testing.T) {
 	testenv.NeedsGoPackages(t)
 
-	fset := token.NewFileSet()
 	pkgs, err := packages.Load(&packages.Config{
-		Fset: fset,
 		Mode: packages.NeedName | packages.NeedModule | packages.NeedCompiledGoFiles | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedSyntax | packages.NeedImports | packages.NeedDeps,
 	}, "go/token", "golang.org/x/tools/gopls/...")
 	if err != nil {
 		t.Fatal(err)
 	}
-	var tokenPkg, safePkg *packages.Package
+	var tokenPkg *packages.Package
 	for _, pkg := range pkgs {
-		switch pkg.PkgPath {
-		case "go/token":
+		if pkg.PkgPath == "go/token" {
 			tokenPkg = pkg
-		case "golang.org/x/tools/gopls/internal/lsp/safetoken":
-			safePkg = pkg
+			break
 		}
 	}
-
 	if tokenPkg == nil {
 		t.Fatal("missing package go/token")
 	}
-	if safePkg == nil {
-		t.Fatal("missing package golang.org/x/tools/gopls/internal/lsp/safetoken")
-	}
 
-	fileObj := tokenPkg.Types.Scope().Lookup("File")
-	tokenOffset, _, _ := types.LookupFieldOrMethod(fileObj.Type(), true, fileObj.Pkg(), "Offset")
+	File := tokenPkg.Types.Scope().Lookup("File")
+	FileSet := tokenPkg.Types.Scope().Lookup("FileSet")
 
-	safeOffset := safePkg.Types.Scope().Lookup("Offset").(*types.Func)
+	alternative := make(map[types.Object]string)
+	setAlternative := func(recv types.Object, old, new string) {
+		oldMethod, _, _ := types.LookupFieldOrMethod(recv.Type(), true, recv.Pkg(), old)
+		alternative[oldMethod] = new
+	}
+	setAlternative(File, "Offset", "safetoken.Offset")
+	setAlternative(File, "Position", "safetoken.Position")
+	setAlternative(File, "PositionFor", "safetoken.Position")
+	setAlternative(FileSet, "Position", "safetoken.StartPosition or EndPosition")
+	setAlternative(FileSet, "PositionFor", "safetoken.StartPosition or EndPosition")
 
 	for _, pkg := range pkgs {
-		if pkg.PkgPath == "go/token" { // Allow usage from within go/token itself.
-			continue
+		switch pkg.PkgPath {
+		case "go/token", "golang.org/x/tools/gopls/internal/lsp/safetoken":
+			continue // allow calls within these packages
 		}
+
 		for ident, obj := range pkg.TypesInfo.Uses {
-			if obj != tokenOffset {
-				continue
-			}
-			if safeOffset.Pos() <= ident.Pos() && ident.Pos() <= safeOffset.Scope().End() {
-				continue // accepted usage
+			if alt, ok := alternative[obj]; ok {
+				posn := safetoken.StartPosition(pkg.Fset, ident.Pos())
+				fmt.Fprintf(os.Stderr, "%s: forbidden use of %v; use %s instead.\n", posn, obj, alt)
+				t.Fail()
 			}
-			t.Errorf(`%s: Unexpected use of (*go/token.File).Offset. Please use golang.org/x/tools/gopls/internal/lsp/safetoken.Offset instead.`, fset.PositionFor(ident.Pos(), false))
 		}
 	}
 }

gopls/internal/lsp/semantic.go

@@ -243,7 +243,7 @@ func (e *encoded) strStack() string {
 		if _, err := safetoken.Offset(e.pgf.Tok, loc); err != nil {
 			msg = append(msg, fmt.Sprintf("invalid position %v for %s", loc, e.pgf.URI))
 		} else {
-			add := e.pgf.Tok.PositionFor(loc, false) // ignore line directives
+			add := safetoken.Position(e.pgf.Tok, loc)
 			nm := filepath.Base(add.Filename)
 			msg = append(msg, fmt.Sprintf("(%s:%d,col:%d)", nm, add.Line, add.Column))
 		}
@@ -413,7 +413,7 @@ func (e *encoded) inspector(n ast.Node) bool {
 		return true
 	// not going to see these
 	case *ast.File, *ast.Package:
-		e.unexpected(fmt.Sprintf("implement %T %s", x, e.pgf.Tok.PositionFor(x.Pos(), false)))
+		e.unexpected(fmt.Sprintf("implement %T %s", x, safetoken.Position(e.pgf.Tok, x.Pos())))
 	// other things we knowingly ignore
 	case *ast.Comment, *ast.CommentGroup:
 		pop()
@@ -773,7 +773,7 @@ func (e *encoded) definitionFor(x *ast.Ident, def types.Object) (tokenType, []st
 		}
 	}
 	// can't happen
-	msg := fmt.Sprintf("failed to find the decl for %s", e.pgf.Tok.PositionFor(x.Pos(), false))
+	msg := fmt.Sprintf("failed to find the decl for %s", safetoken.Position(e.pgf.Tok, x.Pos()))
 	e.unexpected(msg)
 	return "", []string{""}
 }
@@ -803,8 +803,8 @@ func (e *encoded) multiline(start, end token.Pos, val string, tok tokenType) {
 		}
 		return int(f.LineStart(line+1) - n)
 	}
-	spos := e.fset.PositionFor(start, false)
-	epos := e.fset.PositionFor(end, false)
+	spos := safetoken.StartPosition(e.fset, start)
+	epos := safetoken.EndPosition(e.fset, end)
 	sline := spos.Line
 	eline := epos.Line
 	// first line is from spos.Column to end
@@ -827,7 +827,7 @@ func (e *encoded) findKeyword(keyword string, start, end token.Pos) token.Pos {
 		return start + token.Pos(idx)
 	}
 	//(in unparsable programs: type _ <-<-chan int)
-	e.unexpected(fmt.Sprintf("not found:%s %v", keyword, e.fset.PositionFor(start, false)))
+	e.unexpected(fmt.Sprintf("not found:%s %v", keyword, safetoken.StartPosition(e.fset, start)))
 	return token.NoPos
 }
 

gopls/internal/lsp/source/completion/format.go

@@ -14,6 +14,7 @@ import (
 	"strings"
 
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/lsp/snippet"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/gopls/internal/span"
@@ -227,7 +228,7 @@ Suffixes:
 	if !c.opts.documentation {
 		return item, nil
 	}
-	pos := c.pkg.FileSet().PositionFor(obj.Pos(), false)
+	pos := safetoken.StartPosition(c.pkg.FileSet(), obj.Pos())
 
 	// We ignore errors here, because some types, like "unsafe" or "error",
 	// may not have valid positions that we can use to get documentation.

gopls/internal/lsp/source/completion/package.go

@@ -124,7 +124,7 @@ func packageCompletionSurrounding(fset *token.FileSet, pgf *source.ParsedGoFile,
 	if cursorLine <= 0 || cursorLine > len(lines) {
 		return nil, fmt.Errorf("invalid line number")
 	}
-	if fset.PositionFor(expr.Pos(), false).Line == cursorLine { // ignore line directives
+	if safetoken.StartPosition(fset, expr.Pos()).Line == cursorLine {
 		words := strings.Fields(lines[cursorLine-1])
 		if len(words) > 0 && words[0] == PACKAGE {
 			content := PACKAGE

gopls/internal/lsp/source/completion/snippet.go

@@ -7,6 +7,7 @@ package completion
 import (
 	"go/ast"
 
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/lsp/snippet"
 )
 
@@ -43,7 +44,7 @@ func (c *completer) structFieldSnippet(cand candidate, detail string, snip *snip
 
 	// If the cursor position is on a different line from the literal's opening brace,
 	// we are in a multiline literal. Ignore line directives.
-	if fset.PositionFor(c.pos, false).Line != fset.PositionFor(clInfo.cl.Lbrace, false).Line {
+	if safetoken.StartPosition(fset, c.pos).Line != safetoken.StartPosition(fset, clInfo.cl.Lbrace).Line {
 		snip.WriteText(",")
 	}
 }