internal/lsp: add error handling for self imports

This change will provide a more useful error when you
are self importing a package. It has TODOs in place to propagate the
"import cycle not allowed" error from go list to the user.

Updates golang/go#33085

Change-Id: Ia868a7c688b0f0a7a9689cfda5ea8cea8ae1faff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209857
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/check.go

@@ -331,18 +331,15 @@ func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode sourc
 		return nil, ctx.Err()
 	}
 
-	// We don't care about a package's errors unless we have parsed it in full.
-	if mode == source.ParseFull {
-		for _, e := range rawErrors {
-			srcErr, err := sourceError(ctx, fset, pkg, e)
-			if err != nil {
-				log.Error(ctx, "unable to compute error positions", err, telemetry.Package.Of(pkg.ID()))
-				continue
-			}
-			pkg.errors = append(pkg.errors, srcErr)
+	// TODO(golang/go#35964): Propagate `go list` errors here when go/packages supports it.
+	for _, e := range rawErrors {
+		srcErr, err := sourceError(ctx, fset, pkg, e)
+		if err != nil {
+			log.Error(ctx, "unable to compute error positions", err, telemetry.Package.Of(pkg.ID()))
+			continue
 		}
+		pkg.errors = append(pkg.errors, srcErr)
 	}
-
 	return pkg, nil
 }
 

internal/lsp/cache/errors.go

@@ -38,6 +38,14 @@ func sourceError(ctx context.Context, fset *token.FileSet, pkg *pkg, e interface
 		msg = e.Msg
 		kind = toSourceErrorKind(e.Kind)
 
+		// If the range can't be derived from the parseGoListError function, then we do not have a valid position.
+		if _, err := spanToRange(ctx, pkg, spn); err != nil && e.Pos == "" {
+			return &source.Error{
+				Message: msg,
+				Kind:    kind,
+			}, nil
+		}
+
 	case *scanner.Error:
 		msg = e.Msg
 		kind = source.ParseError

internal/lsp/cmd/test/check.go

@@ -50,8 +50,8 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti
 			expect = fmt.Sprintf("%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Message)
 		}
 		expect = r.NormalizePrefix(expect)
-		// Skip the badimport test for now, until we do a better job with diagnostic ranges.
-		if strings.Contains(uri.Filename(), "badimport") {
+		// Skip the badimport and import cycle not allowed test for now, until we do a better job with diagnostic ranges.
+		if strings.Contains(uri.Filename(), "badimport") || strings.Contains(expect, "import cycle") {
 			continue
 		}
 		_, found := got[expect]
@@ -62,8 +62,8 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti
 		}
 	}
 	for extra := range got {
-		// Skip the badimport test for now, until we do a better job with diagnostic ranges.
-		if strings.Contains(extra, "badimport") {
+		// Skip the badimport and import cycle not allowed test for now, until we do a better job with diagnostic ranges.
+		if strings.Contains(extra, "badimport") || strings.Contains(extra, "import cycle") {
 			continue
 		}
 		t.Errorf("extra diagnostic %q", extra)

internal/lsp/source/diagnostics.go

@@ -71,9 +71,14 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, withAnalysis bo
 	}
 	// Prepare any additional reports for the errors in this package.
 	for _, e := range pkg.GetErrors() {
-		if e.Kind != ListError {
+		// We only need to handle lower-level errors.
+		if !(e.Kind == UnknownError || e.Kind == ListError) {
 			continue
 		}
+		// If no file is associated with the error, default to the current file.
+		if e.File.URI.Filename() == "" {
+			e.File = fh.Identity()
+		}
 		clearReports(snapshot, reports, e.File)
 	}
 	// Run diagnostics for the package that this URI belongs to.
@@ -133,7 +138,7 @@ func diagnostics(ctx context.Context, snapshot Snapshot, pkg Package, reports ma
 		case TypeError:
 			set.typeErrors = append(set.typeErrors, diag)
 			diag.Source = "compiler"
-		case ListError:
+		case ListError, UnknownError:
 			set.listErrors = append(set.listErrors, diag)
 			diag.Source = "go list"
 		}

internal/lsp/testdata/circular/b/b.go

@@ -0,0 +1,9 @@
+package b //@diag("", "go list", "import cycle not allowed")
+
+import (
+	"golang.org/x/tools/internal/lsp/circular/one"
+)
+
+func Test1() {
+	one.Test()
+}

internal/lsp/testdata/circular/one/one.go

@@ -0,0 +1,9 @@
+package one
+
+import (
+	"golang.org/x/tools/internal/lsp/circular/b"
+)
+
+func Test() {
+	b.Test1()
+}

internal/lsp/testdata/circular/self.go

@@ -0,0 +1,12 @@
+package circular //@diag("", "go list", "import cycle not allowed")
+
+import (
+	"golang.org/x/tools/internal/lsp/circular"
+)
+
+func print() {
+	Test()
+}
+
+func Test() {
+}

internal/lsp/testdata/summary.txt.golden

@@ -6,7 +6,7 @@ DeepCompletionsCount = 5
 FuzzyCompletionsCount = 7
 RankedCompletionsCount = 26
 CaseSensitiveCompletionsCount = 4
-DiagnosticsCount = 22
+DiagnosticsCount = 24
 FoldingRangesCount = 2
 FormatCount = 6
 ImportCount = 7