From c623a2817b4c985e38252e1ce89b85a91814bcea Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 3 Apr 2024 11:32:30 -0400 Subject: [PATCH] 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). Fixes golang/go#66636 Change-Id: I24385474d4a6ebf6b7e9ae8f20948564bad3f55e Reviewed-on: https://go-review.googlesource.com/c/tools/+/576135 Auto-Submit: Robert Findley Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/internal/cache/analysis.go | 5 +- gopls/internal/cache/check.go | 34 ++++++---- gopls/internal/test/integration/regtest.go | 14 +++++ gopls/internal/test/integration/runner.go | 20 +++--- .../integration/workspace/goversion_test.go | 62 +++++++++++++++++++ internal/testenv/testenv.go | 7 ++- 6 files changed, 116 insertions(+), 26 deletions(-) create mode 100644 gopls/internal/test/integration/workspace/goversion_test.go diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 34090473691..fbc84730296 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -1013,10 +1013,7 @@ func (an *analysisNode) typeCheck(parsed []*parsego.File) *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) { cfg.GoVersion = goVersion } } diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 29a786909ff..6bbdf2e2541 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -1632,19 +1632,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/types@go1.21 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/types@go1.20 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) { cfg.GoVersion = goVersion } } @@ -1655,6 +1643,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. diff --git a/gopls/internal/test/integration/regtest.go b/gopls/internal/test/integration/regtest.go index 4e26fd79d5b..c183cfde061 100644 --- a/gopls/internal/test/integration/regtest.go +++ b/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{ diff --git a/gopls/internal/test/integration/runner.go b/gopls/internal/test/integration/runner.go index adb307f1901..d66df2f8044 100644 --- a/gopls/internal/test/integration/runner.go +++ b/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 @@ -278,16 +283,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 { diff --git a/gopls/internal/test/integration/workspace/goversion_test.go b/gopls/internal/test/integration/workspace/goversion_test.go new file mode 100644 index 00000000000..b6604afe6b3 --- /dev/null +++ b/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")), + ) + }) +} diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go index 6cea5d7b6a2..d4a17ce039a 100644 --- a/internal/testenv/testenv.go +++ b/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 { @@ -198,7 +201,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 }