internal/lsp/cache: show type errors on parse errors in other files
CL 295414 suppressed all type checking errors in the presence of parser
errors, but this was a bit overzealous. go/types attempts to suppress
follow-on errors from bad syntax, so as long as we haven't had to modify
the AST and the current file parses, type checking errors can still
provide a decent signal to the user
Fixes golang/go#44736
Change-Id: Icd89404fbae663b8f934a38908eaaa87c561b64a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297872
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index dc5943a..370a034 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -1818,7 +1818,6 @@
}
func TestIssue44736(t *testing.T) {
- t.Skip("failing test - see golang.org/issues/44736")
const files = `
-- go.mod --
module blah.com
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 0607a16..b028385 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -308,7 +308,7 @@
wg sync.WaitGroup
mu sync.Mutex
- skipTypeErrors bool
+ haveFixedFiles bool
)
for i, cgf := range m.compiledGoFiles {
wg.Add(1)
@@ -328,10 +328,10 @@
pkg.compiledGoFiles[i] = pgf
files[i], parseErrors[i], actualErrors[i] = pgf.File, pgf.ParseErr, err
- // If we have fixed parse errors in any of the files,
- // we should hide type errors, as they may be completely nonsensical.
+ // If we have fixed parse errors in any of the files, we should hide type
+ // errors, as they may be completely nonsensical.
mu.Lock()
- skipTypeErrors = skipTypeErrors || fixed
+ haveFixedFiles = haveFixedFiles || fixed
mu.Unlock()
}(i, cgf)
}
@@ -454,6 +454,19 @@
}
}
+ // Our heuristic for whether to show type checking errors is:
+ // + If any file was 'fixed', don't show type checking errors as we
+ // can't guarantee that they reference accurate locations in the source.
+ // + If there is a parse error _in the current file_, suppress type
+ // errors in that file.
+ // + Otherwise, show type errors even in the presence of parse errors in
+ // other package files. go/types attempts to suppress follow-on errors
+ // due to bad syntax, so on balance type checking errors still provide
+ // a decent signal/noise ratio as long as the file in question parses.
+
+ // Track URIs with parse errors so that we can suppress type errors for these
+ // files.
+ unparseable := map[span.URI]bool{}
if len(parseErrors) != 0 {
pkg.hasListOrParseErrors = true
for _, e := range parseErrors {
@@ -462,11 +475,14 @@
event.Error(ctx, "unable to compute positions for parse errors", err, tag.Package.Of(pkg.ID()))
continue
}
- pkg.diagnostics = append(pkg.diagnostics, diags...)
+ for _, diag := range diags {
+ unparseable[diag.URI] = true
+ pkg.diagnostics = append(pkg.diagnostics, diag)
+ }
}
}
- if pkg.hasListOrParseErrors || skipTypeErrors {
+ if haveFixedFiles {
return pkg, nil
}
@@ -477,8 +493,15 @@
event.Error(ctx, "unable to compute positions for type errors", err, tag.Package.Of(pkg.ID()))
continue
}
- pkg.diagnostics = append(pkg.diagnostics, diags...)
pkg.typeErrors = append(pkg.typeErrors, e.primary)
+ for _, diag := range diags {
+ // If the file didn't parse cleanly, it is highly likely that type
+ // checking errors will be confusing or redundant. But otherwise, type
+ // checking usually provides a good enough signal to include.
+ if !unparseable[diag.URI] {
+ pkg.diagnostics = append(pkg.diagnostics, diag)
+ }
+ }
}
depsErrors, err := snapshot.depsErrors(ctx, pkg)
