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 <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>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index 3a73152..84b8b3d 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -16,7 +16,6 @@
"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 @@
`
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 @@
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 e520099..02e00dd 100644
--- a/gopls/internal/regtest/expectation.go
+++ b/gopls/internal/regtest/expectation.go
@@ -12,6 +12,7 @@
"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 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 0f897e7..a214a16 100644
--- a/gopls/internal/regtest/modfile/modfile_test.go
+++ b/gopls/internal/regtest/modfile/modfile_test.go
@@ -548,6 +548,7 @@
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 @@
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 @@
}
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 @@
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 8712784..f0128d8 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -818,25 +818,25 @@
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 fa9367e..c77de5b 100644
--- a/gopls/internal/regtest/wrappers.go
+++ b/gopls/internal/regtest/wrappers.go
@@ -200,6 +200,16 @@
}
}
+// 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 @@
}
}
+// 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 9112bfd..2e1917a 100644
--- a/internal/lsp/cache/mod.go
+++ b/internal/lsp/cache/mod.go
@@ -60,13 +60,26 @@
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 @@
File: file,
ParseErrors: parseErrors,
},
- err: err,
+ err: parseErr,
}
}, nil)
@@ -214,46 +227,67 @@
// 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 @@
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 @@
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 @@
}
return nil
}
-
-// errorPositionRe matches errors messages of the form <filename>:<line>:<col>,
-// where the <col> is optional.
-var errorPositionRe = regexp.MustCompile(`(?P<pos>.*:([\d]+)(:([\d]+))?): (?P<msg>.+)`)
-
-// 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 1b7ef52..6891a39 100644
--- a/internal/lsp/cache/mod_tidy.go
+++ b/internal/lsp/cache/mod_tidy.go
@@ -151,74 +151,6 @@
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 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 @@
}
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 0f44cab..ddfefc7 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -1022,22 +1022,22 @@
}
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 @@
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 @@
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 e71a84c..64a9e7e 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -573,15 +573,15 @@
}
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 fb788f4..cb57182 100644
--- a/internal/lsp/cache/view_test.go
+++ b/internal/lsp/cache/view_test.go
@@ -8,12 +8,9 @@
"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 @@
}
}
-// 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 96410d9..58a13ec 100644
--- a/internal/lsp/fake/editor.go
+++ b/internal/lsp/fake/editor.go
@@ -747,17 +747,26 @@
// 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 @@
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 @@
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 @@
}
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 @@
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) {
