internal/lsp: refactor go command error handling

Our handling of go command errors was cobbled together, leading to
unexpected gaps and duplication. Refactor it to be more coherent.

Our goal is to turn every go command error into a diagnostic in the
relevant location. The errors don't contain error positions, so we have
to guess where they belong using the module names mentioned in the
error. If we can't find any reference to those modules, we are forced to
add diagnostics to all go.mod files.

I may have destroyed the intent of TestMultiModule_OneBrokenModule but
I'm not sure what to do about it.

Some cleanup along the way:
- Stop parsing modfile.Parse error text: it returns structured errors
and we can just use them.
- Return CriticalErrors from awaitLoadedAllErrors, and do error
extraction lower in the stack. This prevents a ridiculous situation
where initialize formed a CriticalError, then awaitLoadedAllErrors
returned just its MainError, and then GetCriticalError parsed out
a new CriticalError from the MainError we got from a CriticalError.
- In initialize, return modDiagnostics even if load succeeds: we are
missing packages and should not silently fail, I think?
- During testing I tripped over ApplyQuickFixes' willingness to not
actually do anything, so I made that an error.

Fixes golang/go#44132.
I may also have fixed golang/go#44204 but I haven't checked.

Change-Id: Ibf819d0f044d4f99795978a28b18915893e50c88
Reviewed-on: https://go-review.googlesource.com/c/tools/+/291192
Trust: Heschi Kreinick <[email protected]>
Run-TryBot: Heschi Kreinick <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: heschi

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -16,7 +16,6 @@ import (
 	"golang.org/x/tools/internal/lsp"
 	"golang.org/x/tools/internal/lsp/fake"
 	"golang.org/x/tools/internal/lsp/protocol"
-	"golang.org/x/tools/internal/lsp/tests"
 	"golang.org/x/tools/internal/testenv"
 )
 
