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) {