gopls/internal/lsp/cache: skip type errors after parse errors

Parser error recovery can delete large swathes of source code;
see golang/go#58833 for examples. Type checking syntax trees
containing syntax errors may therefore result in a large number
of spurious type errors. So, this change suppressed type errors
in the presence of syntax errors.

Fiddling with these tests is really surprisingly time consuming.

Fixes golang/go#59888

Change-Id: Ib489ecf46652c5a346d9caad89fd059434c620f8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/493616
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>

Author: adonovan

gopls/internal/lsp/cache/analysis.go

@@ -605,7 +605,6 @@ func actuallyAnalyze(ctx context.Context, analyzers []*analysis.Analyzer, m *sou
 
 	// TODO(adonovan): port the old logic to:
 	// - gather go/packages diagnostics from m.Errors? (port goPackagesErrorDiagnostics)
-	// - record unparseable file URIs so we can suppress type errors for these files.
 	// - gather diagnostics from expandErrors + typeErrorDiagnostics + depsErrors.
 
 	// -- analysis --
@@ -762,7 +761,16 @@ func typeCheckForAnalysis(fset *token.FileSet, parsed []*source.ParsedGoFile, m
 		Sizes: m.TypesSizes,
 		Error: func(e error) {
 			pkg.compiles = false // type error
-			pkg.typeErrors = append(pkg.typeErrors, e.(types.Error))
+
+			// Suppress type errors in files with parse errors
+			// as parser recovery can be quite lossy (#59888).
+			typeError := e.(types.Error)
+			for _, p := range parsed {
+				if p.ParseErr != nil && source.NodeContains(p.File, typeError.Pos) {
+					return
+				}
+			}
+			pkg.typeErrors = append(pkg.typeErrors, typeError)
 		},
 		Importer: importerFunc(func(importPath string) (*types.Package, error) {
 			if importPath == "unsafe" {

gopls/internal/lsp/cache/load.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"log"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -487,15 +486,6 @@ func buildMetadata(updates map[PackageID]*source.Metadata, pkg *packages.Package
 		DepsErrors: packagesinternal.GetDepsErrors(pkg),
 	}
 
-	if strings.Contains(string(m.PkgPath), "var/folders") {
-		// On macOS, in marker tests, without a go.mod file,
-		// this statement is reached. ID, Name, and PkgPath
-		// take on values that match the LoadDir, such as:
-		// "/var/folders/fy/dn6v01n16zjdwsqy_8qfbbxr000_9w/T/TestMarkersdiagnosticsissue56943.txt2080018120/001/work".
-		// TODO(adonovan): find out why.
-		log.Printf("strange package path: m=%+v pkg=%+v", *m, *pkg)
-	}
-
 	updates[id] = m
 
 	for _, filename := range pkg.CompiledGoFiles {

gopls/internal/lsp/regtest/marker.go

@@ -133,6 +133,7 @@ var update = flag.Bool("update", false, "if set, update test data during marker
 //   - diag(location, regexp): specifies an expected diagnostic matching the
 //     given regexp at the given location. The test runner requires
 //     a 1:1 correspondence between observed diagnostics and diag annotations.
+//     The diagnostics source and kind fields are ignored, to reduce fuss.
 //
 //     The marker must accurately represent the diagnostic's range.
 //     Use grouping parens in the location regular expression to indicate

gopls/internal/lsp/source/view.go

@@ -530,6 +530,9 @@ type TidiedModule struct {
 
 // Metadata represents package metadata retrieved from go/packages.
 // The Deps* maps do not contain self-import edges.
+//
+// An ad-hoc package (without go.mod or GOPATH) has its ID, PkgPath,
+// and LoadDir equal to the absolute path of its directory.
 type Metadata struct {
 	ID              PackageID
 	PkgPath         PackagePath

gopls/internal/lsp/testdata/badstmt/badstmt.go.in

@@ -4,13 +4,12 @@ import (
 	"golang.org/lsptests/foo"
 )
 
-// The nonewvars expectation asserts that the go/analysis framework ran.
-// See comments in noparse.
+// (The syntax error causes suppression of diagnostics for type errors.
+// See issue #59888.)
 
 func _(x int) {
 	defer foo.F //@complete(" //", Foo),diag(" //", "syntax", "function must be invoked in defer statement|expression in defer must be function call", "error")
 	defer foo.F //@complete(" //", Foo)
-	x := 123 //@diag(":=", "nonewvars", "no new variables", "warning")
 }
 
 func _() {

gopls/internal/lsp/testdata/summary.txt.golden

@@ -8,7 +8,7 @@ DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
 RankedCompletionsCount = 164
 CaseSensitiveCompletionsCount = 4
-DiagnosticsCount = 24
+DiagnosticsCount = 23
 FoldingRangesCount = 2
 SemanticTokenCount = 3
 SuggestedFixCount = 73

gopls/internal/lsp/testdata/summary_go1.18.txt.golden

@@ -8,7 +8,7 @@ DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
 RankedCompletionsCount = 174
 CaseSensitiveCompletionsCount = 4
-DiagnosticsCount = 24
+DiagnosticsCount = 23
 FoldingRangesCount = 2
 SemanticTokenCount = 3
 SuggestedFixCount = 79

gopls/internal/lsp/testdata/summary_go1.21.txt.golden

@@ -8,7 +8,7 @@ DeepCompletionsCount = 5
 FuzzyCompletionsCount = 8
 RankedCompletionsCount = 174
 CaseSensitiveCompletionsCount = 4
-DiagnosticsCount = 25
+DiagnosticsCount = 24
 FoldingRangesCount = 2
 SemanticTokenCount = 3
 SuggestedFixCount = 79

gopls/internal/regtest/marker/testdata/diagnostics/noparse.txt

@@ -1,26 +1,27 @@
 
-This test verifies that only parse errors are created.
-we produce diagnostics related to mismatching
-unexported interface methods in non-workspace packages.
+This test exercises diagnostics produced for syntax errors.
 
-The type error was chosen to exercise the 'nonewvars' type-error analyzer.
-(The 'undeclaredname' analyzer depends on the text of the go/types
-"undeclared name" error, which changed in go1.20.)
+Because parser error recovery can be quite lossy, diagnostics
+for type errors are suppressed in files with syntax errors;
+see issue #59888. But diagnostics are reported for type errors
+in well-formed files of the same package.
 
 -- go.mod --
 module example.com
 go 1.12
 
--- a.go --
-package a
+-- bad.go --
+package p
 
-func bye(x int) {
-	x := 123 //@diag(re"():=", re"no new variables")
-}
-
-func stuff() {
-	
+func f() {
+	append("") // no diagnostic for type error in file containing syntax error
 }
 
 func .() {} //@diag(re"func ().", re"expected 'IDENT', found '.'")
 
+-- good.go --
+package p
+
+func g() {
+	append("") //@diag(re`""`, re"a slice")
+}

gopls/internal/regtest/marker/testdata/diagnostics/parseerr.txt

@@ -0,0 +1,33 @@
+
+This test exercises diagnostics produced for type errors
+in the absence of syntax errors.
+
+The type error was chosen to exercise the 'nonewvars' type-error analyzer.
+(The 'undeclaredname' analyzer depends on the text of the go/types
+"undeclared name" error, which changed in go1.20.)
+
+The append() type error was also carefully chosen to have text and
+position that are invariant across all versions of Go run by the builders.
+
+-- go.mod --
+module example.com
+go 1.12
+
+-- typeerr.go --
+package a
+
+func f(x int) {
+	append("") //@diag(re`""`, re"a slice")
+
+	x := 123 //@diag(re"x := 123", re"no new variables"), suggestedfix(re"():", re"no new variables", "quickfix", fix)
+}
+
+-- @fix/typeerr.go --
+package a
+
+func f(x int) {
+	append("") //@diag(re`""`, re"a slice")
+
+	x = 123 //@diag(re"x := 123", re"no new variables"), suggestedfix(re"():", re"no new variables", "quickfix", fix)
+}
+

gopls/internal/regtest/marker/testdata/diagnostics/typeerr.txt

@@ -21,7 +21,7 @@ func what() {
 	var hi func()
 	if {		hi() //@diag(re"(){", re".*missing.*")
 	}
-	hi := nil //@diag(re"():=", re"no new variables")
+	hi := nil
 }
 -- @noparse --
 7:5: missing condition in if statement