close_issue_1
/
tools
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

internal/lsp: add ranges to some diagnostics messages 

Added a View interface to the source package, which allows for reading
of other files (in the same package or in other packages). We were
already reading files in jump to definition (to handle the lack of
column information in export data), but now we can also read files in
diagnostics, which allows us to determine the end of an identifier so
that we can report ranges in diagnostic messages.

Updates golang/go#29150

Change-Id: I7958d860dea8f41f2df88a467b5e2946bba4d1c5
Reviewed-on: https://go-review.googlesource.com/c/154742
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This commit is contained in:
Rebecca Stambler 2018-12-18 15:46:14 -05:00
parent 3571f65a7b
commit f344c7530c
11 changed files with 227 additions and 130 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
go/packages/packagestest/expect.go
View File

@ -121,6 +121,10 @@ type Range struct {
End token.Pos
}
type RangePosition struct {
Start, End token.Position
}
// Mark adds a new marker to the known set.
func (e *Exported) Mark(name string, r Range) {
if e.markers == nil {
@ -178,6 +182,7 @@ var (
posType = reflect.TypeOf(token.Pos(0))
positionType = reflect.TypeOf(token.Position{})
rangeType = reflect.TypeOf(Range{})
rangePositionType = reflect.TypeOf(RangePosition{})
fsetType = reflect.TypeOf((*token.FileSet)(nil))
regexType = reflect.TypeOf((*regexp.Regexp)(nil))
)
@ -234,6 +239,17 @@ func (e *Exported) buildConverter(pt reflect.Type) (converter, error) {
}
return reflect.ValueOf(r), remains, nil
}, nil
case pt == rangePositionType:
return func(n *expect.Note, args []interface{}) (reflect.Value, []interface{}, error) {
r, remains, err := e.rangeConverter(n, args)
if err != nil {
return reflect.Value{}, nil, err
}
return reflect.ValueOf(RangePosition{
Start: e.fset.Position(r.Start),
End: e.fset.Position(r.End),
}), remains, nil
}, nil
case pt == identifierType:
return func(n *expect.Note, args []interface{}) (reflect.Value, []interface{}, error) {
arg := args[0]

6
internal/lsp/cache/view.go vendored
View File

@ -34,9 +34,9 @@ func NewView(rootPath string) *View {
}
}
// GetFile returns a File for the given uri.
// It will always succeed, adding the file to the managed set if needed.
func (v *View) GetFile(uri source.URI) *File {
// GetFile returns a File for the given URI. It will always succeed because it
// adds the file to the managed set if needed.
func (v *View) GetFile(uri source.URI) source.File {
v.mu.Lock()
f := v.getFile(uri)
v.mu.Unlock()

27
internal/lsp/diagnostics.go
View File

@ -15,40 +15,35 @@ import (
func (s *server) CacheAndDiagnose(ctx context.Context, uri protocol.DocumentURI, text string) {
f := s.view.GetFile(source.URI(uri))
if f, ok := f.(*cache.File); ok {
f.SetContent([]byte(text))
}
go func() {
reports, err := source.Diagnostics(ctx, f)
reports, err := source.Diagnostics(ctx, s.view, f)
if err != nil {
return // handle error?
}
for filename, diagnostics := range reports {
uri := source.ToURI(filename)
s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
URI: protocol.DocumentURI(source.ToURI(filename)),
Diagnostics: toProtocolDiagnostics(s.view, diagnostics),
URI: protocol.DocumentURI(uri),
Diagnostics: toProtocolDiagnostics(s.view, uri, diagnostics),
})
}
}()
}
func toProtocolDiagnostics(v *cache.View, diagnostics []source.Diagnostic) []protocol.Diagnostic {
func toProtocolDiagnostics(v *cache.View, uri source.URI, diagnostics []source.Diagnostic) []protocol.Diagnostic {
reports := []protocol.Diagnostic{}
for _, diag := range diagnostics {
f := v.GetFile(source.ToURI(diag.Filename))
f := v.GetFile(uri)
tok, err := f.GetToken()
if err != nil {
continue // handle error?
}
pos := fromTokenPosition(tok, diag.Position)
if !pos.IsValid() {
continue // handle error?
}
reports = append(reports, protocol.Diagnostic{
Message: diag.Message,
Range: toProtocolRange(tok, source.Range{
Start: pos,
End: pos,
}),
Range: toProtocolRange(tok, diag.Range),
Severity: protocol.SeverityError, // all diagnostics have error severity for now
Source: "LSP",
})
@ -59,10 +54,10 @@ func toProtocolDiagnostics(v *cache.View, diagnostics []source.Diagnostic) []pro
func sorted(d []protocol.Diagnostic) {
sort.Slice(d, func(i int, j int) bool {
if d[i].Range.Start.Line == d[j].Range.Start.Line {
if d[i].Range.Start.Character == d[j].Range.End.Character {
if d[i].Range.Start.Character == d[j].Range.Start.Character {
return d[i].Message < d[j].Message
}
return d[i].Range.Start.Character < d[j].Range.End.Character
return d[i].Range.Start.Character < d[j].Range.Start.Character
}
return d[i].Range.Start.Line < d[j].Range.Start.Line
})

157
internal/lsp/lsp_test.go
View File

@ -11,7 +11,6 @@ import (
"go/token"
"os/exec"
"path/filepath"
"reflect"
"strings"
"testing"
@ -36,7 +35,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
// We hardcode the expected number of test cases to ensure that all tests
// are being executed. If a test is added, this number must be changed.
const expectedCompletionsCount = 60
const expectedDiagnosticsCount = 15
const expectedDiagnosticsCount = 13
const expectedFormatCount = 3
const expectedDefinitionsCount = 16
const expectedTypeDefinitionsCount = 2
@ -155,47 +154,94 @@ func (d diagnostics) test(t *testing.T, exported *packagestest.Exported, v *cach
count := 0
for filename, want := range d {
f := v.GetFile(source.ToURI(filename))
sourceDiagnostics, err := source.Diagnostics(context.Background(), f)
sourceDiagnostics, err := source.Diagnostics(context.Background(), v, f)
if err != nil {
t.Fatal(err)
}
got := toProtocolDiagnostics(v, sourceDiagnostics[filename])
got := toProtocolDiagnostics(v, source.ToURI(filename), sourceDiagnostics[filename])
sorted(got)
if equal := reflect.DeepEqual(want, got); !equal {
t.Error(diffD(filename, want, got))
if diff := diffDiagnostics(filename, want, got); diff != "" {
t.Error(diff)
}
count += len(want)
}
return count
}
func (d diagnostics) collect(pos token.Position, msg string) {
if _, ok := d[pos.Filename]; !ok {
d[pos.Filename] = []protocol.Diagnostic{}
func (d diagnostics) collect(rng packagestest.RangePosition, msg string) {
if rng.Start.Filename != rng.End.Filename {
return
}
filename := rng.Start.Filename
if _, ok := d[filename]; !ok {
d[filename] = []protocol.Diagnostic{}
}
// If a file has an empty diagnostics, mark that and return. This allows us
// to avoid testing diagnostics in files that may have a lot of them.
if msg == "" {
return
}
line := float64(pos.Line - 1)
col := float64(pos.Column - 1)
want := protocol.Diagnostic{
Range: protocol.Range{
Start: protocol.Position{
Line: line,
Character: col,
Line: float64(rng.Start.Line - 1),
Character: float64(rng.Start.Column - 1),
},
End: protocol.Position{
Line: line,
Character: col,
Line: float64(rng.End.Line - 1),
Character: float64(rng.End.Column - 1),
},
},
Severity: protocol.SeverityError,
Source: "LSP",
Message: msg,
}
d[pos.Filename] = append(d[pos.Filename], want)
d[filename] = append(d[filename], want)
}
// diffDiagnostics prints the diff between expected and actual diagnostics test
// results.
func diffDiagnostics(filename string, want, got []protocol.Diagnostic) string {
if len(got) != len(want) {
goto Failed
}
for i, w := range want {
g := got[i]
if w.Message != g.Message {
goto Failed
}
if w.Range.Start != g.Range.Start {
goto Failed
}
// Special case for diagnostics on parse errors.
if strings.Contains(filename, "noparse") {
if g.Range.Start != g.Range.End || w.Range.Start != g.Range.End {
goto Failed
}
} else {
if w.Range.End != g.Range.End {
goto Failed
}
}
if w.Severity != g.Severity {
goto Failed
}
if w.Source != g.Source {
goto Failed
}
}
return ""
Failed:
msg := &bytes.Buffer{}
fmt.Fprintf(msg, "diagnostics failed for %s:\nexpected:\n", filename)
for _, d := range want {
fmt.Fprintf(msg, " %v\n", d)
}
fmt.Fprintf(msg, "got:\n")
for _, d := range got {
fmt.Fprintf(msg, " %v\n", d)
}
return msg.String()
}
func (c completions) test(t *testing.T, exported *packagestest.Exported, s *server, items completionItems) {
@ -240,7 +286,7 @@ func (c completions) test(t *testing.T, exported *packagestest.Exported, s *serv
if err != nil {
t.Fatalf("completion failed for %s:%v:%v: %v", filepath.Base(src.Filename), src.Line, src.Column, err)
}
if diff := diffC(src, want, got); diff != "" {
if diff := diffCompletionItems(src, want, got); diff != "" {
t.Errorf(diff)
}
}
@ -279,6 +325,38 @@ func (i completionItems) collect(pos token.Pos, label, detail, kind string) {
}
}
// diffCompletionItems prints the diff between expected and actual completion
// test results.
func diffCompletionItems(pos token.Position, want, got []protocol.CompletionItem) string {
if len(got) != len(want) {
goto Failed
}
for i, w := range want {
g := got[i]
if w.Label != g.Label {
goto Failed
}
if w.Detail != g.Detail {
goto Failed
}
if w.Kind != g.Kind {
goto Failed
}
}
return ""
Failed:
msg := &bytes.Buffer{}
fmt.Fprintf(msg, "completion failed for %s:%v:%v:\nexpected:\n", filepath.Base(pos.Filename), pos.Line, pos.Column)
for _, d := range want {
fmt.Fprintf(msg, " %v\n", d)
}
fmt.Fprintf(msg, "got:\n")
for _, d := range got {
fmt.Fprintf(msg, " %v\n", d)
}
return msg.String()
}
func (f formats) test(t *testing.T, s *server) {
for filename, gofmted := range f {
edits, err := s.Formatting(context.Background(), &protocol.DocumentFormattingParams{
@ -341,48 +419,3 @@ func (d definitions) collect(fset *token.FileSet, src, target packagestest.Range
tLoc := toProtocolLocation(fset, tRange)
d[sLoc] = tLoc
}
// diffD prints the diff between expected and actual diagnostics test results.
func diffD(filename string, want, got []protocol.Diagnostic) string {
msg := &bytes.Buffer{}
fmt.Fprintf(msg, "diagnostics failed for %s:\nexpected:\n", filename)
for _, d := range want {
fmt.Fprintf(msg, " %v\n", d)
}
fmt.Fprintf(msg, "got:\n")
for _, d := range got {
fmt.Fprintf(msg, " %v\n", d)
}
return msg.String()
}
// diffC prints the diff between expected and actual completion test results.
func diffC(pos token.Position, want, got []protocol.CompletionItem) string {
if len(got) != len(want) {
goto Failed
}
for i, w := range want {
g := got[i]
if w.Label != g.Label {
goto Failed
}
if w.Detail != g.Detail {
goto Failed
}
if w.Kind != g.Kind {
goto Failed
}
}
return ""
Failed:
msg := &bytes.Buffer{}
fmt.Fprintf(msg, "completion failed for %s:%v:%v:\nexpected:\n", filepath.Base(pos.Filename), pos.Line, pos.Column)
for _, d := range want {
fmt.Fprintf(msg, " %v\n", d)
}
fmt.Fprintf(msg, "got:\n")
for _, d := range got {
fmt.Fprintf(msg, " %v\n", d)
}
return msg.String()
}

10
internal/lsp/server.go
View File

@ -143,12 +143,14 @@ func (s *server) WillSaveWaitUntil(context.Context, *protocol.WillSaveTextDocume
}
func (s *server) DidSave(context.Context, *protocol.DidSaveTextDocumentParams) error {
// TODO(rstambler): Should we clear the cache here?
return nil // ignore
}
func (s *server) DidClose(ctx context.Context, params *protocol.DidCloseTextDocumentParams) error {
s.view.GetFile(source.URI(params.TextDocument.URI)).SetContent(nil)
f := s.view.GetFile(source.URI(params.TextDocument.URI))
if f, ok := f.(*cache.File); ok {
f.SetContent(nil)
}
return nil
}
@ -214,7 +216,7 @@ func (s *server) Definition(ctx context.Context, params *protocol.TextDocumentPo
return nil, err
}
pos := fromProtocolPosition(tok, params.Position)
r, err := source.Definition(ctx, f, pos)
r, err := source.Definition(ctx, s.view, f, pos)
if err != nil {
return nil, err
}
@ -228,7 +230,7 @@ func (s *server) TypeDefinition(ctx context.Context, params *protocol.TextDocume
return nil, err
}
pos := fromProtocolPosition(tok, params.Position)
r, err := source.TypeDefinition(ctx, f, pos)
r, err := source.TypeDefinition(ctx, s.view, f, pos)
if err != nil {
return nil, err
}

53
internal/lsp/source/definition.go
View File

@ -11,12 +11,11 @@ import (
"go/ast"
"go/token"
"go/types"
"io/ioutil"
"golang.org/x/tools/go/ast/astutil"
)
func Definition(ctx context.Context, f File, pos token.Pos) (Range, error) {
func Definition(ctx context.Context, v View, f File, pos token.Pos) (Range, error) {
fAST, err := f.GetAST()
if err != nil {
return Range{}, err
@ -49,10 +48,10 @@ func Definition(ctx context.Context, f File, pos token.Pos) (Range, error) {
if err != nil {
return Range{}, err
}
return objToRange(fset, obj), nil
return objToRange(v, fset, obj), nil
}
func TypeDefinition(ctx context.Context, f File, pos token.Pos) (Range, error) {
func TypeDefinition(ctx context.Context, v View, f File, pos token.Pos) (Range, error) {
fAST, err := f.GetAST()
if err != nil {
return Range{}, err
@ -80,7 +79,7 @@ func TypeDefinition(ctx context.Context, f File, pos token.Pos) (Range, error) {
if err != nil {
return Range{}, err
}
return objToRange(fset, obj), nil
return objToRange(v, fset, obj), nil
}
func typeToObject(typ types.Type) (obj types.Object) {
@ -138,33 +137,43 @@ func checkIdentifier(f *ast.File, pos token.Pos) (ident, error) {
return result, nil
}
func objToRange(fSet *token.FileSet, obj types.Object) Range {
func objToRange(v View, fset *token.FileSet, obj types.Object) Range {
p := obj.Pos()
f := fSet.File(p)
f := fset.File(p)
pos := f.Position(p)
if pos.Column == 1 {
// Column is 1, so we probably do not have full position information
// Currently exportdata does not store the column.
// For now we attempt to read the original source and find the identifier
// within the line. If we find it we patch the column to match its offset.
// TODO: we have probably already added the full data for the file to the
// fileset, we ought to track it rather than adding it over and over again
// TODO: if we parse from source, we will never need this hack
if src, err := ioutil.ReadFile(pos.Filename); err == nil {
newF := fSet.AddFile(pos.Filename, -1, len(src))
newF.SetLinesForContent(src)
lineStart := lineStart(newF, pos.Line)
offset := newF.Offset(lineStart)
// We do not have full position information because exportdata does not
// store the column. For now, we attempt to read the original source
// and find the identifier within the line. If we find it, we patch the
// column to match its offset.
//
// TODO: If we parse from source, we will never need this hack.
f := v.GetFile(ToURI(pos.Filename))
tok, err := f.GetToken()
if err != nil {
goto Return
}
src, err := f.Read()
if err != nil {
goto Return
}
start := lineStart(tok, pos.Line)
offset := tok.Offset(start)
col := bytes.Index(src[offset:], []byte(obj.Name()))
p = newF.Pos(offset + col)
}
p = tok.Pos(offset + col)
}
Return:
return Range{
Start: p,
End: p + token.Pos(len([]byte(obj.Name()))), // TODO: use real range of obj
End: p + token.Pos(identifierLen(obj.Name())),
}
}
// TODO: This needs to be fixed to address golang.org/issue/29149.
func identifierLen(ident string) int {
return len([]byte(ident))
}
// this functionality was borrowed from the analysisutil package
func lineStart(f *token.File, line int) token.Pos {
// Use binary search to find the start offset of this line.

53
internal/lsp/source/diagnostics.go
View File

@ -5,7 +5,9 @@
package source
import (
"bytes"
"context"
"fmt"
"go/token"
"strconv"
"strings"
@ -14,11 +16,11 @@ import (
)
type Diagnostic struct {
token.Position
Range
Message string
}
func Diagnostics(ctx context.Context, f File) (map[string][]Diagnostic, error) {
func Diagnostics(ctx context.Context, v View, f File) (map[string][]Diagnostic, error) {
pkg, err := f.GetPackage()
if err != nil {
return nil, err
@ -47,8 +49,25 @@ func Diagnostics(ctx context.Context, f File) (map[string][]Diagnostic, error) {
}
for _, diag := range diags {
pos := errorPos(diag)
diagFile := v.GetFile(ToURI(pos.Filename))
diagTok, err := diagFile.GetToken()
if err != nil {
continue
}
content, err := diagFile.Read()
if err != nil {
continue
}
end, err := identifierEnd(content, pos.Line, pos.Column)
// Don't set a range if it's anything other than a type error.
if err != nil || diag.Kind != packages.TypeError {
end = 0
}
diagnostic := Diagnostic{
Position: pos,
Range: Range{
Start: fromTokenPosition(diagTok, pos.Line, pos.Column),
End: fromTokenPosition(diagTok, pos.Line, pos.Column+end),
},
Message: diag.Msg,
}
if _, ok := reports[pos.Filename]; ok {
@ -58,12 +77,14 @@ func Diagnostics(ctx context.Context, f File) (map[string][]Diagnostic, error) {
return reports, nil
}
// FromTokenPosition converts a token.Position (1-based line and column
// number) to a token.Pos (byte offset value).
// It requires the token file the pos belongs to in order to do this.
func FromTokenPosition(f *token.File, pos token.Position) token.Pos {
line := lineStart(f, pos.Line)
return line + token.Pos(pos.Column-1) // TODO: this is wrong, bytes not characters
// fromTokenPosition converts a token.Position (1-based line and column
// number) to a token.Pos (byte offset value). This requires the token.File
// to which the token.Pos belongs.
func fromTokenPosition(f *token.File, line, col int) token.Pos {
linePos := lineStart(f, line)
// TODO: This is incorrect, as pos.Column represents bytes, not characters.
// This needs to be handled to address golang.org/issue/29149.
return linePos + token.Pos(col-1)
}
func errorPos(pkgErr packages.Error) token.Position {
@ -92,3 +113,17 @@ func chop(text string) (remainder string, value int, ok bool) {
}
return text[:i], int(v), true
}
// identifierEnd returns the length of an identifier within a string,
// given the starting line and column numbers of the identifier.
func identifierEnd(content []byte, l, c int) (int, error) {
lines := bytes.Split(content, []byte("\n"))
if len(lines) < l {
return 0, fmt.Errorf("invalid line number: got %v, but only %v lines", l, len(lines))
}
line := lines[l-1]
if len(line) < c {
return 0, fmt.Errorf("invalid column number: got %v, but the length of the line is %v", c, len(line))
}
return bytes.IndexAny(line[c-1:], " \n,():;[]"), nil
}

2
internal/lsp/source/uri.go
View File

@ -55,7 +55,7 @@ func toURI(path string) *url.URL {
// Handle standard library paths that contain the literal "$GOROOT".
// TODO(rstambler): The go/packages API should allow one to determine a user's $GOROOT.
const prefix = "$GOROOT"
if strings.EqualFold(prefix, path[:len(prefix)]) {
if len(path) >= len(prefix) && strings.EqualFold(prefix, path[:len(prefix)]) {
suffix := path[len(prefix):]
path = runtime.GOROOT() + suffix
}

9
internal/lsp/source/file.go → internal/lsp/source/view.go
View File

@ -11,6 +11,14 @@ import (
"golang.org/x/tools/go/packages"
)
// View abstracts the underlying architecture of the package using the source
// package. The view provides access to files and their contents, so the source
// package does not directly access the file system.
type View interface {
// Consider adding an error to this method, if users require it.
GetFile(uri URI) File
}
// File represents a Go source file that has been type-checked. It is the input
// to most of the exported functions in this package, as it wraps up the
// building blocks for most queries. Users of the source package can abstract
@ -20,6 +28,7 @@ type File interface {
GetFileSet() (*token.FileSet, error)
GetPackage() (*packages.Package, error)
GetToken() (*token.File, error)
Read() ([]byte, error)
}
// Range represents a start and end position.

2
internal/lsp/testdata/godef/broken/unclosedIf.go.in vendored
View File

@ -7,5 +7,3 @@ func unclosedIf() {
var myUnclosedIf string //@myUnclosedIf
fmt.Printf("s = %v\n", myUnclosedIf) //@godef("my", myUnclosedIf)
}
//@diag(EOF, "expected ';', found 'EOF'")
//@diag(EOF, "expected '}', found 'EOF'")

2
internal/lsp/testdata/self/self.go.in vendored
View File

@ -3,7 +3,7 @@
package self
import (
"golang.org/x/tools/internal/lsp/self" //@diag("\"", "could not import golang.org/x/tools/internal/lsp/self (import cycle: [golang.org/x/tools/internal/lsp/self])")
"golang.org/x/tools/internal/lsp/self" //@diag("\"golang.org/x/tools/internal/lsp/self\"", "could not import golang.org/x/tools/internal/lsp/self (import cycle: [golang.org/x/tools/internal/lsp/self])")
)
func Hello() {}