From de3ef4aa45a47161e0965b494d18495e99adb73a Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 23 May 2022 23:01:49 -0400 Subject: [PATCH] internal/lsp/source: remove workaround for newline terminated files Eliminate the need to work around newline terminated files in completion, by storing selection ranges as token.Pos and using an lsppos.TokenMapper derived from the file content, which does not have problems with newline termination. This simplifies the completion logic, and removes the last use of MappedRange.SpanRange, which is an inconisitent API in that it returns positions in the compiled source, rather than edited source. Change-Id: I65232787956325172b48fd42d85cbb598039ee5a Reviewed-on: https://go-review.googlesource.com/c/tools/+/407889 Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot Run-TryBot: Robert Findley gopls-CI: kokoro --- internal/lsp/completion.go | 56 ++++---------------- internal/lsp/source/completion/completion.go | 30 +++++------ internal/lsp/source/completion/definition.go | 9 ++-- internal/lsp/source/completion/package.go | 27 ++++------ internal/lsp/source/util.go | 8 --- 5 files changed, 41 insertions(+), 89 deletions(-) diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go index 5c88ed0e44..06af1bdaec 100644 --- a/internal/lsp/completion.go +++ b/internal/lsp/completion.go @@ -5,19 +5,18 @@ package lsp import ( - "bytes" "context" "fmt" "strings" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/lsp/debug/tag" + "golang.org/x/tools/internal/lsp/lsppos" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/source/completion" "golang.org/x/tools/internal/lsp/template" "golang.org/x/tools/internal/lsp/work" - "golang.org/x/tools/internal/span" ) func (s *Server) completion(ctx context.Context, params *protocol.CompletionParams) (*protocol.CompletionList, error) { @@ -56,55 +55,20 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara Items: []protocol.CompletionItem{}, }, nil } - // We might need to adjust the position to account for the prefix. - rng, err := surrounding.Range() - if err != nil { - return nil, err - } - // internal/span treats end of file as the beginning of the next line, even - // when it's not newline-terminated. We correct for that behaviour here if - // end of file is not newline-terminated. See golang/go#41029. + // Map positions to LSP positions using the original content, rather than + // internal/span, as the latter treats end of file as the beginning of the + // next line, even when it's not newline-terminated. See golang/go#41029 for + // more details. src, err := fh.Read() if err != nil { return nil, err } - numLines := len(bytes.Split(src, []byte("\n"))) - tok := snapshot.FileSet().File(surrounding.Start()) - eof := tok.Pos(tok.Size()) - - // For newline-terminated files, the line count reported by go/token should - // be lower than the actual number of lines we see when splitting by \n. If - // they're the same, the file isn't newline-terminated. - if tok.Size() > 0 && tok.LineCount() == numLines { - // Get the span for the last character in the file-1. This is - // technically incorrect, but will get span to point to the previous - // line. - spn, err := span.NewRange(snapshot.FileSet(), eof-1, eof-1).Span() - if err != nil { - return nil, err - } - m := &protocol.ColumnMapper{ - URI: fh.URI(), - Converter: span.NewContentConverter(fh.URI().Filename(), src), - Content: src, - } - eofRng, err := m.Range(spn) - if err != nil { - return nil, err - } - // Instead of using the computed range, correct for our earlier - // position adjustment by adding 1 to the column, not the line number. - pos := protocol.Position{ - Line: eofRng.Start.Line, - Character: eofRng.Start.Character + 1, - } - if surrounding.Start() >= eof { - rng.Start = pos - } - if surrounding.End() >= eof { - rng.End = pos - } + tf := snapshot.FileSet().File(surrounding.Start()) + mapper := lsppos.NewTokenMapper(src, tf) + rng, err := mapper.Range(surrounding.Start(), surrounding.End()) + if err != nil { + return nil, err } // When using deep completions/fuzzy matching, report results as incomplete so diff --git a/internal/lsp/source/completion/completion.go b/internal/lsp/source/completion/completion.go index 50116c940f..4db3e302c1 100644 --- a/internal/lsp/source/completion/completion.go +++ b/internal/lsp/source/completion/completion.go @@ -29,6 +29,7 @@ import ( "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/snippet" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/typeparams" ) @@ -289,7 +290,7 @@ type completionContext struct { type Selection struct { content string cursor token.Pos - source.MappedRange + rng span.Range } func (p Selection) Content() string { @@ -297,19 +298,19 @@ func (p Selection) Content() string { } func (p Selection) Start() token.Pos { - return p.MappedRange.SpanRange().Start + return p.rng.Start } func (p Selection) End() token.Pos { - return p.MappedRange.SpanRange().End + return p.rng.End } func (p Selection) Prefix() string { - return p.content[:p.cursor-p.SpanRange().Start] + return p.content[:p.cursor-p.rng.Start] } func (p Selection) Suffix() string { - return p.content[p.cursor-p.SpanRange().Start:] + return p.content[p.cursor-p.rng.Start:] } func (c *completer) setSurrounding(ident *ast.Ident) { @@ -324,7 +325,7 @@ func (c *completer) setSurrounding(ident *ast.Ident) { content: ident.Name, cursor: c.pos, // Overwrite the prefix only. - MappedRange: source.NewMappedRange(c.snapshot.FileSet(), c.mapper, ident.Pos(), ident.End()), + rng: span.NewRange(c.snapshot.FileSet(), ident.Pos(), ident.End()), } c.setMatcherFromPrefix(c.surrounding.Prefix()) @@ -344,9 +345,9 @@ func (c *completer) setMatcherFromPrefix(prefix string) { func (c *completer) getSurrounding() *Selection { if c.surrounding == nil { c.surrounding = &Selection{ - content: "", - cursor: c.pos, - MappedRange: source.NewMappedRange(c.snapshot.FileSet(), c.mapper, c.pos, c.pos), + content: "", + cursor: c.pos, + rng: span.NewRange(c.snapshot.FileSet(), c.pos, c.pos), } } return c.surrounding @@ -491,7 +492,7 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan qual := types.RelativeTo(pkg.GetTypes()) objStr = types.ObjectString(obj, qual) } - ans, sel := definition(path, obj, snapshot.FileSet(), pgf.Mapper, fh) + ans, sel := definition(path, obj, snapshot.FileSet(), fh) if ans != nil { sort.Slice(ans, func(i, j int) bool { return ans[i].Score > ans[j].Score @@ -801,9 +802,9 @@ func (c *completer) populateImportCompletions(ctx context.Context, searchImport } c.surrounding = &Selection{ - content: content, - cursor: c.pos, - MappedRange: source.NewMappedRange(c.snapshot.FileSet(), c.mapper, start, end), + content: content, + cursor: c.pos, + rng: span.NewRange(c.snapshot.FileSet(), start, end), } seenImports := make(map[string]struct{}) @@ -1023,8 +1024,7 @@ func (c *completer) setSurroundingForComment(comments *ast.CommentGroup) { c.surrounding = &Selection{ content: cursorComment.Text[start:end], cursor: c.pos, - MappedRange: source.NewMappedRange(c.snapshot.FileSet(), c.mapper, - token.Pos(int(cursorComment.Slash)+start), token.Pos(int(cursorComment.Slash)+end)), + rng: span.NewRange(c.snapshot.FileSet(), token.Pos(int(cursorComment.Slash)+start), token.Pos(int(cursorComment.Slash)+end)), } c.setMatcherFromPrefix(c.surrounding.Prefix()) } diff --git a/internal/lsp/source/completion/definition.go b/internal/lsp/source/completion/definition.go index 17b251cb06..44d5a33b2f 100644 --- a/internal/lsp/source/completion/definition.go +++ b/internal/lsp/source/completion/definition.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/snippet" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/span" ) // some definitions can be completed @@ -22,7 +23,7 @@ import ( // BenchmarkFoo(b *testing.B), FuzzFoo(f *testing.F) // path[0] is known to be *ast.Ident -func definition(path []ast.Node, obj types.Object, fset *token.FileSet, mapper *protocol.ColumnMapper, fh source.FileHandle) ([]CompletionItem, *Selection) { +func definition(path []ast.Node, obj types.Object, fset *token.FileSet, fh source.FileHandle) ([]CompletionItem, *Selection) { if _, ok := obj.(*types.Func); !ok { return nil, nil // not a function at all } @@ -37,9 +38,9 @@ func definition(path []ast.Node, obj types.Object, fset *token.FileSet, mapper * } pos := path[0].Pos() sel := &Selection{ - content: "", - cursor: pos, - MappedRange: source.NewMappedRange(fset, mapper, pos, pos), + content: "", + cursor: pos, + rng: span.NewRange(fset, pos, pos), } var ans []CompletionItem diff --git a/internal/lsp/source/completion/package.go b/internal/lsp/source/completion/package.go index 7a05153e80..3752384095 100644 --- a/internal/lsp/source/completion/package.go +++ b/internal/lsp/source/completion/package.go @@ -45,7 +45,7 @@ func packageClauseCompletions(ctx context.Context, snapshot source.Snapshot, fh return nil, nil, err } - surrounding, err := packageCompletionSurrounding(ctx, snapshot.FileSet(), pgf, rng.Start) + surrounding, err := packageCompletionSurrounding(snapshot.FileSet(), pgf, rng.Start) if err != nil { return nil, nil, fmt.Errorf("invalid position for package completion: %w", err) } @@ -72,7 +72,7 @@ func packageClauseCompletions(ctx context.Context, snapshot source.Snapshot, fh // packageCompletionSurrounding returns surrounding for package completion if a // package completions can be suggested at a given position. A valid location // for package completion is above any declarations or import statements. -func packageCompletionSurrounding(ctx context.Context, fset *token.FileSet, pgf *source.ParsedGoFile, pos token.Pos) (*Selection, error) { +func packageCompletionSurrounding(fset *token.FileSet, pgf *source.ParsedGoFile, pos token.Pos) (*Selection, error) { // If the file lacks a package declaration, the parser will return an empty // AST. As a work-around, try to parse an expression from the file contents. filename := pgf.URI.Filename() @@ -96,11 +96,6 @@ func packageCompletionSurrounding(ctx context.Context, fset *token.FileSet, pgf return nil, fmt.Errorf("cursor out of bounds") } cursor := tok.Pos(offset) - m := &protocol.ColumnMapper{ - URI: pgf.URI, - Content: pgf.Src, - Converter: span.NewContentConverter(filename, pgf.Src), - } // If we were able to parse out an identifier as the first expression from // the file, it may be the beginning of a package declaration ("pack "). @@ -111,9 +106,9 @@ func packageCompletionSurrounding(ctx context.Context, fset *token.FileSet, pgf return nil, fmt.Errorf("cursor in non-matching ident") } return &Selection{ - content: name.Name, - cursor: cursor, - MappedRange: source.NewMappedRange(fset, m, name.Pos(), name.End()), + content: name.Name, + cursor: cursor, + rng: span.NewRange(fset, name.Pos(), name.End()), }, nil } } @@ -148,9 +143,9 @@ func packageCompletionSurrounding(ctx context.Context, fset *token.FileSet, pgf // otherwise fallback to the general case. if cursor >= start && cursor <= end { return &Selection{ - content: content, - cursor: cursor, - MappedRange: source.NewMappedRange(fset, m, start, end), + content: content, + cursor: cursor, + rng: span.NewRange(fset, start, end), }, nil } } @@ -175,9 +170,9 @@ func packageCompletionSurrounding(ctx context.Context, fset *token.FileSet, pgf } return &Selection{ - content: "", - cursor: cursor, - MappedRange: source.NewMappedRange(fset, m, start, end), + content: "", + cursor: cursor, + rng: span.NewRange(fset, start, end), }, nil } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index a9e32e3b5b..e391044fa1 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -84,14 +84,6 @@ func (s MappedRange) Span() (span.Span, error) { return span.FileSpan(s.spanRange.TokFile, s.m.Converter, s.spanRange.Start, s.spanRange.End) } -// SpanRange returns a span.Range in the compiled source. -// -// See the documentation of NewMappedRange for information on edited vs -// compiled source. -func (s MappedRange) SpanRange() span.Range { - return s.spanRange -} - // URI returns the URI of the edited file. // // See the documentation of NewMappedRange for information on edited vs