gopls/internal/lsp/safetoken: fix bug in Offset at EOF

During parser error recovery, it may synthesize tokens such as RBRACE
at EOF, causing the End position of the incomplete syntax nodes to
be computed as Rbrace+len("}"), which is out of bounds, and would
cause token.File.Offset to panic, or safetoken.Offset to return an
error.

This change is a workaround in gopls so that such End positions are
considered valid, and are mapped to the end of the file.

Also
- a regression test.
- remove safetoken.InRange, to avoid ambiguity. It was used in
  only one place (and dubiously even there).

Fixes golang/go#57484
Updates golang/go#57490

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

gopls/internal/lsp/cache/analysis.go

@@ -1061,7 +1061,7 @@ func (act *action) exec() (interface{}, *actionSummary, error) {
 
 			diagnostic, err := toGobDiagnostic(posToLocation, d)
 			if err != nil {
-				bug.Reportf("internal error converting diagnostic from analyzer %s: %v", analyzer.Name, err)
+				bug.Reportf("internal error converting diagnostic from analyzer %q: %v", analyzer.Name, err)
 				return
 			}
 			diagnostics = append(diagnostics, diagnostic)

gopls/internal/lsp/safetoken/safetoken.go

@@ -2,35 +2,58 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// Package safetoken provides wrappers around methods in go/token, that return
-// errors rather than panicking.
+// 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.
 package safetoken
 
 import (
 	"fmt"
 	"go/token"
 )
 
-// Offset returns f.Offset(pos), but first checks that the pos is in range
-// for the given file.
+// Offset returns f.Offset(pos), but first checks that the file
+// contains the pos.
+//
+// The definition of "contains" here differs from that of token.File
+// in order to work around a bug in the parser (issue #57490): during
+// error recovery, the parser may create syntax nodes whose computed
+// End position is 1 byte beyond EOF, which would cause
+// 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) {
+	if !inRange(f, pos) {
+		// Accept a Pos that is 1 byte beyond EOF,
+		// and map it to the EOF offset.
+		// (Workaround for #57490.)
+		if int(pos) == f.Base()+f.Size()+1 {
+			return f.Size(), nil
+		}
+
 		return -1, fmt.Errorf("pos %d is not in range [%d:%d] of file %s",
 			pos, f.Base(), f.Base()+f.Size(), f.Name())
 	}
 	return int(pos) - f.Base(), nil
 }
 
-// Pos returns f.Pos(offset), but first checks that the offset is valid for
-// the given file.
+// Pos returns f.Pos(offset), but first checks that the offset is
+// non-negative and not larger than the size of the file.
 func Pos(f *token.File, offset int) (token.Pos, error) {
 	if !(0 <= offset && offset <= f.Size()) {
 		return token.NoPos, fmt.Errorf("offset %d is not in range for file %s of size %d", offset, f.Name(), f.Size())
 	}
 	return token.Pos(f.Base() + offset), nil
 }
 
-// InRange reports whether file f contains position pos.
-func InRange(f *token.File, pos token.Pos) bool {
+// inRange reports whether file f contains position pos,
+// according to the invariants of token.File.
+//
+// This function is not public because of the ambiguity it would
+// create w.r.t. the definition of "contains". Use Offset instead.
+func inRange(f *token.File, pos token.Pos) bool {
 	return token.Pos(f.Base()) <= pos && pos <= token.Pos(f.Base()+f.Size())
 }

gopls/internal/lsp/safetoken/safetoken_test.go

@@ -5,26 +5,54 @@
 package safetoken_test
 
 import (
+	"go/parser"
 	"go/token"
 	"go/types"
 	"testing"
 
 	"golang.org/x/tools/go/packages"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/internal/testenv"
 )
 
+func TestWorkaroundIssue57490(t *testing.T) {
+	// During error recovery the parser synthesizes various close
+	// tokens at EOF, causing the End position of incomplete
+	// 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)
+	tf := fset.File(file.Pos())
+	if false {
+		tf.Offset(file.End()) // panic: invalid Pos value 36 (should be in [1, 35])
+	}
+
+	// The offset of the EOF position is the file size.
+	offset, err := safetoken.Offset(tf, file.End()-1)
+	if err != nil || offset != tf.Size() {
+		t.Errorf("Offset(EOF) = (%d, %v), want token.File.Size %d", offset, err, tf.Size())
+	}
+
+	// The offset of the file.End() position, 1 byte beyond EOF,
+	// is also the size of the file.
+	offset, err = safetoken.Offset(tf, file.End())
+	if err != nil || offset != tf.Size() {
+		t.Errorf("Offset(ast.File.End()) = (%d, %v), want token.File.Size %d", offset, err, tf.Size())
+	}
+}
+
 // 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 TestTokenOffset(t *testing.T) {
+func TestGoplsSourceDoesNotCallTokenFileOffset(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/internal/lsp/...", "golang.org/x/tools/gopls/...")
+	}, "go/token", "golang.org/x/tools/gopls/...")
 	if err != nil {
 		t.Fatal(err)
 	}

gopls/internal/lsp/semantic.go

@@ -244,14 +244,12 @@ func (e *encoded) strStack() string {
 	}
 	if len(e.stack) > 0 {
 		loc := e.stack[len(e.stack)-1].Pos()
-		if !safetoken.InRange(e.pgf.Tok, loc) {
+		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 if safetoken.InRange(e.pgf.Tok, loc) {
+		} else {
 			add := e.pgf.Tok.PositionFor(loc, false) // ignore line directives
 			nm := filepath.Base(add.Filename)
 			msg = append(msg, fmt.Sprintf("(%s:%d,col:%d)", nm, add.Line, add.Column))
-		} else {
-			msg = append(msg, fmt.Sprintf("(loc %d out of range)", loc))
 		}
 	}
 	msg = append(msg, "]")