@@ -526,20 +525,15 @@ func _() {
 `
 	Run(t, generated, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
-		original := env.ReadWorkspaceFile("main.go")
 		var d protocol.PublishDiagnosticsParams
 		env.Await(
 			OnceMet(
 				DiagnosticAt("main.go", 5, 8),
 				ReadDiagnostics("main.go", &d),
 			),
 		)
-		// Apply fixes and save the buffer.
-		env.ApplyQuickFixes("main.go", d.Diagnostics)
-		env.SaveBuffer("main.go")
-		fixed := env.ReadWorkspaceFile("main.go")
-		if original != fixed {
-			t.Fatalf("generated file was changed by quick fixes:\n%s", tests.Diff(t, original, fixed))
+		if fixes := env.GetQuickFixes("main.go", d.Diagnostics); len(fixes) != 0 {
+			t.Errorf("got quick fixes %v, wanted none", fixes)
 		}
 	})
 }

gopls/internal/regtest/expectation.go

@@ -12,6 +12,7 @@ import (
 	"golang.org/x/tools/internal/lsp"
 	"golang.org/x/tools/internal/lsp/fake"
 	"golang.org/x/tools/internal/lsp/protocol"
+	"golang.org/x/tools/internal/testenv"
 )
 
 // An Expectation asserts that the state of the editor at a point in time
@@ -580,3 +581,16 @@ func NoDiagnosticAt(name string, line, col int) DiagnosticExpectation {
 func NoDiagnosticWithMessage(name, msg string) DiagnosticExpectation {
 	return DiagnosticExpectation{path: name, message: msg, present: false}
 }
+
+// GoSum asserts that a "go.sum is out of sync" diagnostic for the given module
+// (as formatted in a go.mod file, e.g. "example.com v1.0.0") is present.
+func (e *Env) GoSumDiagnostic(name, module string) Expectation {
+	e.T.Helper()
+	// In 1.16, go.sum diagnostics should appear on the relevant module. Earlier
+	// errors have no information and appear on the module declaration.
+	if testenv.Go1Point() >= 16 {
+		return e.DiagnosticAtRegexpWithMessage(name, module, "go.sum is out of sync")
+	} else {
+		return e.DiagnosticAtRegexpWithMessage(name, `module`, "go.sum is out of sync")
+	}
+}

gopls/internal/regtest/modfile/modfile_test.go

@@ -548,6 +548,7 @@ func main() {
 			d := protocol.PublishDiagnosticsParams{}
 			env.Await(
 				OnceMet(
+					// Make sure the diagnostic mentions the new version -- the old diagnostic is in the same place.
 					env.DiagnosticAtRegexpWithMessage("a/go.mod", "example.com v1.2.3", "[email protected]"),
 					ReadDiagnostics("a/go.mod", &d),
 				),
@@ -822,7 +823,7 @@ func main() {
 		env.OpenFile("go.mod")
 		env.Await(
 			OnceMet(
-				DiagnosticAt("go.mod", 0, 0),
+				env.GoSumDiagnostic("go.mod", `example.com v1.2.3`),
 				ReadDiagnostics("go.mod", d),
 			),
 		)
@@ -1001,9 +1002,7 @@ require random.com v1.2.3
 }
 
 func TestSumUpdateQuickFix(t *testing.T) {
-	// Error messages changed in 1.16 that changed the diagnostic positions.
-	testenv.NeedsGo1Point(t, 16)
-
+	testenv.NeedsGo1Point(t, 14)
 	const mod = `
 -- go.mod --
 module mod.com
@@ -1030,22 +1029,14 @@ func main() {
 		Modes(Singleton),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("go.mod")
-		pos := env.RegexpSearch("go.mod", "example.com")
 		params := &protocol.PublishDiagnosticsParams{}
 		env.Await(
 			OnceMet(
-				env.DiagnosticAtRegexp("go.mod", "example.com"),
+				env.GoSumDiagnostic("go.mod", "example.com"),
 				ReadDiagnostics("go.mod", params),
 			),
 		)
-		var diagnostic protocol.Diagnostic
-		for _, d := range params.Diagnostics {
-			if d.Range.Start.Line == uint32(pos.Line) {
-				diagnostic = d
-				break
-			}
-		}
-		env.ApplyQuickFixes("go.mod", []protocol.Diagnostic{diagnostic})
+		env.ApplyQuickFixes("go.mod", params.Diagnostics)
 		const want = `example.com v1.2.3 h1:Yryq11hF02fEf2JlOS2eph+ICE2/ceevGV3C9dl5V/c=
 example.com v1.2.3/go.mod h1:Y2Rc5rVWjWur0h3pd9aEvK5Pof8YKDANh9gHA2Maujo=
 `

gopls/internal/regtest/workspace/workspace_test.go

@@ -818,25 +818,25 @@ func main() {
 		Modes(Experimental),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		params := &protocol.PublishDiagnosticsParams{}
-		env.OpenFile("a/go.mod")
+		env.OpenFile("b/go.mod")
 		env.Await(
-			ReadDiagnostics("a/go.mod", params),
+			OnceMet(
+				env.GoSumDiagnostic("b/go.mod", `example.com v1.2.3`),
+				ReadDiagnostics("b/go.mod", params),
+			),
 		)
 		for _, d := range params.Diagnostics {
-			if d.Message != `go.sum is out of sync with go.mod. Please update it by applying the quick fix.` {
+			if !strings.Contains(d.Message, "go.sum is out of sync") {
 				continue
 			}
-			actions, err := env.Editor.GetQuickFixes(env.Ctx, "a/go.mod", nil, []protocol.Diagnostic{d})
-			if err != nil {
-				t.Fatal(err)
-			}
+			actions := env.GetQuickFixes("b/go.mod", []protocol.Diagnostic{d})
 			if len(actions) != 2 {
 				t.Fatalf("expected 2 code actions, got %v", len(actions))
 			}
-			env.ApplyQuickFixes("a/go.mod", []protocol.Diagnostic{d})
+			env.ApplyQuickFixes("b/go.mod", []protocol.Diagnostic{d})
 		}
 		env.Await(
-			EmptyDiagnostics("a/go.mod"),
+			EmptyDiagnostics("b/go.mod"),
 		)
 	})
 }

gopls/internal/regtest/wrappers.go

@@ -200,6 +200,16 @@ func (e *Env) ApplyQuickFixes(path string, diagnostics []protocol.Diagnostic) {
 	}
 }
 
+// GetQuickFixes returns the available quick fix code actions.
+func (e *Env) GetQuickFixes(path string, diagnostics []protocol.Diagnostic) []protocol.CodeAction {
+	e.T.Helper()
+	actions, err := e.Editor.GetQuickFixes(e.Ctx, path, nil, diagnostics)
+	if err != nil {
+		e.T.Fatal(err)
+	}
+	return actions
+}
+
 // Hover in the editor, calling t.Fatal on any error.
 func (e *Env) Hover(name string, pos fake.Pos) (*protocol.MarkupContent, fake.Pos) {
 	e.T.Helper()
@@ -265,6 +275,8 @@ func (e *Env) RunGoCommandInDir(dir, verb string, args ...string) {
 	}
 }
 
+// DumpGoSum prints the correct go.sum contents for dir in txtar format,
+// for use in creating regtests.
 func (e *Env) DumpGoSum(dir string) {
 	e.T.Helper()