Skip to content

Commit

Permalink
gopls/internal/cache: fix crash in snapshot.Analyze with patch versions
Browse files Browse the repository at this point in the history
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 <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
findleyr authored and gopherbot committed Apr 3, 2024
1 parent f345449 commit c623a28
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 26 deletions.
5 changes: 1 addition & 4 deletions gopls/internal/cache/analysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
34 changes: 21 additions & 13 deletions gopls/internal/cache/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand All @@ -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.
Expand Down
14 changes: 14 additions & 0 deletions gopls/internal/test/integration/regtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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{
Expand Down
20 changes: 13 additions & 7 deletions gopls/internal/test/integration/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
62 changes: 62 additions & 0 deletions gopls/internal/test/integration/workspace/goversion_test.go
Original file line number Diff line number Diff line change
@@ -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")),
)
})
}
7 changes: 5 additions & 2 deletions internal/testenv/testenv.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down

0 comments on commit c623a28

Please sign in to comment.