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>
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 8bd21b8..5d637e2 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -331,18 +331,15 @@
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
}
diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go
index 9afdf6d..2504c33 100644
--- a/internal/lsp/cache/errors.go
+++ b/internal/lsp/cache/errors.go
@@ -38,6 +38,14 @@
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
diff --git a/internal/lsp/cmd/test/check.go b/internal/lsp/cmd/test/check.go
index 024d987..65e9688 100644
--- a/internal/lsp/cmd/test/check.go
+++ b/internal/lsp/cmd/test/check.go
@@ -50,8 +50,8 @@
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 @@
}
}
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)
diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go
index d53249b..dbc9450 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -71,9 +71,14 @@
}
// 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 @@
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"
}
diff --git a/internal/lsp/testdata/circular/b/b.go b/internal/lsp/testdata/circular/b/b.go
new file mode 100644
index 0000000..950abba
--- /dev/null
+++ b/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()
+}
diff --git a/internal/lsp/testdata/circular/one/one.go b/internal/lsp/testdata/circular/one/one.go
new file mode 100644
index 0000000..8e74e8b
--- /dev/null
+++ b/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()
+}
diff --git a/internal/lsp/testdata/circular/self.go b/internal/lsp/testdata/circular/self.go
new file mode 100644
index 0000000..452ab50
--- /dev/null
+++ b/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() {
+}
diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden
index 60d49aa..3c952c0 100644
--- a/internal/lsp/testdata/summary.txt.golden
+++ b/internal/lsp/testdata/summary.txt.golden
@@ -6,7 +6,7 @@
FuzzyCompletionsCount = 7
RankedCompletionsCount = 26
CaseSensitiveCompletionsCount = 4
-DiagnosticsCount = 22
+DiagnosticsCount = 24
FoldingRangesCount = 2
FormatCount = 6
ImportCount = 7
