From 81e741e32fac43eebb64438e6440cbf3dad7bbb5 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 28 Dec 2022 10:49:34 -0500 Subject: [PATCH] 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 Reviewed-by: Robert Findley TryBot-Result: Gopher Robot Auto-Submit: Alan Donovan gopls-CI: kokoro --- gopls/doc/generate.go | 3 +- .../lsp/analysis/fillstruct/fillstruct.go | 5 +- .../lsp/analysis/undeclaredname/undeclared.go | 7 +- gopls/internal/lsp/cache/errors.go | 3 +- gopls/internal/lsp/safetoken/safetoken.go | 60 +++++++++++-- .../internal/lsp/safetoken/safetoken_test.go | 84 ++++++++++++------- gopls/internal/lsp/semantic.go | 12 +-- .../internal/lsp/source/completion/format.go | 3 +- .../internal/lsp/source/completion/package.go | 2 +- .../internal/lsp/source/completion/snippet.go | 3 +- gopls/internal/lsp/source/extract.go | 4 +- gopls/internal/lsp/source/format.go | 4 +- gopls/internal/lsp/source/implementation.go | 4 +- gopls/internal/lsp/source/references.go | 3 +- gopls/internal/lsp/source/stub.go | 4 +- gopls/internal/lsp/source/util.go | 3 +- gopls/internal/lsp/tests/tests.go | 5 +- gopls/internal/span/span.go | 4 +- gopls/internal/span/token.go | 3 +- 19 files changed, 152 insertions(+), 64 deletions(-) diff --git a/gopls/doc/generate.go b/gopls/doc/generate.go index 8c0bbd3a701..d674bfce489 100644 --- a/gopls/doc/generate.go +++ b/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 } } diff --git a/gopls/internal/lsp/analysis/fillstruct/fillstruct.go b/gopls/internal/lsp/analysis/fillstruct/fillstruct.go index 7c1e0442ded..00857a0953e 100644 --- a/gopls/internal/lsp/analysis/fillstruct/fillstruct.go +++ b/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. diff --git a/gopls/internal/lsp/analysis/undeclaredname/undeclared.go b/gopls/internal/lsp/analysis/undeclaredname/undeclared.go index 646f4fa4786..3e42f08d55b 100644 --- a/gopls/internal/lsp/analysis/undeclaredname/undeclared.go +++ b/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(). diff --git a/gopls/internal/lsp/cache/errors.go b/gopls/internal/lsp/cache/errors.go index 7e685f3df4c..7ca4f078eff 100644 --- a/gopls/internal/lsp/cache/errors.go +++ b/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) } diff --git a/gopls/internal/lsp/safetoken/safetoken.go b/gopls/internal/lsp/safetoken/safetoken.go index 200935612e0..fa9c67515f8 100644 --- a/gopls/internal/lsp/safetoken/safetoken.go +++ b/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 +} diff --git a/gopls/internal/lsp/safetoken/safetoken_test.go b/gopls/internal/lsp/safetoken/safetoken_test.go index 452820f679b..afd569472ac 100644 --- a/gopls/internal/lsp/safetoken/safetoken_test.go +++ b/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)) } } } diff --git a/gopls/internal/lsp/semantic.go b/gopls/internal/lsp/semantic.go index 7368b59b613..4117eb70b82 100644 --- a/gopls/internal/lsp/semantic.go +++ b/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 } diff --git a/gopls/internal/lsp/source/completion/format.go b/gopls/internal/lsp/source/completion/format.go index 0f8189a7bff..bb3ccd72d69 100644 --- a/gopls/internal/lsp/source/completion/format.go +++ b/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. diff --git a/gopls/internal/lsp/source/completion/package.go b/gopls/internal/lsp/source/completion/package.go index 65f86ca249d..70d98dfc839 100644 --- a/gopls/internal/lsp/source/completion/package.go +++ b/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 diff --git a/gopls/internal/lsp/source/completion/snippet.go b/gopls/internal/lsp/source/completion/snippet.go index 811acf63b51..53577232939 100644 --- a/gopls/internal/lsp/source/completion/snippet.go +++ b/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(",") } } diff --git a/gopls/internal/lsp/source/extract.go b/gopls/internal/lsp/source/extract.go index b0ad550dfe8..0ac18e10390 100644 --- a/gopls/internal/lsp/source/extract.go +++ b/gopls/internal/lsp/source/extract.go @@ -28,7 +28,7 @@ func extractVariable(fset *token.FileSet, rng span.Range, src []byte, file *ast. tokFile := fset.File(file.Pos()) expr, path, ok, err := CanExtractVariable(rng, file) if !ok { - return nil, fmt.Errorf("extractVariable: cannot extract %s: %v", fset.PositionFor(rng.Start, false), err) + return nil, fmt.Errorf("extractVariable: cannot extract %s: %v", safetoken.StartPosition(fset, rng.Start), err) } // Create new AST node for extracted code. @@ -224,7 +224,7 @@ func extractFunctionMethod(fset *token.FileSet, rng span.Range, src []byte, file p, ok, methodOk, err := CanExtractFunction(tok, rng, src, file) if (!ok && !isMethod) || (!methodOk && isMethod) { return nil, fmt.Errorf("%s: cannot extract %s: %v", errorPrefix, - fset.PositionFor(rng.Start, false), err) + safetoken.StartPosition(fset, rng.Start), err) } tok, path, rng, outer, start := p.tok, p.path, p.rng, p.outer, p.start fileScope := info.Scopes[file] diff --git a/gopls/internal/lsp/source/format.go b/gopls/internal/lsp/source/format.go index 7aca5e7d06b..6662137b526 100644 --- a/gopls/internal/lsp/source/format.go +++ b/gopls/internal/lsp/source/format.go @@ -265,8 +265,8 @@ func importPrefix(src []byte) (string, error) { if end, err := safetoken.Offset(tok, c.End()); err != nil { return "", err } else if end > importEnd { - startLine := tok.PositionFor(c.Pos(), false).Line - endLine := tok.PositionFor(c.End(), false).Line + startLine := safetoken.Position(tok, c.Pos()).Line + endLine := safetoken.Position(tok, c.End()).Line // Work around golang/go#41197 by checking if the comment might // contain "\r", and if so, find the actual end position of the diff --git a/gopls/internal/lsp/source/implementation.go b/gopls/internal/lsp/source/implementation.go index 3933fdf5a9b..cd2fc860a6e 100644 --- a/gopls/internal/lsp/source/implementation.go +++ b/gopls/internal/lsp/source/implementation.go @@ -153,7 +153,7 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol. candObj = sel.Obj() } - pos := s.FileSet().PositionFor(candObj.Pos(), false) + pos := safetoken.StartPosition(s.FileSet(), candObj.Pos()) if candObj == queryMethod || seen[pos] { continue } @@ -166,7 +166,7 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol. var posn token.Position if pkg != nil { - posn = pkg.FileSet().PositionFor(candObj.Pos(), false) + posn = safetoken.StartPosition(pkg.FileSet(), candObj.Pos()) } if seen[posn] { continue diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go index f51f7f1f0da..a1dcc5417a9 100644 --- a/gopls/internal/lsp/source/references.go +++ b/gopls/internal/lsp/source/references.go @@ -15,6 +15,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/span" "golang.org/x/tools/internal/bug" "golang.org/x/tools/internal/event" @@ -158,7 +159,7 @@ func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, i } // Inv: qos[0].pkg != nil, since Pos is valid. // Inv: qos[*].pkg != nil, since all qos are logically the same declaration. - filename := qos[0].pkg.FileSet().PositionFor(pos, false).Filename + filename := safetoken.StartPosition(qos[0].pkg.FileSet(), pos).Filename pgf, err := qos[0].pkg.File(span.URIFromPath(filename)) if err != nil { return nil, err diff --git a/gopls/internal/lsp/source/stub.go b/gopls/internal/lsp/source/stub.go index 18829fb60e0..2568bd07e20 100644 --- a/gopls/internal/lsp/source/stub.go +++ b/gopls/internal/lsp/source/stub.go @@ -47,7 +47,7 @@ func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh VersionedFi } // Parse the file defining the concrete type. - concreteFilename := snapshot.FileSet().PositionFor(si.Concrete.Obj().Pos(), false).Filename + concreteFilename := safetoken.StartPosition(snapshot.FileSet(), si.Concrete.Obj().Pos()).Filename concreteFH, err := snapshot.GetFile(ctx, span.URIFromPath(concreteFilename)) if err != nil { return nil, nil, err @@ -261,7 +261,7 @@ func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.Method return nil, fmt.Errorf("expected %v to be an interface but got %T", iface, ifaceObj.Type().Underlying()) } // Parse the imports from the file that declares the interface. - ifaceFilename := snapshot.FileSet().PositionFor(ifaceObj.Pos(), false).Filename + ifaceFilename := safetoken.StartPosition(snapshot.FileSet(), ifaceObj.Pos()).Filename ifaceFH, err := snapshot.GetFile(ctx, span.URIFromPath(ifaceFilename)) if err != nil { return nil, err diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go index 978e2c7209f..face4c99ac2 100644 --- a/gopls/internal/lsp/source/util.go +++ b/gopls/internal/lsp/source/util.go @@ -18,6 +18,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/span" "golang.org/x/tools/internal/bug" "golang.org/x/tools/internal/typeparams" @@ -94,7 +95,7 @@ func IsGenerated(ctx context.Context, snapshot Snapshot, uri span.URI) bool { for _, comment := range commentGroup.List { if matched := generatedRx.MatchString(comment.Text); matched { // Check if comment is at the beginning of the line in source. - if pgf.Tok.PositionFor(comment.Slash, false).Column == 1 { + if safetoken.Position(pgf.Tok, comment.Slash).Column == 1 { return true } } diff --git a/gopls/internal/lsp/tests/tests.go b/gopls/internal/lsp/tests/tests.go index 8e977b08ccf..b7a6b047aef 100644 --- a/gopls/internal/lsp/tests/tests.go +++ b/gopls/internal/lsp/tests/tests.go @@ -29,6 +29,7 @@ import ( "golang.org/x/tools/go/packages/packagestest" "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/lsp/source/completion" "golang.org/x/tools/gopls/internal/lsp/tests/compare" @@ -1368,7 +1369,7 @@ func (data *Data) collectWorkspaceSymbols(typ WorkspaceSymbolsTestType) func(*ex if data.WorkspaceSymbols[typ] == nil { data.WorkspaceSymbols[typ] = make(map[span.URI][]string) } - pos := data.Exported.ExpectFileSet.PositionFor(note.Pos, false) + pos := safetoken.StartPosition(data.Exported.ExpectFileSet, note.Pos) uri := span.URIFromPath(pos.Filename) data.WorkspaceSymbols[typ][uri] = append(data.WorkspaceSymbols[typ][uri], query) } @@ -1398,7 +1399,7 @@ func (data *Data) collectCompletionSnippets(spn span.Span, item token.Pos, plain } func (data *Data) collectLinks(spn span.Span, link string, note *expect.Note, fset *token.FileSet) { - position := fset.PositionFor(note.Pos, false) // ignore line directives + position := safetoken.StartPosition(fset, note.Pos) uri := spn.URI() data.Links[uri] = append(data.Links[uri], Link{ Src: spn, diff --git a/gopls/internal/span/span.go b/gopls/internal/span/span.go index 00c7b3f2a43..0c24a2deeed 100644 --- a/gopls/internal/span/span.go +++ b/gopls/internal/span/span.go @@ -13,6 +13,8 @@ import ( "path" "sort" "strings" + + "golang.org/x/tools/gopls/internal/lsp/safetoken" ) // Span represents a source code range in standardized form. @@ -294,7 +296,7 @@ func (p *point) updateOffset(tf *token.File) error { // 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) + posn := safetoken.Position(file, pos) return NewPoint(posn.Line, posn.Column, posn.Offset) } *span = New(URIFromPath(file.Name()), point(start), point(end)) diff --git a/gopls/internal/span/token.go b/gopls/internal/span/token.go index cdc747f330e..ca78d67800f 100644 --- a/gopls/internal/span/token.go +++ b/gopls/internal/span/token.go @@ -8,6 +8,7 @@ import ( "fmt" "go/token" + "golang.org/x/tools/gopls/internal/lsp/safetoken" "golang.org/x/tools/internal/bug" ) @@ -119,7 +120,7 @@ func positionFromOffset(tf *token.File, offset int) (string, int, int, error) { return "", 0, 0, fmt.Errorf("offset %d is beyond EOF (%d) in file %s", offset, tf.Size(), tf.Name()) } pos := tf.Pos(offset) - p := tf.PositionFor(pos, false) // ignore line directives + p := safetoken.Position(tf, pos) // TODO(golang/go#41029): Consider returning line, column instead of line+1, 1 if // the file's last character is not a newline. if offset == tf.Size() {