internal/lsp/analysis/fillreturns: be defensive w.r.t. type errors

In the presence of type errors, TypeOf may return nil, and this appears
to have contributed to a crash in the fillreturns analysis. I haven't
been able to find or deduce a test case, but this change makes the logic
more defensive.

Also remove a stale pre-go1.17 test that used to trigger a panic
(the one fixed here? unclear) to assert that panics were recoverable.

Updates golang/go#54655

Change-Id: Ic9ca9a307eede50a2d4db6424822a155dd43e635
Reviewed-on: https://go-review.googlesource.com/c/tools/+/426019
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -1868,37 +1868,6 @@ package main
 	})
 }
 
-// Tests golang/go#45075: A panic in fillreturns broke diagnostics.
-// Expect an error log indicating that fillreturns panicked, as well type
-// errors for the broken code.
-func TestFillReturnsPanic(t *testing.T) {
-	// At tip, the panic no longer reproduces.
-	testenv.SkipAfterGo1Point(t, 16)
-
-	const files = `
--- go.mod --
-module mod.com
-
-go 1.15
--- main.go --
-package main
-
-func foo() int {
-	return x, nil
-}
-`
-	Run(t, files, func(t *testing.T, env *Env) {
-		env.OpenFile("main.go")
-		env.Await(
-			OnceMet(
-				env.DoneWithOpen(),
-				LogMatching(protocol.Error, `.*analysis fillreturns.*panicked.*`, 1, true),
-				env.DiagnosticAtRegexpWithMessage("main.go", `return x`, "wrong number of return values"),
-			),
-		)
-	})
-}
-
 // This test confirms that the view does not reinitialize when a go.mod file is
 // opened.
 func TestNoReinitialize(t *testing.T) {

internal/lsp/analysis/fillreturns/fillreturns.go

@@ -71,6 +71,8 @@ outer:
 		}
 
 		// Get the end position of the error.
+		// (This heuristic assumes that the buffer is formatted,
+		// at least up to the end position of the error.)
 		var buf bytes.Buffer
 		if err := format.Node(&buf, pass.Fset, file); err != nil {
 			continue
@@ -156,22 +158,24 @@ outer:
 		fixed := make([]ast.Expr, len(enclosingFunc.Results.List))
 
 		// For each value in the return function declaration, find the leftmost element
-		// in the return statement that has the desired type. If no such element exits,
+		// in the return statement that has the desired type. If no such element exists,
 		// fill in the missing value with the appropriate "zero" value.
+		// Beware that type information may be incomplete.
 		var retTyps []types.Type
 		for _, ret := range enclosingFunc.Results.List {
-			retTyps = append(retTyps, info.TypeOf(ret.Type))
+			retTyp := info.TypeOf(ret.Type)
+			if retTyp == nil {
+				return nil, nil
+			}
+			retTyps = append(retTyps, retTyp)
 		}
 		matches :=
 			analysisinternal.FindMatchingIdents(retTyps, file, ret.Pos(), info, pass.Pkg)
 		for i, retTyp := range retTyps {
 			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) {
+				if t := info.TypeOf(val); t == nil || !matchingTypes(t, retTyp) {
 					continue
 				}
 				if !analysisinternal.IsZeroValue(val) {