From 1e7abacf3b50c924a87de1fb83a8fb62b44c374f Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 9 Feb 2021 21:57:43 -0500 Subject: [PATCH] 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 Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler Reviewed-by: Robert Findley --- .../regtest/diagnostics/diagnostics_test.go | 10 +- gopls/internal/regtest/expectation.go | 14 + .../internal/regtest/modfile/modfile_test.go | 19 +- .../regtest/workspace/workspace_test.go | 18 +- gopls/internal/regtest/wrappers.go | 12 + internal/lsp/cache/mod.go | 272 ++++++++++-------- internal/lsp/cache/mod_tidy.go | 82 +----- internal/lsp/cache/snapshot.go | 37 +-- internal/lsp/cache/view.go | 18 +- internal/lsp/cache/view_test.go | 85 ------ internal/lsp/fake/editor.go | 31 +- 11 files changed, 257 insertions(+), 341 deletions(-) diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index 3a731527ad..84b8b3dba2 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/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,7 +525,6 @@ 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( @@ -534,12 +532,8 @@ func _() { 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) } }) } diff --git a/gopls/internal/regtest/expectation.go b/gopls/internal/regtest/expectation.go index e520099868..02e00dd3f3 100644 --- a/gopls/internal/regtest/expectation.go +++ b/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") + } +} diff --git a/gopls/internal/regtest/modfile/modfile_test.go b/gopls/internal/regtest/modfile/modfile_test.go index 0f897e772c..a214a16034 100644 --- a/gopls/internal/regtest/modfile/modfile_test.go +++ b/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", "example.com@v1.2.3"), 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= ` diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index 8712784626..f0128d86cb 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/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"), ) }) } diff --git a/gopls/internal/regtest/wrappers.go b/gopls/internal/regtest/wrappers.go index fa9367e129..c77de5b71a 100644 --- a/gopls/internal/regtest/wrappers.go +++ b/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() diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index 9112bfdeb8..2e1917af08 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -60,13 +60,26 @@ func (s *snapshot) ParseMod(ctx context.Context, modFH source.FileHandle) (*sour Converter: span.NewContentConverter(modFH.URI().Filename(), contents), Content: contents, } - file, err := modfile.Parse(modFH.URI().Filename(), contents, nil) - + file, parseErr := modfile.Parse(modFH.URI().Filename(), contents, nil) // Attempt to convert the error to a standardized parse error. var parseErrors []*source.Diagnostic - if err != nil { - if parseErr := extractErrorWithPosition(ctx, err.Error(), s); parseErr != nil { - parseErrors = []*source.Diagnostic{parseErr} + if parseErr != nil { + mfErrList, ok := parseErr.(modfile.ErrorList) + if !ok { + return &parseModData{err: fmt.Errorf("unexpected parse error type %v", parseErr)} + } + for _, mfErr := range mfErrList { + rng, err := rangeFromPositions(m, mfErr.Pos, mfErr.Pos) + if err != nil { + return &parseModData{err: err} + } + parseErrors = []*source.Diagnostic{{ + URI: modFH.URI(), + Range: rng, + Severity: protocol.SeverityError, + Source: source.ParseError, + Message: mfErr.Err.Error(), + }} } } return &parseModData{ @@ -76,7 +89,7 @@ func (s *snapshot) ParseMod(ctx context.Context, modFH source.FileHandle) (*sour File: file, ParseErrors: parseErrors, }, - err: err, + err: parseErr, } }, nil) @@ -214,46 +227,67 @@ func (s *snapshot) ModWhy(ctx context.Context, fh source.FileHandle) (map[string // extractGoCommandError tries to parse errors that come from the go command // and shape them into go.mod diagnostics. -func (s *snapshot) extractGoCommandErrors(ctx context.Context, snapshot source.Snapshot, goCmdError string) []*source.Diagnostic { - var srcErrs []*source.Diagnostic - if srcErr := s.parseModError(ctx, goCmdError); srcErr != nil { - srcErrs = append(srcErrs, srcErr...) +func (s *snapshot) extractGoCommandErrors(ctx context.Context, goCmdError string) ([]*source.Diagnostic, error) { + diagLocations := map[*source.ParsedModule]span.Span{} + backupDiagLocations := map[*source.ParsedModule]span.Span{} + + // The go command emits parse errors for completely invalid go.mod files. + // Those are reported by our own diagnostics and can be ignored here. + // As of writing, we are not aware of any other errors that include + // file/position information, so don't even try to find it. + if strings.Contains(goCmdError, "errors parsing go.mod") { + return nil, nil } - if srcErr := extractErrorWithPosition(ctx, goCmdError, s); srcErr != nil { - srcErrs = append(srcErrs, srcErr) - } else { - for _, uri := range s.ModFiles() { - fh, err := s.GetFile(ctx, uri) - if err != nil { - continue - } - if srcErr := s.matchErrorToModule(ctx, fh, goCmdError); srcErr != nil { - srcErrs = append(srcErrs, srcErr) - } + + // Match the error against all the mod files in the workspace. + for _, uri := range s.ModFiles() { + fh, err := s.GetFile(ctx, uri) + if err != nil { + return nil, err + } + pm, err := s.ParseMod(ctx, fh) + if err != nil { + return nil, err + } + spn, found, err := s.matchErrorToModule(ctx, pm, goCmdError) + if err != nil { + return nil, err + } + if found { + diagLocations[pm] = spn + } else { + backupDiagLocations[pm] = spn } } - return srcErrs + + // If we didn't find any good matches, assign diagnostics to all go.mod files. + if len(diagLocations) == 0 { + diagLocations = backupDiagLocations + } + + var srcErrs []*source.Diagnostic + for pm, spn := range diagLocations { + diag, err := s.goCommandDiagnostic(pm, spn, goCmdError) + if err != nil { + return nil, err + } + srcErrs = append(srcErrs, diag) + } + return srcErrs, nil } var moduleVersionInErrorRe = regexp.MustCompile(`[:\s]([+-._~0-9A-Za-z]+)@([+-._~0-9A-Za-z]+)[:\s]`) -// matchErrorToModule attempts to match module version in error messages. +// matchErrorToModule matches a go command error message to a go.mod file. // Some examples: // // example.com@v1.2.2: reading example.com/@v/v1.2.2.mod: no such file or directory // go: github.com/cockroachdb/apd/v2@v2.0.72: reading github.com/cockroachdb/apd/go.mod at revision v2.0.72: unknown revision v2.0.72 // go: example.com@v1.2.3 requires\n\trandom.org@v1.2.3: parsing go.mod:\n\tmodule declares its path as: bob.org\n\tbut was required as: random.org // -// We search for module@version, starting from the end to find the most -// relevant module, e.g. random.org@v1.2.3 above. Then we associate the error -// with a directive that references any of the modules mentioned. -func (s *snapshot) matchErrorToModule(ctx context.Context, fh source.FileHandle, goCmdError string) *source.Diagnostic { - pm, err := s.ParseMod(ctx, fh) - if err != nil { - return nil - } - - var innermost *module.Version +// It returns the location of a reference to the one of the modules and true +// if one exists. If none is found it returns a fallback location and false. +func (s *snapshot) matchErrorToModule(ctx context.Context, pm *source.ParsedModule, goCmdError string) (span.Span, bool, error) { var reference *modfile.Line matches := moduleVersionInErrorRe.FindAllStringSubmatch(goCmdError, -1) @@ -267,9 +301,6 @@ func (s *snapshot) matchErrorToModule(ctx context.Context, fh source.FileHandle, if err := module.Check(ver.Path, ver.Version); err != nil { continue } - if innermost == nil { - innermost = &ver - } reference = findModuleReference(pm.File, ver) if reference != nil { break @@ -278,52 +309,112 @@ func (s *snapshot) matchErrorToModule(ctx context.Context, fh source.FileHandle, if reference == nil { // No match for the module path was found in the go.mod file. - // Show the error on the module declaration, if one exists. + // Show the error on the module declaration, if one exists, or + // just the first line of the file. if pm.File.Module == nil { - return nil + return span.New(pm.URI, span.NewPoint(1, 1, 0), span.Point{}), false, nil } - reference = pm.File.Module.Syntax + spn, err := spanFromPositions(pm.Mapper, pm.File.Module.Syntax.Start, pm.File.Module.Syntax.End) + return spn, false, err } - rng, err := rangeFromPositions(pm.Mapper, reference.Start, reference.End) + spn, err := spanFromPositions(pm.Mapper, reference.Start, reference.End) + return spn, true, err +} + +// goCommandDiagnostic creates a diagnostic for a given go command error. +func (s *snapshot) goCommandDiagnostic(pm *source.ParsedModule, spn span.Span, goCmdError string) (*source.Diagnostic, error) { + rng, err := pm.Mapper.Range(spn) if err != nil { - return nil + return nil, err } - disabledByGOPROXY := strings.Contains(goCmdError, "disabled by GOPROXY=off") - shouldAddDep := strings.Contains(goCmdError, "to add it") - if innermost != nil && (disabledByGOPROXY || shouldAddDep) { + + matches := moduleVersionInErrorRe.FindAllStringSubmatch(goCmdError, -1) + var innermost *module.Version + for i := len(matches) - 1; i >= 0; i-- { + ver := module.Version{Path: matches[i][1], Version: matches[i][2]} + // Any module versions that come from the workspace module should not + // be shown to the user. + if source.IsWorkspaceModuleVersion(ver.Version) { + continue + } + if err := module.Check(ver.Path, ver.Version); err != nil { + continue + } + innermost = &ver + break + } + + switch { + case strings.Contains(goCmdError, "inconsistent vendoring"): + cmd, err := command.NewVendorCommand("Run go mod vendor", command.URIArg{URI: protocol.URIFromSpanURI(pm.URI)}) + if err != nil { + return nil, err + } + return &source.Diagnostic{ + URI: pm.URI, + Range: rng, + Severity: protocol.SeverityError, + Source: source.ListError, + Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". +See https://github.com/golang/go/issues/39164 for more detail on this issue.`, + SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, + }, nil + + case strings.Contains(goCmdError, "updates to go.sum needed"), strings.Contains(goCmdError, "missing go.sum entry"): + var args []protocol.DocumentURI + for _, uri := range s.ModFiles() { + args = append(args, protocol.URIFromSpanURI(uri)) + } + tidyCmd, err := command.NewTidyCommand("Run go mod tidy", command.URIArgs{URIs: args}) + if err != nil { + return nil, err + } + updateCmd, err := command.NewUpdateGoSumCommand("Update go.sum", command.URIArgs{URIs: args}) + if err != nil { + return nil, err + } + msg := "go.sum is out of sync with go.mod. Please update it by applying the quick fix." + if innermost != nil { + msg = fmt.Sprintf("go.sum is out of sync with go.mod: entry for %v is missing. Please updating it by applying the quick fix.", innermost) + } + return &source.Diagnostic{ + URI: pm.URI, + Range: rng, + Severity: protocol.SeverityError, + Source: source.ListError, + Message: msg, + SuggestedFixes: []source.SuggestedFix{ + source.SuggestedFixFromCommand(tidyCmd), + source.SuggestedFixFromCommand(updateCmd), + }, + }, nil + case strings.Contains(goCmdError, "disabled by GOPROXY=off") && innermost != nil: title := fmt.Sprintf("Download %v@%v", innermost.Path, innermost.Version) cmd, err := command.NewAddDependencyCommand(title, command.DependencyArgs{ - URI: protocol.URIFromSpanURI(fh.URI()), + URI: protocol.URIFromSpanURI(pm.URI), AddRequire: false, GoCmdArgs: []string{fmt.Sprintf("%v@%v", innermost.Path, innermost.Version)}, }) if err != nil { - return nil - } - msg := goCmdError - if disabledByGOPROXY { - msg = fmt.Sprintf("%v@%v has not been downloaded", innermost.Path, innermost.Version) + return nil, err } return &source.Diagnostic{ - URI: fh.URI(), + URI: pm.URI, Range: rng, Severity: protocol.SeverityError, - Message: msg, + Message: fmt.Sprintf("%v@%v has not been downloaded", innermost.Path, innermost.Version), Source: source.ListError, SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, - } - } - diagSource := source.ListError - if fh != nil { - diagSource = source.ParseError - } - return &source.Diagnostic{ - URI: fh.URI(), - Range: rng, - Severity: protocol.SeverityError, - Source: diagSource, - Message: goCmdError, + }, nil + default: + return &source.Diagnostic{ + URI: pm.URI, + Range: rng, + Severity: protocol.SeverityError, + Source: source.ListError, + Message: goCmdError, + }, nil } } @@ -345,56 +436,3 @@ func findModuleReference(mf *modfile.File, ver module.Version) *modfile.Line { } return nil } - -// errorPositionRe matches errors messages of the form ::, -// where the is optional. -var errorPositionRe = regexp.MustCompile(`(?P.*:([\d]+)(:([\d]+))?): (?P.+)`) - -// extractErrorWithPosition returns a structured error with position -// information for the given unstructured error. If a file handle is provided, -// the error position will be on that file. This is useful for parse errors, -// where we already know the file with the error. -func extractErrorWithPosition(ctx context.Context, goCmdError string, src source.FileSource) *source.Diagnostic { - matches := errorPositionRe.FindStringSubmatch(strings.TrimSpace(goCmdError)) - if len(matches) == 0 { - return nil - } - var pos, msg string - for i, name := range errorPositionRe.SubexpNames() { - if name == "pos" { - pos = matches[i] - } - if name == "msg" { - msg = matches[i] - } - } - spn := span.Parse(pos) - fh, err := src.GetFile(ctx, spn.URI()) - if err != nil { - return nil - } - content, err := fh.Read() - if err != nil { - return nil - } - m := &protocol.ColumnMapper{ - URI: spn.URI(), - Converter: span.NewContentConverter(spn.URI().Filename(), content), - Content: content, - } - rng, err := m.Range(spn) - if err != nil { - return nil - } - diagSource := source.ListError - if fh != nil { - diagSource = source.ParseError - } - return &source.Diagnostic{ - URI: spn.URI(), - Range: rng, - Severity: protocol.SeverityError, - Source: diagSource, - Message: msg, - } -} diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index 1b7ef5262f..6891a39314 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -151,74 +151,6 @@ func (s *snapshot) ModTidy(ctx context.Context, pm *source.ParsedModule) (*sourc return mth.tidy(ctx, s) } -func (s *snapshot) parseModError(ctx context.Context, errText string) []*source.Diagnostic { - // Match on common error messages. This is really hacky, but I'm not sure - // of any better way. This can be removed when golang/go#39164 is resolved. - isInconsistentVendor := strings.Contains(errText, "inconsistent vendoring") - isGoSumUpdates := strings.Contains(errText, "updates to go.sum needed") || strings.Contains(errText, "missing go.sum entry") - - if !isInconsistentVendor && !isGoSumUpdates { - return nil - } - - switch { - case isInconsistentVendor: - uri := s.ModFiles()[0] - args := command.URIArg{URI: protocol.URIFromSpanURI(uri)} - rng, err := s.uriToModDecl(ctx, uri) - if err != nil { - return nil - } - cmd, err := command.NewVendorCommand("Run go mod vendor", args) - if err != nil { - return nil - } - return []*source.Diagnostic{{ - URI: uri, - Range: rng, - Severity: protocol.SeverityError, - Source: source.ListError, - Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". -See https://github.com/golang/go/issues/39164 for more detail on this issue.`, - SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, - }} - - case isGoSumUpdates: - var args []protocol.DocumentURI - for _, uri := range s.ModFiles() { - args = append(args, protocol.URIFromSpanURI(uri)) - } - var diagnostics []*source.Diagnostic - for _, uri := range s.ModFiles() { - rng, err := s.uriToModDecl(ctx, uri) - if err != nil { - return nil - } - tidyCmd, err := command.NewTidyCommand("Run go mod tidy", command.URIArgs{URIs: args}) - if err != nil { - return nil - } - updateCmd, err := command.NewUpdateGoSumCommand("Update go.sum", command.URIArgs{URIs: args}) - if err != nil { - return nil - } - diagnostics = append(diagnostics, &source.Diagnostic{ - URI: uri, - Range: rng, - Severity: protocol.SeverityError, - Source: source.ListError, - Message: `go.sum is out of sync with go.mod. Please update it by applying the quick fix.`, - SuggestedFixes: []source.SuggestedFix{ - source.SuggestedFixFromCommand(tidyCmd), - source.SuggestedFixFromCommand(updateCmd), - }, - }) - } - return diagnostics - default: - return nil - } -} func (s *snapshot) uriToModDecl(ctx context.Context, uri span.URI) (protocol.Range, error) { fh, err := s.GetFile(ctx, uri) @@ -546,6 +478,14 @@ func missingModuleForImport(snapshot source.Snapshot, m *protocol.ColumnMapper, } func rangeFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (protocol.Range, error) { + spn, err := spanFromPositions(m, s, e) + if err != nil { + return protocol.Range{}, err + } + return m.Range(spn) +} + +func spanFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (span.Span, error) { toPoint := func(offset int) (span.Point, error) { l, c, err := m.Converter.ToPosition(offset) if err != nil { @@ -555,11 +495,11 @@ func rangeFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (protoc } start, err := toPoint(s.Byte) if err != nil { - return protocol.Range{}, err + return span.Span{}, err } end, err := toPoint(e.Byte) if err != nil { - return protocol.Range{}, err + return span.Span{}, err } - return m.Range(span.New(m.URI, start, end)) + return span.New(m.URI, start, end), nil } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 0f44cab1c0..ddfefc7348 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1022,22 +1022,22 @@ func (s *snapshot) isOpenLocked(uri span.URI) bool { } func (s *snapshot) awaitLoaded(ctx context.Context) error { - err := s.awaitLoadedAllErrors(ctx) + loadErr := s.awaitLoadedAllErrors(ctx) // If we still have absolutely no metadata, check if the view failed to // initialize and return any errors. // TODO(rstambler): Should we clear the error after we return it? s.mu.Lock() defer s.mu.Unlock() - if len(s.metadata) == 0 { - return err + if len(s.metadata) == 0 && loadErr != nil { + return loadErr.MainError } return nil } func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError { loadErr := s.awaitLoadedAllErrors(ctx) - if errors.Is(loadErr, context.Canceled) { + if loadErr != nil && errors.Is(loadErr.MainError, context.Canceled) { return nil } @@ -1060,14 +1060,10 @@ func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError { return nil } - if strings.Contains(loadErr.Error(), "cannot find main module") { + if strings.Contains(loadErr.MainError.Error(), "cannot find main module") { return s.workspaceLayoutError(ctx) } - criticalErr := &source.CriticalError{ - MainError: loadErr, - DiagList: s.extractGoCommandErrors(ctx, s, loadErr.Error()), - } - return criticalErr + return loadErr } const adHocPackagesWarning = `You are outside of a module and outside of $GOPATH/src. @@ -1095,27 +1091,32 @@ func containsCommandLineArguments(pkgs []source.Package) bool { return false } -func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) error { +func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalError { // Do not return results until the snapshot's view has been initialized. s.AwaitInitialized(ctx) if ctx.Err() != nil { - return ctx.Err() + return &source.CriticalError{MainError: ctx.Err()} } if err := s.reloadWorkspace(ctx); err != nil { - return err + diags, _ := s.extractGoCommandErrors(ctx, err.Error()) + return &source.CriticalError{ + MainError: err, + DiagList: diags, + } } if err := s.reloadOrphanedFiles(ctx); err != nil { - return err + diags, _ := s.extractGoCommandErrors(ctx, err.Error()) + return &source.CriticalError{ + MainError: err, + DiagList: diags, + } } // TODO(rstambler): Should we be more careful about returning the // initialization error? Is it possible for the initialization error to be // corrected without a successful reinitialization? - if s.initializedErr == nil { - return nil - } - return s.initializedErr.MainError + return s.initializedErr } func (s *snapshot) AwaitInitialized(ctx context.Context) { diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index e71a84c095..64a9e7e4ab 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -573,15 +573,15 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) { } if err != nil { event.Error(ctx, "initial workspace load failed", err) - if modDiagnostics != nil { - s.initializedErr = &source.CriticalError{ - MainError: errors.Errorf("errors loading modules: %v: %w", err, modDiagnostics), - DiagList: modDiagnostics, - } - } else { - s.initializedErr = &source.CriticalError{ - MainError: err, - } + extractedDiags, _ := s.extractGoCommandErrors(ctx, err.Error()) + s.initializedErr = &source.CriticalError{ + MainError: err, + DiagList: append(modDiagnostics, extractedDiags...), + } + } else if len(modDiagnostics) != 0 { + s.initializedErr = &source.CriticalError{ + MainError: fmt.Errorf("error loading module names"), + DiagList: modDiagnostics, } } else { // Clear out the initialization error, in case it had been set diff --git a/internal/lsp/cache/view_test.go b/internal/lsp/cache/view_test.go index fb788f430f..cb57182353 100644 --- a/internal/lsp/cache/view_test.go +++ b/internal/lsp/cache/view_test.go @@ -8,12 +8,9 @@ import ( "io/ioutil" "os" "path/filepath" - "strings" "testing" - "golang.org/x/mod/modfile" "golang.org/x/tools/internal/lsp/fake" - "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" ) @@ -106,88 +103,6 @@ module fg } } -// This tests the logic used to extract positions from parse and other Go -// command errors. -func TestExtractPositionFromError(t *testing.T) { - workspace := ` --- a/go.mod -- -modul a.com --- b/go.mod -- -module b.com - -go 1.12.hello --- c/go.mod -- -module c.com - -require a.com master -` - dir, err := fake.Tempdir(workspace) - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(dir) - - tests := []struct { - filename string - wantRng protocol.Range - }{ - { - filename: "a/go.mod", - wantRng: protocol.Range{}, - }, - { - filename: "b/go.mod", - wantRng: protocol.Range{ - Start: protocol.Position{Line: 2}, - End: protocol.Position{Line: 2}, - }, - }, - { - filename: "c/go.mod", - wantRng: protocol.Range{ - Start: protocol.Position{Line: 2}, - End: protocol.Position{Line: 2}, - }, - }, - } - for _, test := range tests { - ctx := context.Background() - rel := fake.RelativeTo(dir) - uri := span.URIFromPath(rel.AbsPath(test.filename)) - if source.DetectLanguage("", uri.Filename()) != source.Mod { - t.Fatalf("expected only go.mod files") - } - // Try directly parsing the given, invalid go.mod file. Then, extract a - // position from the error message. - src := &osFileSource{} - modFH, err := src.GetFile(ctx, uri) - if err != nil { - t.Fatal(err) - } - content, err := modFH.Read() - if err != nil { - t.Fatal(err) - } - _, parseErr := modfile.Parse(uri.Filename(), content, nil) - if parseErr == nil { - t.Fatalf("%s: expected an unparseable go.mod file", uri.Filename()) - } - srcErr := extractErrorWithPosition(ctx, parseErr.Error(), src) - if srcErr == nil { - t.Fatalf("unable to extract positions from %v", parseErr.Error()) - } - if srcErr.URI != uri { - t.Errorf("unexpected URI: got %s, wanted %s", srcErr.URI, uri) - } - if protocol.CompareRange(test.wantRng, srcErr.Range) != 0 { - t.Errorf("unexpected range: got %s, wanted %s", srcErr.Range, test.wantRng) - } - if !strings.HasSuffix(parseErr.Error(), srcErr.Message) { - t.Errorf("unexpected message: got %s, wanted %s", srcErr.Message, parseErr) - } - } -} - func TestInVendor(t *testing.T) { for _, tt := range []struct { path string diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 96410d9086..58a13ec265 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -747,17 +747,26 @@ func (e *Editor) Symbol(ctx context.Context, query string) ([]SymbolInformation, // OrganizeImports requests and performs the source.organizeImports codeAction. func (e *Editor) OrganizeImports(ctx context.Context, path string) error { - return e.codeAction(ctx, path, nil, nil, protocol.SourceOrganizeImports) + _, err := e.codeAction(ctx, path, nil, nil, protocol.SourceOrganizeImports) + return err } // RefactorRewrite requests and performs the source.refactorRewrite codeAction. func (e *Editor) RefactorRewrite(ctx context.Context, path string, rng *protocol.Range) error { - return e.codeAction(ctx, path, rng, nil, protocol.RefactorRewrite) + applied, err := e.codeAction(ctx, path, rng, nil, protocol.RefactorRewrite) + if applied == 0 { + return errors.Errorf("no refactorings were applied") + } + return err } // ApplyQuickFixes requests and performs the quickfix codeAction. func (e *Editor) ApplyQuickFixes(ctx context.Context, path string, rng *protocol.Range, diagnostics []protocol.Diagnostic) error { - return e.codeAction(ctx, path, rng, diagnostics, protocol.QuickFix, protocol.SourceFixAll) + applied, err := e.codeAction(ctx, path, rng, diagnostics, protocol.QuickFix, protocol.SourceFixAll) + if applied == 0 { + return errors.Errorf("no quick fixes were applied") + } + return err } // GetQuickFixes returns the available quick fix code actions. @@ -765,14 +774,15 @@ func (e *Editor) GetQuickFixes(ctx context.Context, path string, rng *protocol.R return e.getCodeActions(ctx, path, rng, diagnostics, protocol.QuickFix, protocol.SourceFixAll) } -func (e *Editor) codeAction(ctx context.Context, path string, rng *protocol.Range, diagnostics []protocol.Diagnostic, only ...protocol.CodeActionKind) error { +func (e *Editor) codeAction(ctx context.Context, path string, rng *protocol.Range, diagnostics []protocol.Diagnostic, only ...protocol.CodeActionKind) (int, error) { actions, err := e.getCodeActions(ctx, path, rng, diagnostics, only...) if err != nil { - return err + return 0, err } + applied := 0 for _, action := range actions { if action.Title == "" { - return errors.Errorf("empty title for code action") + return 0, errors.Errorf("empty title for code action") } var match bool for _, o := range only { @@ -784,6 +794,7 @@ func (e *Editor) codeAction(ctx context.Context, path string, rng *protocol.Rang if !match { continue } + applied++ for _, change := range action.Edit.DocumentChanges { path := e.sandbox.Workdir.URIToPath(change.TextDocument.URI) if int32(e.buffers[path].version) != change.TextDocument.Version { @@ -792,7 +803,7 @@ func (e *Editor) codeAction(ctx context.Context, path string, rng *protocol.Rang } edits := convertEdits(change.Edits) if err := e.EditBuffer(ctx, path, edits); err != nil { - return errors.Errorf("editing buffer %q: %w", path, err) + return 0, errors.Errorf("editing buffer %q: %w", path, err) } } // Execute any commands. The specification says that commands are @@ -802,15 +813,15 @@ func (e *Editor) codeAction(ctx context.Context, path string, rng *protocol.Rang Command: action.Command.Command, Arguments: action.Command.Arguments, }); err != nil { - return err + return 0, err } } // Some commands may edit files on disk. if err := e.sandbox.Workdir.CheckForFileChanges(ctx); err != nil { - return err + return 0, err } } - return nil + return applied, nil } func (e *Editor) getCodeActions(ctx context.Context, path string, rng *protocol.Range, diagnostics []protocol.Diagnostic, only ...protocol.CodeActionKind) ([]protocol.CodeAction, error) {