[gopls-release-branch.0.15] gopls/internal/cache: fix crash in snapshot.Analyze with patch versions

Fix the same crash as golang/go#66195, this time in Analyze: don't set
invalid Go versions on the types.Config.

The largest part of this change was writing a realistic test, since the
lack of a test for golang/go#66195 caused us to miss this additional
location. It was possible to create a test that worked, by using a flag
to select a go1.21 binary location.

For this test, it was required to move a couple additional integration
test preconditions into integration.Main (otherwise, the test would be
skipped due to the modified environment).

Updates golang/go#66636
Updates golang/go#66730

Change-Id: I24385474d4a6ebf6b7e9ae8f20948564bad3f55e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576135
Auto-Submit: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
(cherry picked from commit c623a28)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577301

Author: findleyr

gopls/internal/cache/analysis.go

@@ -32,11 +32,11 @@ import (
 
 	"golang.org/x/sync/errgroup"
 	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/filecache"
-	"golang.org/x/tools/gopls/internal/cache/metadata"
-	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/progress"
+	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/settings"
 	"golang.org/x/tools/gopls/internal/util/astutil"
 	"golang.org/x/tools/gopls/internal/util/bug"
@@ -1012,10 +1012,7 @@ func (an *analysisNode) typeCheck(parsed []*ParsedGoFile) *analysisPackage {
 	// Set Go dialect.
 	if mp.Module != nil && mp.Module.GoVersion != "" {
 		goVersion := "go" + mp.Module.GoVersion
-		// types.NewChecker panics if GoVersion is invalid.
-		// An unparsable mod file should probably stop us
-		// before we get here, but double check just in case.
-		if goVersionRx.MatchString(goVersion) {
+		if validGoVersion(goVersion) {
 			typesinternal.SetGoVersion(cfg, goVersion)
 		}
 	}

gopls/internal/cache/check.go

@@ -1630,19 +1630,7 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs
 
 	if inputs.goVersion != "" {
 		goVersion := "go" + inputs.goVersion
-
-		// types.NewChecker panics if GoVersion is invalid. An unparsable mod
-		// file should probably stop us before we get here, but double check
-		// just in case.
-		//
-		// Prior to go/[email protected] the precondition was stricter:
-		// no patch version. That's not a problem when also using go1.20 list,
-		// as it would reject go.mod files containing a patch version, but
-		// go/[email protected] will panic on go.mod versions that are returned
-		// by go1.21 list, hence the need for the extra check.
-		if goVersionRx.MatchString(goVersion) &&
-			(slices.Contains(build.Default.ReleaseTags, "go1.21") ||
-				strings.Count(goVersion, ".") < 2) { // no patch version
+		if validGoVersion(goVersion) {
 			typesinternal.SetGoVersion(cfg, goVersion)
 		}
 	}
@@ -1653,6 +1641,26 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs
 	return cfg
 }
 
+// validGoVersion reports whether goVersion is a valid Go version for go/types.
+// types.NewChecker panics if GoVersion is invalid.
+//
+// Note that, prior to go1.21, go/types required exactly two components to the
+// version number. For example, go types would panic with the Go version
+// go1.21.1. validGoVersion handles this case when built with go1.20 or earlier.
+func validGoVersion(goVersion string) bool {
+	if !goVersionRx.MatchString(goVersion) {
+		return false // malformed version string
+	}
+
+	// TODO(rfindley): remove once we no longer support building gopls with Go
+	// 1.20 or earlier.
+	if !slices.Contains(build.Default.ReleaseTags, "go1.21") && strings.Count(goVersion, ".") >= 2 {
+		return false // unsupported patch version
+	}
+
+	return true
+}
+
 // depsErrors creates diagnostics for each metadata error (e.g. import cycle).
 // These may be attached to import declarations in the transitive source files
 // of pkg, or to 'requires' declarations in the package's go.mod file.

gopls/internal/test/integration/regtest.go

@@ -106,8 +106,12 @@ func DefaultModes() Mode {
 	return modes
 }
 
+var runFromMain = false // true if Main has been called
+
 // Main sets up and tears down the shared integration test state.
 func Main(m *testing.M, hook func(*settings.Options)) {
+	runFromMain = true
+
 	// golang/go#54461: enable additional debugging around hanging Go commands.
 	gocommand.DebugHangingGoCommands = true
 
@@ -127,6 +131,16 @@ func Main(m *testing.M, hook func(*settings.Options)) {
 	// Disable GOPACKAGESDRIVER, as it can cause spurious test failures.
 	os.Setenv("GOPACKAGESDRIVER", "off")
 
+	if skipReason := checkBuilder(); skipReason != "" {
+		fmt.Printf("Skipping all tests: %s\n", skipReason)
+		os.Exit(0)
+	}
+
+	if err := testenv.HasTool("go"); err != nil {
+		fmt.Println("Missing go command")
+		os.Exit(1)
+	}
+
 	flag.Parse()
 
 	runner = &Runner{

gopls/internal/test/integration/runner.go

@@ -135,9 +135,14 @@ type TestFunc func(t *testing.T, env *Env)
 func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOption) {
 	// TODO(rfindley): this function has gotten overly complicated, and warrants
 	// refactoring.
-	t.Helper()
-	checkBuilder(t)
-	testenv.NeedsGoPackages(t)
+
+	if !runFromMain {
+		// Main performs various setup precondition checks.
+		// While it could theoretically be made OK for a Runner to be used outside
+		// of Main, it is simpler to enforce that we only use the Runner from
+		// integration test suites.
+		t.Fatal("integration.Runner.Run must be run from integration.Main")
+	}
 
 	tests := []struct {
 		name      string
@@ -271,16 +276,17 @@ var longBuilders = map[string]string{
 	"windows-arm-zx2c4":       "",
 }
 
-func checkBuilder(t *testing.T) {
-	t.Helper()
+// TODO(rfindley): inline into Main.
+func checkBuilder() string {
 	builder := os.Getenv("GO_BUILDER_NAME")
 	if reason, ok := longBuilders[builder]; ok && testing.Short() {
 		if reason != "" {
-			t.Skipf("Skipping %s with -short due to %s", builder, reason)
+			return fmt.Sprintf("skipping %s with -short due to %s", builder, reason)
 		} else {
-			t.Skipf("Skipping %s with -short", builder)
+			return fmt.Sprintf("skipping %s with -short", builder)
 		}
 	}
+	return ""
 }
 
 type loggingFramer struct {

gopls/internal/test/integration/workspace/goversion_test.go

@@ -0,0 +1,62 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package workspace
+
+import (
+	"flag"
+	"os"
+	"runtime"
+	"testing"
+
+	. "golang.org/x/tools/gopls/internal/test/integration"
+)
+
+var go121bin = flag.String("go121bin", "", "bin directory containing go 1.21 or later")
+
+// TODO(golang/go#65917): delete this test once we no longer support building
+// gopls with older Go versions.
+func TestCanHandlePatchVersions(t *testing.T) {
+	// This test verifies the fixes for golang/go#66195 and golang/go#66636 --
+	// that gopls does not crash when encountering a go version with a patch
+	// number in the go.mod file.
+	//
+	// This is tricky to test, because the regression requires that gopls is
+	// built with an older go version, and then the environment is upgraded to
+	// have a more recent go. To set up this scenario, the test requires a path
+	// to a bin directory containing go1.21 or later.
+	if *go121bin == "" {
+		t.Skip("-go121bin directory is not set")
+	}
+
+	if runtime.GOOS != "linux" && runtime.GOOS != "darwin" {
+		t.Skip("requires linux or darwin") // for PATH separator
+	}
+
+	path := os.Getenv("PATH")
+	t.Setenv("PATH", *go121bin+":"+path)
+
+	const files = `
+-- go.mod --
+module example.com/bar
+
+go 1.21.1
+
+-- p.go --
+package bar
+
+type I interface { string }
+`
+
+	WithOptions(
+		EnvVars{
+			"PATH": path,
+		},
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("p.go")
+		env.AfterChange(
+			NoDiagnostics(ForFile("p.go")),
+		)
+	})
+}

internal/testenv/testenv.go

@@ -45,7 +45,10 @@ var checkGoBuild struct {
 	err  error
 }
 
-func hasTool(tool string) error {
+// HasTool reports an error if the required tool is not available in PATH.
+//
+// For certain tools, it checks that the tool executable is correct.
+func HasTool(tool string) error {
 	if tool == "cgo" {
 		enabled, err := cgoEnabled(false)
 		if err != nil {
@@ -192,7 +195,7 @@ func allowMissingTool(tool string) bool {
 // NeedsTool skips t if the named tool is not present in the path.
 // As a special case, "cgo" means "go" is present and can compile cgo programs.
 func NeedsTool(t testing.TB, tool string) {
-	err := hasTool(tool)
+	err := HasTool(tool)
 	if err == nil {
 		return
 	}