internal/lsp: fix suppressed panic in analyzer

This change ensures that the End position provided to span.NewRange
in suggestedAnalysisFixes is valid even when the diagnostic has
only a valid start position. This seems to be the cause of
some panics observed in the ARM builders in the attached issue.

Also, reduce the scope of the recover operation to just the analyzer's
run method: we don't want to hide further bugs (or discard stack traces)
in the setup or postprocessing logic.

Also:
- split a single assertion in span.NewRange into two.
- Add information to various error messages to help identify causes.
- Add TODO comments about inconsistent treatment of token.File
  in span.FileSpan, and temporarily remove bug.Errorf that
  is obviously reachable from valid inputs.
- Add TODO to fix another panic in an analyzer that is covered
  by our tests but was hitherto suppressed.
- Add TODO to use bug.Errorf after recover to prevent recurrences.
  We can't do that until the previous panic is fixed.

Updates https://github.com/golang/go/issues/54655

Change-Id: I0576d03fcfffe0c8df157cf6c6520c9d402f8803
Reviewed-on: https://go-review.googlesource.com/c/tools/+/425356
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>

3 files changed, 35 insertions, 7 deletions

diff --git a/internal/lsp/analysis/fillreturns/fillreturns.go b/internal/lsp/analysis/fillreturns/fillreturns.go
index 705ae124d..ab2fa8691 100644
--- a/internal/lsp/analysis/fillreturns/fillreturns.go
+++ b/internal/lsp/analysis/fillreturns/fillreturns.go
@@ -168,6 +168,9 @@ outer:
 			var match ast.Expr
 			var idx int
 			for j, val := range remaining {
+				// TODO(adonovan): if TypeOf returns nil (as it may, since we
+				// RunDespiteErrors), then matchingTypes will panic in
+				// types.AssignableTo. Fix it, and audit for other instances.
 				if !matchingTypes(info.TypeOf(val), retTyp) {
 					continue
 				}
diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go
index ca0e04d64..3ddc3a011 100644
--- a/internal/lsp/cache/analysis.go
+++ b/internal/lsp/cache/analysis.go
@@ -242,11 +242,6 @@ func runAnalysis(ctx context.Context, snapshot *snapshot, analyzer *analysis.Ana
 		objectFacts:  make(map[objectFactKey]analysis.Fact),
 		packageFacts: make(map[packageFactKey]analysis.Fact),
 	}
-	defer func() {
-		if r := recover(); r != nil {
-			data.err = fmt.Errorf("analysis %s for package %s panicked: %v", analyzer.Name, pkg.PkgPath(), r)
-		}
-	}()
 
 	// Plumb the output values of the dependencies
 	// into the inputs of this action.  Also facts.
@@ -362,7 +357,20 @@ func runAnalysis(ctx context.Context, snapshot *snapshot, analyzer *analysis.Ana
 		data.err = fmt.Errorf("analysis skipped due to errors in package")
 		return data
 	}
-	data.result, data.err = pass.Analyzer.Run(pass)
+
+	// Recover from panics (only) within the analyzer logic.
+	// (Use an anonymous function to limit the recover scope.)
+	func() {
+		defer func() {
+			if r := recover(); r != nil {
+				// TODO(adonovan): use bug.Errorf here so that we
+				// detect crashes covered by our test suite.
+				// e.g.
+				data.err = fmt.Errorf("analysis %s for package %s panicked: %v", analyzer.Name, pkg.PkgPath(), r)
+			}
+		}()
+		data.result, data.err = pass.Analyzer.Run(pass)
+	}()
 	if data.err != nil {
 		return data
 	}
diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go
index 80ae5451c..943782989 100644
--- a/internal/lsp/cache/errors.go
+++ b/internal/lsp/cache/errors.go
@@ -298,7 +298,11 @@ func suggestedAnalysisFixes(snapshot *snapshot, pkg *pkg, diag *analysis.Diagnos
 			if tokFile == nil {
 				return nil, bug.Errorf("no file for edit position")
 			}
-			spn, err := span.NewRange(tokFile, e.Pos, e.End).Span()
+			end := e.End
+			if !end.IsValid() {
+				end = e.Pos
+			}
+			spn, err := span.NewRange(tokFile, e.Pos, end).Span()
 			if err != nil {
 				return nil, err
 			}
@@ -353,7 +357,20 @@ func typeErrorData(fset *token.FileSet, pkg *pkg, terr types.Error) (typesintern
 		start, end = terr.Pos, terr.Pos
 		ecode = 0
 	}
+	// go/types may return invalid positions in some cases, such as
+	// in errors on tokens missing from the syntax tree.
+	if !start.IsValid() {
+		return 0, span.Span{}, fmt.Errorf("type error (%q, code %d, go116=%t) without position", terr.Msg, ecode, ok)
+	}
+	// go/types errors retain their FileSet.
+	// Sanity-check that we're using the right one.
+	if fset != terr.Fset {
+		return 0, span.Span{}, bug.Errorf("wrong FileSet for type error")
+	}
 	posn := fset.Position(start)
+	if !posn.IsValid() {
+		return 0, span.Span{}, fmt.Errorf("position %d of type error %q (code %q) not found in FileSet", start, start, terr)
+	}
 	pgf, err := pkg.File(span.URIFromPath(posn.Filename))
 	if err != nil {
 		return 0, span.Span{}, err