From 7a079fcd7976efee71404a62f2b2be0bbc34f3a5 Mon Sep 17 00:00:00 2001
From: Heschi Kreinick <heschi@google.com>
Date: Fri, 19 Feb 2021 16:30:51 -0500
Subject: [PATCH] internal/lsp/cache: fix related error processing

Due to misuse of the expandErrors function, when we encountered a
continuing error, we would group all the related errors into a single
one, then return both it and all its secondaries individually. That
makes for a strange user experience where you get one complete error and
N-1 incomplete ones.

Instead, create a copy of the complete error for each secondary error,
moving the copy to the location of the secondary error and adding a bit
to the text to (hopefully) clarify what's going on.

Change-Id: I87cf0e3b2cea98317f650d16a78476edf108a934
Reviewed-on: https://go-review.googlesource.com/c/tools/+/295413
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
---
 gopls/internal/regtest/misc/failures_test.go | 14 ++-----
 internal/lsp/cache/check.go                  | 43 +++++++++++++++-----
 2 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/gopls/internal/regtest/misc/failures_test.go b/gopls/internal/regtest/misc/failures_test.go
index 68bbded8c4..41a833ef00 100644
--- a/gopls/internal/regtest/misc/failures_test.go
+++ b/gopls/internal/regtest/misc/failures_test.go
@@ -5,7 +5,6 @@
 package misc
 
 import (
-	"log"
 	"testing"
 
 	. "golang.org/x/tools/gopls/internal/regtest"
@@ -59,18 +58,13 @@ const a = 2
 
 func TestFailingDiagnosticClearingOnEdit(t *testing.T) {
 	Run(t, badPackageDup, func(t *testing.T, env *Env) {
-		log.SetFlags(log.Lshortfile)
 		env.OpenFile("b.go")
-		env.Await(env.AnyDiagnosticAtCurrentVersion("a.go"))
-		// no diagnostics for either b.go or 'gen.go', but there should be
-		env.Await(NoDiagnostics("b.go"))
+		// no diagnostics for any files, but there should be
+		env.Await(NoDiagnostics("a.go"), NoDiagnostics("b.go"))
 
 		// Fix the error by editing the const name in b.go to `b`.
 		env.RegexpReplace("b.go", "(a) = 2", "b")
-		env.Await(
-			EmptyDiagnostics("a.go"),
-			// expected, as there have never been any diagnostics for b.go
-			NoDiagnostics("b.go"),
-		)
+
+		// The diagnostics that weren't sent above should now be cleared.
 	})
 }
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 5940651f75..0c50d610eb 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -442,8 +442,7 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source
 
 	// We don't care about a package's errors unless we have parsed it in full.
 	if mode == source.ParseFull {
-		expandErrors(rawErrors)
-		for _, e := range rawErrors {
+		for _, e := range expandErrors(rawErrors) {
 			srcDiags, err := sourceDiagnostics(ctx, snapshot, pkg, protocol.SeverityError, e)
 			if err != nil {
 				event.Error(ctx, "unable to compute error positions", err, tag.Package.Of(pkg.ID()))
@@ -654,27 +653,51 @@ func (e extendedError) Error() string {
 // This code associates the secondary error with its primary error, which can
 // then be used as RelatedInformation when the error becomes a diagnostic.
 func expandErrors(errs []error) []error {
+	var result []error
 	for i := 0; i < len(errs); {
 		e, ok := errs[i].(types.Error)
 		if !ok {
+			result = append(result, errs[i])
 			i++
 			continue
 		}
-		enew := extendedError{
+		original := extendedError{
 			primary: e,
 		}
-		j := i + 1
-		for ; j < len(errs); j++ {
-			spl, ok := errs[j].(types.Error)
+		for i++; i < len(errs); i++ {
+			spl, ok := errs[i].(types.Error)
 			if !ok || len(spl.Msg) == 0 || spl.Msg[0] != '\t' {
 				break
 			}
-			enew.secondaries = append(enew.secondaries, spl)
+			spl.Msg = spl.Msg[1:]
+			original.secondaries = append(original.secondaries, spl)
 		}
-		errs[i] = enew
-		i = j
+
+		// Clone the error to all its related locations -- VS Code, at least,
+		// doesn't do it for us.
+		result = append(result, original)
+		for i, mainSecondary := range original.secondaries {
+			// Create the new primary error, with a tweaked message, in the
+			// secondary's location. We need to start from the secondary to
+			// capture its unexported location fields.
+			relocatedSecondary := mainSecondary
+			relocatedSecondary.Msg = fmt.Sprintf("%v (full error below)", original.primary.Msg)
+			relocatedSecondary.Soft = original.primary.Soft
+
+			// Copy over the secondary errors, noting the location of the
+			// current error we're cloning.
+			clonedError := extendedError{primary: relocatedSecondary, secondaries: []types.Error{original.primary}}
+			for j, secondary := range original.secondaries {
+				if i == j {
+					secondary.Msg += " (this error)"
+				}
+				clonedError.secondaries = append(clonedError.secondaries, secondary)
+			}
+			result = append(result, clonedError)
+		}
+
 	}
-	return errs
+	return result
 }
 
 // resolveImportPath resolves an import path in pkg to a package from deps.