Skip to content

Commit

Permalink
[gopls-release-branch.0.15] internal/check: filter out too-new Go ver…
Browse files Browse the repository at this point in the history
…sions for type checking

NOTE: Patched for the release branch to use versions.Compare rather than
versions.Before.

The type checker produces an error if the Go version is too new. When
compiled with Go 1.21, this error is silently dropped on the floor and
the type checked package is empty, due to golang/go##66525.

Guard against this very problematic failure mode by filtering out Go
versions that are too new. We should also produce a diagnostic, but that
is more complicated and covered by golang/go#61673.

Also: fix a bug where sandbox cleanup would fail due to being run with a
non-local toolchain.

Updates golang/go#66677
Updates golang/go#66730

Change-Id: Ia66f17c195382c9c55cf0ef883e898553ce950e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576678
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit de6db98)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577302
  • Loading branch information
findleyr committed Apr 8, 2024
1 parent 1202974 commit 4bdbdca
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 2 deletions.
17 changes: 17 additions & 0 deletions gopls/internal/cache/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -1652,6 +1652,10 @@ func validGoVersion(goVersion string) bool {
return false // malformed version string
}

if relVer := releaseVersion(); relVer != "" && versions.Compare(relVer, goVersion) < 0 {
return false // 'go list' is too new for go/types
}

// 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 {
Expand All @@ -1661,6 +1665,19 @@ func validGoVersion(goVersion string) bool {
return true
}

// releaseVersion reports the Go language version used to compile gopls, or ""
// if it cannot be determined.
func releaseVersion() string {
if len(build.Default.ReleaseTags) > 0 {
v := build.Default.ReleaseTags[len(build.Default.ReleaseTags)-1]
var dummy int
if _, err := fmt.Sscanf(v, "go1.%d", &dummy); err == nil {
return v
}
}
return ""
}

// 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
4 changes: 3 additions & 1 deletion gopls/internal/test/integration/fake/sandbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,9 @@ func (sb *Sandbox) GoVersion(ctx context.Context) (int, error) {
func (sb *Sandbox) Close() error {
var goCleanErr error
if sb.gopath != "" {
goCleanErr = sb.RunGoCommand(context.Background(), "", "clean", []string{"-modcache"}, nil, false)
// Important: run this command in RootDir so that it doesn't interact with
// any toolchain downloads that may occur
goCleanErr = sb.RunGoCommand(context.Background(), sb.RootDir(), "clean", []string{"-modcache"}, nil, false)
}
err := robustio.RemoveAll(sb.rootdir)
if err != nil || goCleanErr != nil {
Expand Down
64 changes: 64 additions & 0 deletions gopls/internal/test/integration/workspace/goversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ package workspace
import (
"flag"
"os"
"os/exec"
"runtime"
"strings"
"testing"

. "golang.org/x/tools/gopls/internal/test/integration"
"golang.org/x/tools/internal/testenv"
)

var go121bin = flag.String("go121bin", "", "bin directory containing go 1.21 or later")
Expand Down Expand Up @@ -60,3 +63,64 @@ type I interface { string }
)
})
}

func TestTypeCheckingFutureVersions(t *testing.T) {
// This test checks the regression in golang/go#66677, where go/types fails
// silently when the language version is 1.22.
//
// It does this by recreating the scenario of a toolchain upgrade to 1.22, as
// reported in the issue. For this to work, the test must be able to download
// toolchains from proxy.golang.org.
//
// This is really only a problem for Go 1.21, because with Go 1.23, the bug
// is fixed, and starting with 1.23 we're going to *require* 1.23 to build
// gopls.
//
// TODO(golang/go#65917): delete this test after Go 1.23 is released and
// gopls requires the latest Go to build.
testenv.SkipAfterGo1Point(t, 21)

if testing.Short() {
t.Skip("skipping with -short, as this test uses the network")
}

// If go 1.22.2 is already available in the module cache, reuse it rather
// than downloading it anew.
out, err := exec.Command("go", "env", "GOPATH").Output()
if err != nil {
t.Fatal(err)
}
gopath := strings.TrimSpace(string(out)) // use the ambient 1.22.2 toolchain if available

const files = `
-- go.mod --
module example.com/foo
go 1.22.2
-- main.go --
package main
func main() {
x := 1
}
`

WithOptions(
Modes(Default), // slow test, only run in one mode
EnvVars{
"GOPATH": gopath,
"GOTOOLCHAIN": "", // not local
"GOPROXY": "https://proxy.golang.org",
"GOSUMDB": "sum.golang.org",
},
).Run(t, files, func(t *testing.T, env *Env) {
env.OpenFile("main.go")
env.AfterChange(
Diagnostics(
env.AtRegexp("main.go", "x"),
WithMessage("not used"),
),
)
})
}
3 changes: 2 additions & 1 deletion gopls/internal/test/marker/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ treatment by the test runner:
- "flags": this file is treated as a whitespace-separated list of flags
that configure the MarkerTest instance. Supported flags:
-min_go=go1.20 sets the minimum Go version for the test;
-{min,max}_go=go1.20 sets the {min,max}imum Go version for the test
(inclusive)
-cgo requires that CGO_ENABLED is set and the cgo tool is available
-write_sumfile=a,b,c instructs the test runner to generate go.sum files
in these directories before running the test.
Expand Down
9 changes: 9 additions & 0 deletions gopls/internal/test/marker/marker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,13 @@ func Test(t *testing.T) {
}
testenv.NeedsGo1Point(t, go1point)
}
if test.maxGoVersion != "" {
var go1point int
if _, err := fmt.Sscanf(test.maxGoVersion, "go1.%d", &go1point); err != nil {
t.Fatalf("parsing -max_go version: %v", err)
}
testenv.SkipAfterGo1Point(t, go1point)
}
if test.cgo {
testenv.NeedsTool(t, "cgo")
}
Expand Down Expand Up @@ -500,6 +507,7 @@ type markerTest struct {

// Parsed flags values.
minGoVersion string
maxGoVersion string
cgo bool
writeGoSum []string // comma separated dirs to write go sum for
skipGOOS []string // comma separated GOOS values to skip
Expand All @@ -513,6 +521,7 @@ type markerTest struct {
func (t *markerTest) flagSet() *flag.FlagSet {
flags := flag.NewFlagSet(t.name, flag.ContinueOnError)
flags.StringVar(&t.minGoVersion, "min_go", "", "if set, the minimum go1.X version required for this test")
flags.StringVar(&t.maxGoVersion, "max_go", "", "if set, the maximum go1.X version required for this test")
flags.BoolVar(&t.cgo, "cgo", false, "if set, requires cgo (both the cgo tool and CGO_ENABLED=1)")
flags.Var((*stringListValue)(&t.writeGoSum), "write_sumfile", "if set, write the sumfile for these directories")
flags.Var((*stringListValue)(&t.skipGOOS), "skip_goos", "if set, skip this test on these GOOS values")
Expand Down

0 comments on commit 4bdbdca

Please sign in to comment.