From ed30b9180dd32a098f1d5f32e0450c4e1ea76545 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Sat, 25 Jan 2020 19:58:28 -0500 Subject: [PATCH] internal/lsp: don't show list errors unless necessary The go/packages workaround to hide errors for overlay packages doesn't seem to work well. It's easier to just hide list errors in gopls diagnostics unless the package genuinely failed to type-check. Check if the package has missing dependencies as an approximation of if it is well-typed. This required some additional special casing for the import cycle error detection, which now causes them to have duplicate diagnostics. It's a rare enough case that this doesn't concern me, but we should clean this up at some point. Fixes golang/go#36754. Change-Id: If12c92fb9a0e0b69b711ae9a509ecb1b2a32255c Reviewed-on: https://go-review.googlesource.com/c/tools/+/216310 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- go/packages/golist_overlay.go | 6 ------ go/packages/packages114_test.go | 3 --- internal/lsp/cache/errors.go | 5 ++--- internal/lsp/source/diagnostics.go | 9 ++++++--- internal/lsp/source/util.go | 3 +++ internal/lsp/testdata/circular/double/b/b.go | 2 +- internal/lsp/testdata/circular/self.go | 2 +- internal/lsp/testdata/circular/triple/a/a.go | 2 +- internal/lsp/testdata/summary.txt.golden | 2 +- 9 files changed, 15 insertions(+), 19 deletions(-) diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go index 5d088cb956..c6925c87be 100644 --- a/go/packages/golist_overlay.go +++ b/go/packages/golist_overlay.go @@ -132,12 +132,6 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, opath) modifiedPkgsSet[pkg.ID] = true } - - // Clear out the package's errors, since we've probably corrected - // them by adding the overlay. This may eliminate some legitimate - // errors, but that's a risk with overlays in general. - pkg.Errors = nil - imports, err := extractImports(opath, contents) if err != nil { // Let the parser or type checker report errors later. diff --git a/go/packages/packages114_test.go b/go/packages/packages114_test.go index eb2fa29ac6..8786219094 100644 --- a/go/packages/packages114_test.go +++ b/go/packages/packages114_test.go @@ -74,9 +74,6 @@ func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) { if !containsFile { t.Fatalf("expected %s in CompiledGoFiles, got %v", f, d.CompiledGoFiles) } - if len(d.Errors) > 0 { - t.Fatalf("expected no errors in package, got %v", d.Errors) - } // Check value of d.D. dD := constant(d, "D") if dD == nil { diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index 30ad8c7b55..f910fc1e02 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -37,11 +37,10 @@ func sourceError(ctx context.Context, fset *token.FileSet, pkg *pkg, e interface case packages.Error: kind = toSourceErrorKind(e.Kind) var ok bool - msg, spn, ok = parseGoListImportCycleError(ctx, fset, e, pkg) - if ok { + if msg, spn, ok = parseGoListImportCycleError(ctx, fset, e, pkg); ok { + kind = source.TypeError break } - if e.Pos == "" { spn = parseGoListError(e.Msg) diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index bafa6e38cc..9e048985d4 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -82,7 +82,7 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, withA } } // Run diagnostics for the package that this URI belongs to. - hadDiagnostics, err := diagnostics(ctx, snapshot, reports, pkg) + hadDiagnostics, err := diagnostics(ctx, snapshot, reports, pkg, len(ph.MissingDependencies()) > 0) if err != nil { return nil, warn, err } @@ -127,7 +127,7 @@ type diagnosticSet struct { listErrors, parseErrors, typeErrors []*Diagnostic } -func diagnostics(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]Diagnostic, pkg Package) (bool, error) { +func diagnostics(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]Diagnostic, pkg Package, hasMissingDeps bool) (bool, error) { ctx, done := trace.StartSpan(ctx, "source.diagnostics", telemetry.Package.Of(pkg.ID())) _ = ctx // circumvent SA4006 defer done() @@ -163,7 +163,10 @@ func diagnostics(ctx context.Context, snapshot Snapshot, reports map[FileIdentit if len(set.parseErrors) > 0 { diags = set.parseErrors } else if len(set.listErrors) > 0 { - diags = set.listErrors + // Only show list errors if the package has missing dependencies. + if hasMissingDeps { + diags = set.listErrors + } } if len(diags) > 0 { nonEmptyDiagnostics = true diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index d4c1f5463c..34e63176f2 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -609,6 +609,9 @@ func CompareDiagnostic(a, b Diagnostic) int { if r := protocol.CompareRange(a.Range, b.Range); r != 0 { return r } + if a.Source < b.Source { + return -1 + } if a.Message < b.Message { return -1 } diff --git a/internal/lsp/testdata/circular/double/b/b.go b/internal/lsp/testdata/circular/double/b/b.go index dea86f1eda..b99c92df6f 100644 --- a/internal/lsp/testdata/circular/double/b/b.go +++ b/internal/lsp/testdata/circular/double/b/b.go @@ -1,5 +1,5 @@ package b import ( - _ "golang.org/x/tools/internal/lsp/circular/double/one" //@diag("_ \"golang.org/x/tools/internal/lsp/circular/double/one\"", "go list", "import cycle not allowed") + _ "golang.org/x/tools/internal/lsp/circular/double/one" //@diag("_ \"golang.org/x/tools/internal/lsp/circular/double/one\"", "compiler", "import cycle not allowed"),diag("\"golang.org/x/tools/internal/lsp/circular/double/one\"", "compiler", "could not import golang.org/x/tools/internal/lsp/circular/double/one (no package for import golang.org/x/tools/internal/lsp/circular/double/one)") ) diff --git a/internal/lsp/testdata/circular/self.go b/internal/lsp/testdata/circular/self.go index 84676593b0..d0cb5b6a53 100644 --- a/internal/lsp/testdata/circular/self.go +++ b/internal/lsp/testdata/circular/self.go @@ -1,5 +1,5 @@ package circular import ( - _ "golang.org/x/tools/internal/lsp/circular" //@diag("_ \"golang.org/x/tools/internal/lsp/circular\"", "go list", "import cycle not allowed") + _ "golang.org/x/tools/internal/lsp/circular" //@diag("_ \"golang.org/x/tools/internal/lsp/circular\"", "compiler", "import cycle not allowed"),diag("\"golang.org/x/tools/internal/lsp/circular\"", "compiler", "could not import golang.org/x/tools/internal/lsp/circular (no package for import golang.org/x/tools/internal/lsp/circular)") ) diff --git a/internal/lsp/testdata/circular/triple/a/a.go b/internal/lsp/testdata/circular/triple/a/a.go index 48fd0bb5b2..137c2776e7 100644 --- a/internal/lsp/testdata/circular/triple/a/a.go +++ b/internal/lsp/testdata/circular/triple/a/a.go @@ -1,5 +1,5 @@ package a import ( - _ "golang.org/x/tools/internal/lsp/circular/triple/b" //@diag("_ \"golang.org/x/tools/internal/lsp/circular/triple/b\"", "go list", "import cycle not allowed") + _ "golang.org/x/tools/internal/lsp/circular/triple/b" //@diag("_ \"golang.org/x/tools/internal/lsp/circular/triple/b\"", "compiler", "import cycle not allowed") ) diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index efb78fc4dd..bbb39094cd 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/internal/lsp/testdata/summary.txt.golden @@ -6,7 +6,7 @@ DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 RankedCompletionsCount = 66 CaseSensitiveCompletionsCount = 4 -DiagnosticsCount = 35 +DiagnosticsCount = 37 FoldingRangesCount = 2 FormatCount = 6 ImportCount = 7