diff --git a/go/packages/packagestest/expect.go b/go/packages/packagestest/expect.go index 0902057b..b2ee54c1 100644 --- a/go/packages/packagestest/expect.go +++ b/go/packages/packagestest/expect.go @@ -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 { @@ -173,13 +177,14 @@ func (e *Exported) getMarkers() error { } var ( - noteType = reflect.TypeOf((*expect.Note)(nil)) - identifierType = reflect.TypeOf(expect.Identifier("")) - posType = reflect.TypeOf(token.Pos(0)) - positionType = reflect.TypeOf(token.Position{}) - rangeType = reflect.TypeOf(Range{}) - fsetType = reflect.TypeOf((*token.FileSet)(nil)) - regexType = reflect.TypeOf((*regexp.Regexp)(nil)) + noteType = reflect.TypeOf((*expect.Note)(nil)) + identifierType = reflect.TypeOf(expect.Identifier("")) + 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)) ) // converter converts from a marker's argument parsed from the comment to @@ -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] diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 48410a1f..f3286f5f 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -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() diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 17fa8897..dcc2cafb 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -15,40 +15,35 @@ import ( func (s *server) CacheAndDiagnose(ctx context.Context, uri protocol.DocumentURI, text string) { f := s.view.GetFile(source.URI(uri)) - f.SetContent([]byte(text)) - + 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, - }), + Message: diag.Message, + 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 }) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index b5b4b9d9..ff0e69fb 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -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() -} diff --git a/internal/lsp/server.go b/internal/lsp/server.go index cc100749..03169d75 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -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 } diff --git a/internal/lsp/source/definition.go b/internal/lsp/source/definition.go index f83a4f38..d97be3bf 100644 --- a/internal/lsp/source/definition.go +++ b/internal/lsp/source/definition.go @@ -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) - col := bytes.Index(src[offset:], []byte(obj.Name())) - p = newF.Pos(offset + col) + // 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 = 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. diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 8f592657..2c1e01ed 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -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,9 +49,26 @@ 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, - Message: diag.Msg, + 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 { reports[pos.Filename] = append(reports[pos.Filename], diagnostic) @@ -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 +} diff --git a/internal/lsp/source/uri.go b/internal/lsp/source/uri.go index ddd70d38..d630e704 100644 --- a/internal/lsp/source/uri.go +++ b/internal/lsp/source/uri.go @@ -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 } diff --git a/internal/lsp/source/file.go b/internal/lsp/source/view.go similarity index 76% rename from internal/lsp/source/file.go rename to internal/lsp/source/view.go index aa7bc8e2..f0cd253e 100644 --- a/internal/lsp/source/file.go +++ b/internal/lsp/source/view.go @@ -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. diff --git a/internal/lsp/testdata/godef/broken/unclosedIf.go.in b/internal/lsp/testdata/godef/broken/unclosedIf.go.in index d29b108a..0f2cf1b1 100644 --- a/internal/lsp/testdata/godef/broken/unclosedIf.go.in +++ b/internal/lsp/testdata/godef/broken/unclosedIf.go.in @@ -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'") diff --git a/internal/lsp/testdata/self/self.go.in b/internal/lsp/testdata/self/self.go.in index b9c5f0d8..3da79aa5 100644 --- a/internal/lsp/testdata/self/self.go.in +++ b/internal/lsp/testdata/self/self.go.in @@ -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() {}