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: 1581cbe
GitHub-Pull-Request: #102
Reviewed-on: https://go-review.googlesource.com/c/tools/+/177605
Run-TryBot: Ian Cottrell <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Cottrell <[email protected]>

Author: evandigby

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
 }

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,

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()

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)

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,
 	}
 }

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())

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())
+			}
+		})
+	}
+}

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
 }