internal/lsp: refactor error handling code in type-checking

This change adds a source.Error type which is used to collect the error
information that comes out of the loading, parsing, and type checking
stages. We also add specific sources per-error, rather than having them
all be labeled as "LSP".

This change will enable follow-ups that do a better job of extracting
error ranges.

Change-Id: I3fbb5e42d66aa2c5bb1b2f41d1eadfc45f3a749b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202298
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

internal/lsp/cache/check.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	"fmt"
 	"go/ast"
-	"go/scanner"
 	"go/types"
 	"sort"
 	"sync"
@@ -265,6 +264,11 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*p
 	ctx, done := trace.StartSpan(ctx, "cache.importer.typeCheck", telemetry.Package.Of(cph.m.id))
 	defer done()
 
+	var rawErrors []error
+	for _, err := range cph.m.errors {
+		rawErrors = append(rawErrors, err)
+	}
+
 	pkg := &pkg{
 		snapshot:   imp.snapshot,
 		id:         cph.m.id,
@@ -282,11 +286,6 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*p
 			Scopes:     make(map[ast.Node]*types.Scope),
 		},
 	}
-	// If the package comes back with errors from `go list`,
-	// don't bother type-checking it.
-	for _, err := range cph.m.errors {
-		pkg.errors = append(cph.m.errors, err)
-	}
 	var (
 		files       = make([]*ast.File, len(pkg.files))
 		parseErrors = make([]error, len(pkg.files))
@@ -302,9 +301,9 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*p
 	}
 	wg.Wait()
 
