From d88f79806bbd013f54a668506864ce559edf6f0a Mon Sep 17 00:00:00 2001 From: Evan Digby Date: Fri, 17 May 2019 17:45:50 +0000 Subject: [PATCH] internal/lsp: fix swallowed package errors If a package has an error that makes it completely unparseable, such as containing a .go file with no "package" statement, the error was previously unreported. Such errors would manifest as other errors. Fixes golang/go#31712 Change-Id: I11b8d0e2e4d64b03fbcb4c35e7f0b02fccc83fad GitHub-Last-Rev: 1581cbe36c269dd964f0b9226dbd63b1650c2a5b GitHub-Pull-Request: golang/tools#102 Reviewed-on: https://go-review.googlesource.com/c/tools/+/177605 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cmd/cmd.go | 2 +- internal/lsp/diagnostics.go | 9 ++-- internal/lsp/link.go | 5 +++ internal/lsp/lsp_test.go | 4 +- internal/lsp/protocol/span.go | 11 ++++- internal/lsp/source/diagnostics.go | 32 +++++++++++++- internal/lsp/source/diagnostics_test.go | 55 +++++++++++++++++++++++++ internal/lsp/util.go | 11 +++-- 8 files changed, 116 insertions(+), 13 deletions(-) create mode 100644 internal/lsp/source/diagnostics_test.go diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index 00ccd029..9b676c9e 100644 --- a/internal/lsp/cmd/cmd.go +++ b/internal/lsp/cmd/cmd.go @@ -339,7 +339,7 @@ func (c *cmdClient) getFile(ctx context.Context, uri span.URI) *cmdFile { } f := c.fset.AddFile(fname, -1, len(content)) f.SetLinesForContent(content) - file.mapper = protocol.NewColumnMapper(uri, c.fset, f, content) + file.mapper = protocol.NewColumnMapper(uri, fname, c.fset, f, content) } return file } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 851d9bdc..4e4f7fc3 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -6,6 +6,7 @@ package lsp import ( "context" + "strings" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" @@ -40,8 +41,10 @@ func (s *Server) Diagnostics(ctx context.Context, view source.View, uri span.URI // Anytime we compute diagnostics, make sure to also send along any // undelivered ones (only for remaining URIs). for uri, diagnostics := range s.undelivered { - s.publishDiagnostics(ctx, view, uri, diagnostics) - + err := s.publishDiagnostics(ctx, view, uri, diagnostics) + if err != nil { + s.session.Logger().Errorf(ctx, "failed to deliver diagnostic for %s: %v", uri, err) + } // If we fail to deliver the same diagnostics twice, just give up. delete(s.undelivered, uri) } @@ -78,7 +81,7 @@ func toProtocolDiagnostics(ctx context.Context, v source.View, diagnostics []sou return nil, err } reports = append(reports, protocol.Diagnostic{ - Message: diag.Message, + Message: strings.TrimSpace(diag.Message), // go list returns errors prefixed by newline Range: rng, Severity: severity, Source: diag.Source, diff --git a/internal/lsp/link.go b/internal/lsp/link.go index d437fb12..fd8eb49d 100644 --- a/internal/lsp/link.go +++ b/internal/lsp/link.go @@ -6,6 +6,7 @@ package lsp import ( "context" + "fmt" "strconv" "golang.org/x/tools/internal/lsp/protocol" @@ -21,6 +22,10 @@ func (s *Server) documentLink(ctx context.Context, params *protocol.DocumentLink } // find the import block ast := f.GetAST(ctx) + if ast == nil { + return nil, fmt.Errorf("no AST for %v", uri) + } + var result []protocol.DocumentLink for _, imp := range ast.Imports { spn, err := span.NewRange(f.GetFileSet(ctx), imp.Pos(), imp.End()).Span() diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 172f77c2..e6cffa07 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -594,7 +594,7 @@ func (r *runner) mapper(uri span.URI) (*protocol.ColumnMapper, error) { if err != nil { return nil, err } - return protocol.NewColumnMapper(uri, fset, f, content), nil + return protocol.NewColumnMapper(uri, filename, fset, f, content), nil } func TestBytesOffset(t *testing.T) { @@ -624,7 +624,7 @@ func TestBytesOffset(t *testing.T) { fset := token.NewFileSet() f := fset.AddFile(fname, -1, len(test.text)) f.SetLinesForContent([]byte(test.text)) - mapper := protocol.NewColumnMapper(span.FileURI(fname), fset, f, []byte(test.text)) + mapper := protocol.NewColumnMapper(span.FileURI(fname), fname, fset, f, []byte(test.text)) got, err := mapper.Point(test.pos) if err != nil && test.want != -1 { t.Errorf("unexpected error: %v", err) diff --git a/internal/lsp/protocol/span.go b/internal/lsp/protocol/span.go index 9045b2cd..f41fba82 100644 --- a/internal/lsp/protocol/span.go +++ b/internal/lsp/protocol/span.go @@ -23,10 +23,17 @@ func NewURI(uri span.URI) string { return string(uri) } -func NewColumnMapper(uri span.URI, fset *token.FileSet, f *token.File, content []byte) *ColumnMapper { +func NewColumnMapper(uri span.URI, fn string, fset *token.FileSet, f *token.File, content []byte) *ColumnMapper { + var converter *span.TokenConverter + if f == nil { + converter = span.NewContentConverter(fn, content) + } else { + converter = span.NewTokenConverter(fset, f) + } + return &ColumnMapper{ URI: uri, - Converter: span.NewTokenConverter(fset, f), + Converter: converter, Content: content, } } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index f6ae6755..29779284 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -8,6 +8,7 @@ import ( "bytes" "context" "fmt" + "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/asmdecl" @@ -72,6 +73,12 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag } reports[uri] = []Diagnostic{} } + + // Prepare reports for package errors + for _, pkgErr := range pkg.GetErrors() { + reports[packageErrorSpan(pkgErr).URI()] = []Diagnostic{} + } + // Run diagnostics for the package that this URI belongs to. if !diagnostics(ctx, v, pkg, reports) { // If we don't have any list, parse, or type errors, run analyses. @@ -117,7 +124,7 @@ func diagnostics(ctx context.Context, v View, pkg Package, reports map[span.URI] diags = listErrors } for _, diag := range diags { - spn := span.Parse(diag.Pos) + spn := packageErrorSpan(diag) if spn.IsPoint() && diag.Kind == packages.TypeError { spn = pointToSpan(ctx, v, spn) } @@ -161,6 +168,29 @@ func analyses(ctx context.Context, v View, pkg Package, reports map[span.URI][]D return nil } +// parseDiagnosticMessage attempts to parse a standard error message by stripping off the trailing error message. +// Works only on errors where the message is prefixed by ": ". +// e.g.: +// attributes.go:13:1: expected 'package', found 'type' +func parseDiagnosticMessage(input string) span.Span { + input = strings.TrimSpace(input) + + msgIndex := strings.Index(input, ": ") + if msgIndex < 0 { + return span.Parse(input) + } + + return span.Parse(input[:msgIndex]) +} + +func packageErrorSpan(pkgErr packages.Error) span.Span { + if pkgErr.Pos == "" { + return parseDiagnosticMessage(pkgErr.Msg) + } + + return span.Parse(pkgErr.Pos) +} + func pointToSpan(ctx context.Context, v View, spn span.Span) span.Span { // Don't set a range if it's anything other than a type error. f, err := v.GetFile(ctx, spn.URI()) diff --git a/internal/lsp/source/diagnostics_test.go b/internal/lsp/source/diagnostics_test.go new file mode 100644 index 00000000..8d37a52b --- /dev/null +++ b/internal/lsp/source/diagnostics_test.go @@ -0,0 +1,55 @@ +// Copyright 2019 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package source + +import ( + "strings" + "testing" +) + +func TestParseErrorMessage(t *testing.T) { + tests := []struct { + name string + in string + expectedFileName string + expectedLine int + expectedColumn int + }{ + { + name: "from go list output", + in: "\nattributes.go:13:1: expected 'package', found 'type'", + expectedFileName: "attributes.go", + expectedLine: 13, + expectedColumn: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spn := parseDiagnosticMessage(tt.in) + fn, err := spn.URI().Filename() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !strings.HasSuffix(fn, tt.expectedFileName) { + t.Errorf("expected filename with suffix %v but got %v", tt.expectedFileName, fn) + } + + if !spn.HasPosition() { + t.Fatalf("expected span to have position") + } + + pos := spn.Start() + if pos.Line() != tt.expectedLine { + t.Errorf("expected line %v but got %v", tt.expectedLine, pos.Line()) + } + + if pos.Column() != tt.expectedColumn { + t.Errorf("expected line %v but got %v", tt.expectedLine, pos.Line()) + } + }) + } +} diff --git a/internal/lsp/util.go b/internal/lsp/util.go index 0d61de5d..aa8be476 100644 --- a/internal/lsp/util.go +++ b/internal/lsp/util.go @@ -51,11 +51,14 @@ func getSourceFile(ctx context.Context, v source.View, uri span.URI) (source.Fil if err != nil { return nil, nil, err } - tok := f.GetToken(ctx) - if tok == nil { - return nil, nil, fmt.Errorf("no file information for %v", f.URI()) + + fname, err := f.URI().Filename() + if err != nil { + return nil, nil, err } - m := protocol.NewColumnMapper(f.URI(), f.GetFileSet(ctx), tok, f.GetContent(ctx)) + + m := protocol.NewColumnMapper(f.URI(), fname, f.GetFileSet(ctx), f.GetToken(ctx), f.GetContent(ctx)) + return f, m, nil }