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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

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.

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 {

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)
 

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

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
 	}

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

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

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

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