-	for _, err := range parseErrors {
-		if err != nil {
-			imp.snapshot.view.session.cache.appendPkgError(pkg, err)
+	for _, e := range parseErrors {
+		if e != nil {
+			rawErrors = append(rawErrors, e)
 		}
 	}
 
@@ -318,7 +317,7 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*p
 	files = files[:i]
 
 	// Use the default type information for the unsafe package.
-	if cph.m.pkgPath == "unsafe" {
+	if pkg.pkgPath == "unsafe" {
 		pkg.types = types.Unsafe
 	} else if len(files) == 0 { // not the unsafe package, no parsed files
 		return nil, errors.Errorf("no parsed files for package %s", pkg.pkgPath)
@@ -327,8 +326,8 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*p
 	}
 
 	cfg := &types.Config{
-		Error: func(err error) {
-			imp.snapshot.view.session.cache.appendPkgError(pkg, err)
+		Error: func(e error) {
+			rawErrors = append(rawErrors, e)
 		},
 		Importer: imp.depImporter(ctx, cph, pkg),
 	}
@@ -337,6 +336,14 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*p
 	// Type checking errors are handled via the config, so ignore them here.
 	_ = check.Files(files)
 
+	for _, e := range rawErrors {
+		srcErr, err := sourceError(ctx, imp.snapshot.view, pkg, e)
+		if err != nil {
+			return nil, err
+		}
+		pkg.errors = append(pkg.errors, *srcErr)
+	}
+
 	return pkg, nil
 }
 
@@ -356,34 +363,3 @@ func (imp *importer) depImporter(ctx context.Context, cph *checkPackageHandle, p
 		ctx:                      ctx,
 	}
 }
-
-func (c *cache) appendPkgError(pkg *pkg, err error) {
-	if err == nil {
-		return
-	}
-	var errs []packages.Error
-	switch err := err.(type) {
-	case *scanner.Error:
-		errs = append(errs, packages.Error{
-			Pos:  err.Pos.String(),
-			Msg:  err.Msg,
-			Kind: packages.ParseError,
-		})
-	case scanner.ErrorList:
-		// The first parser error is likely the root cause of the problem.
-		if err.Len() > 0 {
-			errs = append(errs, packages.Error{
-				Pos:  err[0].Pos.String(),
-				Msg:  err[0].Msg,
-				Kind: packages.ParseError,
-			})
-		}
-	case types.Error:
-		errs = append(errs, packages.Error{
-			Pos:  c.FileSet().Position(err.Pos).String(),
-			Msg:  err.Msg,
-			Kind: packages.TypeError,
-		})
-	}
-	pkg.errors = append(pkg.errors, errs...)
-}

internal/lsp/cache/error_test.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package source
+package cache
 
 import (
 	"strings"
@@ -28,7 +28,7 @@ func TestParseErrorMessage(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			spn := parseDiagnosticMessage(tt.in)
+			spn := parseGoListError(tt.in)
 			fn := spn.URI().Filename()
 
 			if !strings.HasSuffix(fn, tt.expectedFileName) {

internal/lsp/cache/errors.go

@@ -0,0 +1,103 @@
+package cache
+
+import (
+	"bytes"
+	"context"
+	"go/scanner"
+	"go/types"
+	"strings"
+
+	"golang.org/x/tools/go/packages"
+	"golang.org/x/tools/internal/lsp/protocol"
+	"golang.org/x/tools/internal/lsp/source"
+	"golang.org/x/tools/internal/span"
+)
+
+func sourceError(ctx context.Context, view *view, pkg *pkg, e error) (*source.Error, error) {
+	var (
+		spn  span.Span
+		msg  string
+		kind packages.ErrorKind
+	)
+	switch e := e.(type) {
+	case packages.Error:
+		if e.Pos == "" {
+			spn = parseGoListError(e.Msg)
+		} else {
+			spn = span.Parse(e.Pos)
+		}
+		msg = e.Msg
+		kind = e.Kind
+	case *scanner.Error:
+		msg = e.Msg
+		kind = packages.ParseError
+		spn = span.Parse(e.Pos.String())
+	case scanner.ErrorList:
+		// The first parser error is likely the root cause of the problem.
+		if e.Len() > 0 {
+			spn = span.Parse(e[0].Pos.String())
+			msg = e[0].Msg
+			kind = packages.ParseError
+		}
+	case types.Error:
+		spn = span.Parse(view.session.cache.fset.Position(e.Pos).String())
+		msg = e.Msg
+		kind = packages.TypeError
+	}
+	rng, err := spanToRange(ctx, pkg, spn, kind == packages.TypeError)
+	if err != nil {
+		return nil, err
+	}
+	return &source.Error{
+		URI:   spn.URI(),
+		Range: rng,
+		Msg:   msg,
+		Kind:  kind,
+	}, nil
+}
+
+// spanToRange converts a span.Span to a protocol.Range,
+// assuming that the span belongs to the package whose diagnostics are being computed.
+func spanToRange(ctx context.Context, pkg *pkg, spn span.Span, isTypeError bool) (protocol.Range, error) {
+	ph, err := pkg.File(spn.URI())
+	if err != nil {
+		return protocol.Range{}, err
+	}
+	_, m, _, err := ph.Cached(ctx)
+	if err != nil {
+		return protocol.Range{}, err
+	}
+	data, _, err := ph.File().Read(ctx)
+	if err != nil {
+		return protocol.Range{}, err
+	}
+	if spn.IsPoint() && isTypeError {
+		if s, err := spn.WithOffset(m.Converter); err == nil {
+			start := s.Start()
+			offset := start.Offset()
+			if offset < len(data) {
+				if width := bytes.IndexAny(data[offset:], " \n,():;[]"); width > 0 {
+					spn = span.New(spn.URI(), start, span.NewPoint(start.Line(), start.Column()+width, offset+width))
+				}
+			}
+		}
+	}
+	return m.Range(spn)
+}
+
+// parseGoListError attempts to parse a standard `go list` error message
+// by stripping off the trailing error message.
+//
+// It works only on errors whose message is prefixed by colon,
+// followed by a space (": "). For example:
+//
+//   attributes.go:13:1: expected 'package', found 'type'
+//
+func parseGoListError(input string) span.Span {
+	input = strings.TrimSpace(input)
+	msgIndex := strings.Index(input, ": ")
+	if msgIndex < 0 {
+		return span.Parse(input)
+	}
+	return span.Parse(input[:msgIndex])
+}

internal/lsp/cache/pkg.go

@@ -11,7 +11,6 @@ import (
 	"sync"
 
 	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
@@ -28,7 +27,7 @@ type pkg struct {
 	mode    source.ParseMode
 
 	files      []source.ParseGoHandle
-	errors     []packages.Error
+	errors     []source.Error
 	imports    map[packagePath]*pkg
 	types      *types.Package
 	typesInfo  *types.Info
@@ -80,7 +79,7 @@ func (p *pkg) GetSyntax(ctx context.Context) []*ast.File {
 	return syntax
 }
 
-func (p *pkg) GetErrors() []packages.Error {
+func (p *pkg) GetErrors() []source.Error {
 	return p.errors
 }
 

internal/lsp/source/diagnostics.go

@@ -5,10 +5,8 @@
 package source
 
 import (
-	"bytes"
 	"context"
 	"fmt"
-	"strings"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/packages"
@@ -76,7 +74,7 @@ func Diagnostics(ctx context.Context, view View, f File, disabledAnalyses map[st
 		if err.Kind != packages.ListError {
 			continue
 		}
-		clearReports(view, reports, packagesErrorSpan(err).URI())
+		clearReports(view, reports, err.URI)
 	}
 
 	// Run diagnostics for the package that this URI belongs to.
@@ -111,11 +109,10 @@ func diagnostics(ctx context.Context, view View, pkg Package, reports map[span.U
 
 	diagSets := make(map[span.URI]*diagnosticSet)
 	for _, err := range pkg.GetErrors() {
-		spn := packagesErrorSpan(err)
 		diag := &Diagnostic{
-			URI:      spn.URI(),
+			URI:      err.URI,
 			Message:  err.Msg,
-			Source:   "LSP",
+			Range:    err.Range,
 			Severity: protocol.SeverityError,
 		}
 		set, ok := diagSets[diag.URI]
@@ -126,17 +123,14 @@ func diagnostics(ctx context.Context, view View, pkg Package, reports map[span.U
 		switch err.Kind {
 		case packages.ParseError:
 			set.parseErrors = append(set.parseErrors, diag)
+			diag.Source = "syntax"
 		case packages.TypeError:
 			set.typeErrors = append(set.typeErrors, diag)
+			diag.Source = "compiler"
 		default:
 			set.listErrors = append(set.listErrors, diag)
+			diag.Source = "go list"
 		}
-		rng, err := spanToRange(ctx, view, pkg, spn, err.Kind == packages.TypeError)
-		if err != nil {
-			log.Error(ctx, "failed to convert span to range", err)
-			continue
-		}
-		diag.Range = rng
 	}
 	var nonEmptyDiagnostics bool // track if we actually send non-empty diagnostics
 	for uri, set := range diagSets {
@@ -159,35 +153,6 @@ func diagnostics(ctx context.Context, view View, pkg Package, reports map[span.U
 	return nonEmptyDiagnostics
 }
 
-// spanToRange converts a span.Span to a protocol.Range,
-// assuming that the span belongs to the package whose diagnostics are being computed.
-func spanToRange(ctx context.Context, view View, pkg Package, spn span.Span, isTypeError bool) (protocol.Range, error) {
-	ph, err := pkg.File(spn.URI())
-	if err != nil {
-		return protocol.Range{}, err
-	}
-	_, m, _, err := ph.Cached(ctx)
-	if err != nil {
-		return protocol.Range{}, err
-	}
-	data, _, err := ph.File().Read(ctx)
-	if err != nil {
-		return protocol.Range{}, err
-	}
-	// Try to get a range for the diagnostic.
-	// TODO: Don't just limit ranges to type errors.
-	if spn.IsPoint() && isTypeError {
-		if s, err := spn.WithOffset(m.Converter); err == nil {
-			start := s.Start()
-			offset := start.Offset()
-			if width := bytes.IndexAny(data[offset:], " \n,():;[]"); width > 0 {
-				spn = span.New(spn.URI(), start, span.NewPoint(start.Line(), start.Column()+width, offset+width))
-			}
-		}
-	}
-	return m.Range(spn)
-}
-
 func analyses(ctx context.Context, snapshot Snapshot, cph CheckPackageHandle, disabledAnalyses map[string]struct{}, reports map[span.URI][]Diagnostic) error {
 	// Type checking and parsing succeeded. Run analyses.
 	if err := runAnalyses(ctx, snapshot, cph, disabledAnalyses, func(diags []*analysis.Diagnostic, a *analysis.Analyzer) error {
@@ -206,10 +171,8 @@ func analyses(ctx context.Context, snapshot Snapshot, cph CheckPackageHandle, di
 }
 
 func toDiagnostic(ctx context.Context, view View, diag *analysis.Diagnostic, category string) (Diagnostic, error) {
-	r := span.NewRange(view.Session().Cache().FileSet(), diag.Pos, diag.End)
-	spn, err := r.Span()
+	spn, err := span.NewRange(view.Session().Cache().FileSet(), diag.Pos, diag.End).Span()
 	if err != nil {
-		// The diagnostic has an invalid position, so we don't have a valid span.
 		return Diagnostic{}, err
 	}
 	f, err := view.GetFile(ctx, spn.URI())
@@ -230,7 +193,15 @@ func toDiagnostic(ctx context.Context, view View, diag *analysis.Diagnostic, cat
 	if err != nil {
 		return Diagnostic{}, err
 	}
-	rng, err := spanToRange(ctx, view, pkg, spn, false)
+	ph, err := pkg.File(spn.URI())
+	if err != nil {
+		return Diagnostic{}, err
+	}
+	_, m, _, err := ph.Cached(ctx)
+	if err != nil {
+		return Diagnostic{}, err
+	}
+	rng, err := m.Range(spn)
 	if err != nil {
 		return Diagnostic{}, err
 	}
@@ -275,30 +246,6 @@ func addReport(v View, reports map[span.URI][]Diagnostic, uri span.URI, diagnost
 	}
 }
 
-func packagesErrorSpan(err packages.Error) span.Span {
-	if err.Pos == "" {
-		return parseDiagnosticMessage(err.Msg)
-	}
-	return span.Parse(err.Pos)
-}
-
-// parseDiagnosticMessage attempts to parse a standard `go list` error message
-// by stripping off the trailing error message.
-//
-// It works only on errors whose message is prefixed by colon,
-// followed by a space (": "). For example:
-//
-//   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 singleDiagnostic(uri span.URI, format string, a ...interface{}) map[span.URI][]Diagnostic {
 	return map[span.URI][]Diagnostic{
 		uri: []Diagnostic{{

internal/lsp/source/format.go

@@ -49,7 +49,7 @@ func Format(ctx context.Context, view View, f File) ([]protocol.TextEdit, error)
 	if err != nil {
 		return nil, err
 	}
-	if hasListErrors(pkg.GetErrors()) || hasParseErrors(pkg, f.URI()) {
+	if hasListErrors(pkg) || hasParseErrors(pkg, f.URI()) {
 		// Even if this package has list or parse errors, this file may not
 		// have any parse errors and can still be formatted. Using format.Node
 		// on an ast with errors may result in code being added or removed.
@@ -102,7 +102,7 @@ func Imports(ctx context.Context, view View, f File) ([]protocol.TextEdit, error
 	if err != nil {
 		return nil, err
 	}
-	if hasListErrors(pkg.GetErrors()) {
+	if hasListErrors(pkg) {
 		return nil, errors.Errorf("%s has list errors, not running goimports", f.URI())
 	}
 	ph, err := pkg.File(f.URI())
@@ -168,7 +168,7 @@ func AllImportsFixes(ctx context.Context, view View, f File) (edits []protocol.T
 	if err != nil {
 		return nil, nil, err
 	}
-	if hasListErrors(pkg.GetErrors()) {
+	if hasListErrors(pkg) {
 		return nil, nil, errors.Errorf("%s has list errors, not running goimports", f.URI())
 	}
 	options := &imports.Options{
@@ -269,16 +269,15 @@ func CandidateImports(ctx context.Context, view View, filename string) (pkgs []i
 // hasParseErrors returns true if the given file has parse errors.
 func hasParseErrors(pkg Package, uri span.URI) bool {
 	for _, err := range pkg.GetErrors() {
-		spn := packagesErrorSpan(err)
-		if spn.URI() == uri && err.Kind == packages.ParseError {
+		if err.URI == uri && err.Kind == packages.ParseError {
 			return true
 		}
 	}
 	return false
 }
 
-func hasListErrors(errors []packages.Error) bool {
-	for _, err := range errors {
+func hasListErrors(pkg Package) bool {
+	for _, err := range pkg.GetErrors() {
 		if err.Kind == packages.ListError {
 			return true
 		}

internal/lsp/source/view.go

@@ -280,7 +280,7 @@ type Package interface {
 	Files() []ParseGoHandle
 	File(uri span.URI) (ParseGoHandle, error)
 	GetSyntax(context.Context) []*ast.File
-	GetErrors() []packages.Error
+	GetErrors() []Error
 	GetTypes() *types.Package
 	GetTypesInfo() *types.Info
 	GetTypesSizes() types.Sizes
@@ -297,6 +297,17 @@ type Package interface {
 	FindFile(ctx context.Context, uri span.URI) (ParseGoHandle, Package, error)
 }
 
+type Error struct {
+	Msg   string
+	URI   span.URI
+	Range protocol.Range
+	Kind  packages.ErrorKind
+}
+
+func (e *Error) Error() string {
+	return fmt.Sprintf("%s:%s: %s", e.URI, e.Range, e.Msg)
+}
+
 type BuiltinPackage interface {
 	Lookup(name string) *ast.Object
 	Files() []ParseGoHandle

internal/lsp/testdata/bad/bad0.go

@@ -4,9 +4,9 @@ package bad
 
 func stuff() { //@item(stuff, "stuff", "func()", "func")
 	x := "heeeeyyyy"
-	random2(x) //@diag("x", "LSP", "cannot use x (variable of type string) as int value in argument to random2")
+	random2(x) //@diag("x", "compiler", "cannot use x (variable of type string) as int value in argument to random2")
 	random2(1) //@complete("dom", random, random2, random3)
-	y := 3     //@diag("y", "LSP", "y declared but not used")
+	y := 3     //@diag("y", "compiler", "y declared but not used")
 }
 
 type bob struct { //@item(bob, "bob", "struct{...}", "struct")
@@ -16,6 +16,6 @@ type bob struct { //@item(bob, "bob", "struct{...}", "struct")
 func _() {
 	var q int
 	_ = &bob{
-		f: q, //@diag("f", "LSP", "unknown field f in struct literal")
+		f: q, //@diag("f", "compiler", "unknown field f in struct literal")
 	}
 }

internal/lsp/testdata/bad/bad1.go

@@ -2,7 +2,7 @@
 
 package bad
 
-var a unknown //@item(global_a, "a", "unknown", "var"),diag("unknown", "LSP", "undeclared name: unknown")
+var a unknown //@item(global_a, "a", "unknown", "var"),diag("unknown", "compiler", "undeclared name: unknown")
 
 func random() int { //@item(random, "random", "func() int", "func")
 	//@complete("", global_a, bob, random, random2, random3, stuff)
@@ -10,9 +10,9 @@ func random() int { //@item(random, "random", "func() int", "func")
 }
 
 func random2(y int) int { //@item(random2, "random2", "func(y int) int", "func"),item(bad_y_param, "y", "int", "var")
-	x := 6     //@item(x, "x", "int", "var"),diag("x", "LSP", "x declared but not used")
-	var q blah //@item(q, "q", "blah", "var"),diag("q", "LSP", "q declared but not used"),diag("blah", "LSP", "undeclared name: blah")
-	var t blob //@item(t, "t", "blob", "var"),diag("t", "LSP", "t declared but not used"),diag("blob", "LSP", "undeclared name: blob")
+	x := 6     //@item(x, "x", "int", "var"),diag("x", "compiler", "x declared but not used")
+	var q blah //@item(q, "q", "blah", "var"),diag("q", "compiler", "q declared but not used"),diag("blah", "compiler", "undeclared name: blah")
+	var t blob //@item(t, "t", "blob", "var"),diag("t", "compiler", "t declared but not used"),diag("blob", "compiler", "undeclared name: blob")
 	//@complete("", q, t, x, bad_y_param, global_a, bob, random, random2, random3, stuff)
 
 	return y

internal/lsp/testdata/bad/badimport.go

@@ -1,5 +1,5 @@
 package bad
 
 import (
-	_ "nosuchpkg" //@diag("_", "LSP", "could not import nosuchpkg (no package data for import path nosuchpkg)")
+	_ "nosuchpkg" //@diag("_", "compiler", "could not import nosuchpkg (no package data for import path nosuchpkg)")
 )

internal/lsp/testdata/badstmt/badstmt.go.in

@@ -5,7 +5,7 @@ import (
 )
 
 func _() {
-	defer foo.F //@complete(" //", Foo),diag(" //", "LSP", "function must be invoked in defer statement")
+	defer foo.F //@complete(" //", Foo),diag(" //", "syntax", "function must be invoked in defer statement")
 	y := 1
 	defer foo.F //@complete(" //", Foo)
 }

internal/lsp/testdata/format/bad_format.go.golden

@@ -9,7 +9,7 @@ import (
 
 func hello() {
 
-	var x int //@diag("x", "LSP", "x declared but not used")
+	var x int //@diag("x", "compiler", "x declared but not used")
 }
 
 func hi() {

internal/lsp/testdata/format/bad_format.go.in

@@ -11,7 +11,7 @@ func hello() {
 
 
 
-	var x int //@diag("x", "LSP", "x declared but not used")
+	var x int //@diag("x", "compiler", "x declared but not used")
 }
 
 func hi() {

internal/lsp/testdata/generated/generated.go

@@ -3,5 +3,5 @@ package generated
 // Code generated by generator.go. DO NOT EDIT.
 
 func _() {
-	var y int //@diag("y", "LSP", "y declared but not used")
+	var y int //@diag("y", "compiler", "y declared but not used")
 }

internal/lsp/testdata/generated/generator.go

@@ -1,5 +1,5 @@
 package generated
 
 func _() {
-	var x int //@diag("x", "LSP", "x declared but not used")
+	var x int //@diag("x", "compiler", "x declared but not used")
 }

internal/lsp/testdata/noparse/noparse.go.in

@@ -8,4 +8,4 @@ func stuff() {
 	x := 5
 }
 
-func .() {} //@diag(".", "LSP", "expected 'IDENT', found '.'")
+func .() {} //@diag(".", "syntax", "expected 'IDENT', found '.'")

internal/lsp/testdata/noparse_format/noparse_format.go.in

@@ -4,6 +4,6 @@ package noparse_format //@format("package")
 
 func what() {
 	var b int
-	if {		hi() //@diag("{", "LSP", "missing condition in if statement")
+	if {		hi() //@diag("{", "syntax", "missing condition in if statement")
 	}
 }

internal/lsp/testdata/testy/testy_test.go

@@ -3,6 +3,6 @@ package testy
 import "testing"
 
 func TestSomething(t *testing.T) { //@item(TestSomething, "TestSomething(t *testing.T)", "", "func")
-	var x int //@mark(testyX, "x"),diag("x", "LSP", "x declared but not used"),refs("x", testyX)
+	var x int //@mark(testyX, "x"),diag("x", "compiler", "x declared but not used"),refs("x", testyX)
 	a()       //@mark(testyA, "a")
 }