From 85e6ad773c681df9702c85baf7808ed1324de4e3 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 28 Dec 2022 10:00:32 -0500 Subject: [PATCH] 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 Reviewed-by: Robert Findley TryBot-Result: Gopher Robot gopls-CI: kokoro --- gopls/internal/lsp/cache/analysis.go | 2 +- gopls/internal/lsp/safetoken/safetoken.go | 41 +++++++++++++++---- .../internal/lsp/safetoken/safetoken_test.go | 32 ++++++++++++++- gopls/internal/lsp/semantic.go | 6 +-- 4 files changed, 65 insertions(+), 16 deletions(-) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index 32fcdf7f758..8185f23bed2 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/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) diff --git a/gopls/internal/lsp/safetoken/safetoken.go b/gopls/internal/lsp/safetoken/safetoken.go index 9f804f7e13d..200935612e0 100644 --- a/gopls/internal/lsp/safetoken/safetoken.go +++ b/gopls/internal/lsp/safetoken/safetoken.go @@ -2,8 +2,9 @@ // 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 ( @@ -11,18 +12,36 @@ import ( "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()) @@ -30,7 +49,11 @@ func Pos(f *token.File, offset int) (token.Pos, error) { 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()) } diff --git a/gopls/internal/lsp/safetoken/safetoken_test.go b/gopls/internal/lsp/safetoken/safetoken_test.go index 00bf7bd9346..452820f679b 100644 --- a/gopls/internal/lsp/safetoken/safetoken_test.go +++ b/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) } diff --git a/gopls/internal/lsp/semantic.go b/gopls/internal/lsp/semantic.go index ea90da17378..59d4fc3222b 100644 --- a/gopls/internal/lsp/semantic.go +++ b/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, "]")