From 7f348c7a4c28c9162056530f6be369bb179e05a5 Mon Sep 17 00:00:00 2001 From: Tim King Date: Tue, 27 Feb 2024 16:42:54 -0800 Subject: [PATCH] internal/versions: updates the meaning of FileVersions. For Go >=1.22, FileVersions returns an unknown [invalid] Future version when the file versions would be invalid. This better matches go/types. For Go <=1.21, FileVersions returns either the runtime.Version() or the Future version. Adds AtLeast and Before for doing comparisons with Future. Updates golang/go#65612 Fixes golang/go#66007 Change-Id: I93ff1681b0f9117765614a20d82642749597307c Reviewed-on: https://go-review.googlesource.com/c/tools/+/567635 Reviewed-by: Robert Findley TryBot-Result: Gopher Robot Run-TryBot: Tim King LUCI-TryBot-Result: Go LUCI --- go/analysis/passes/loopclosure/loopclosure.go | 5 +- .../passes/loopclosure/loopclosure_test.go | 3 + .../passes/loopclosure/testdata/src/a/a.go | 3 +- .../testdata/src/subtests/subtest.go | 3 +- .../testdata/src/typeparams/typeparams.go | 11 +- go/ssa/builder.go | 5 +- go/ssa/create.go | 2 +- internal/versions/features.go | 43 +++++++ internal/versions/toolchain.go | 14 +++ internal/versions/toolchain_go119.go | 14 +++ internal/versions/toolchain_go120.go | 14 +++ internal/versions/toolchain_go121.go | 14 +++ internal/versions/types_go121.go | 18 ++- internal/versions/types_go122.go | 25 +++- internal/versions/types_test.go | 4 +- internal/versions/versions.go | 4 +- internal/versions/versions_test.go | 117 ++++++++++++++++++ 17 files changed, 274 insertions(+), 25 deletions(-) create mode 100644 internal/versions/features.go create mode 100644 internal/versions/toolchain.go create mode 100644 internal/versions/toolchain_go119.go create mode 100644 internal/versions/toolchain_go120.go create mode 100644 internal/versions/toolchain_go121.go diff --git a/go/analysis/passes/loopclosure/loopclosure.go b/go/analysis/passes/loopclosure/loopclosure.go index 7b78939c86e..fe05eda44e4 100644 --- a/go/analysis/passes/loopclosure/loopclosure.go +++ b/go/analysis/passes/loopclosure/loopclosure.go @@ -55,9 +55,8 @@ func run(pass *analysis.Pass) (interface{}, error) { switch n := n.(type) { case *ast.File: // Only traverse the file if its goversion is strictly before go1.22. - goversion := versions.Lang(versions.FileVersions(pass.TypesInfo, n)) - // goversion is empty for older go versions (or the version is invalid). - return goversion == "" || versions.Compare(goversion, "go1.22") < 0 + goversion := versions.FileVersion(pass.TypesInfo, n) + return versions.Before(goversion, versions.Go1_22) case *ast.RangeStmt: body = n.Body addVar(n.Key) diff --git a/go/analysis/passes/loopclosure/loopclosure_test.go b/go/analysis/passes/loopclosure/loopclosure_test.go index 386f53289ce..c8aab4834d3 100644 --- a/go/analysis/passes/loopclosure/loopclosure_test.go +++ b/go/analysis/passes/loopclosure/loopclosure_test.go @@ -17,6 +17,9 @@ import ( ) func Test(t *testing.T) { + // legacy loopclosure test expectations are incorrect > 1.21. + testenv.SkipAfterGo1Point(t, 21) + testdata := analysistest.TestData() analysistest.Run(t, testdata, loopclosure.Analyzer, "a", "golang.org/...", "subtests", "typeparams") diff --git a/go/analysis/passes/loopclosure/testdata/src/a/a.go b/go/analysis/passes/loopclosure/testdata/src/a/a.go index 7a7f05f663f..eb4d2a6cc7a 100644 --- a/go/analysis/passes/loopclosure/testdata/src/a/a.go +++ b/go/analysis/passes/loopclosure/testdata/src/a/a.go @@ -2,7 +2,8 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// This file contains tests for the loopclosure checker. +// This file contains legacy tests for the loopclosure checker. +// Legacy expectations are incorrect after go1.22. package testdata diff --git a/go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go b/go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go index 50283ec6152..faf98387c5d 100644 --- a/go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go +++ b/go/analysis/passes/loopclosure/testdata/src/subtests/subtest.go @@ -2,8 +2,9 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// This file contains tests that the loopclosure analyzer detects leaked +// This file contains legacy tests that the loopclosure analyzer detects leaked // references via parallel subtests. +// Legacy expectations are incorrect after go1.22. package subtests diff --git a/go/analysis/passes/loopclosure/testdata/src/typeparams/typeparams.go b/go/analysis/passes/loopclosure/testdata/src/typeparams/typeparams.go index 55e129c0ab0..ef5b143f6da 100644 --- a/go/analysis/passes/loopclosure/testdata/src/typeparams/typeparams.go +++ b/go/analysis/passes/loopclosure/testdata/src/typeparams/typeparams.go @@ -2,7 +2,8 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// This file contains tests for the loopclosure checker. +// This file contains legacy tests for the loopclosure checker for GoVersion 0 - if afterGo122 { + if versions.AtLeast(fn.goversion, versions.Go1_22) { b.forStmtGo122(fn, s, label) return } @@ -2243,7 +2242,7 @@ func (b *builder) rangeStmt(fn *Function, s *ast.RangeStmt, label *lblock) { } } - afterGo122 := versions.Compare(fn.goversion, "go1.21") > 0 + afterGo122 := versions.AtLeast(fn.goversion, versions.Go1_22) if s.Tok == token.DEFINE && !afterGo122 { // pre-go1.22: If iteration variables are defined (:=), this // occurs once outside the loop. diff --git a/go/ssa/create.go b/go/ssa/create.go index 4545c178d78..f8f584a1a56 100644 --- a/go/ssa/create.go +++ b/go/ssa/create.go @@ -245,7 +245,7 @@ func (prog *Program) CreatePackage(pkg *types.Package, files []*ast.File, info * if len(files) > 0 { // Go source package. for _, file := range files { - goversion := versions.Lang(versions.FileVersions(p.info, file)) + goversion := versions.Lang(versions.FileVersion(p.info, file)) for _, decl := range file.Decls { membersFromDecl(p, decl, goversion) } diff --git a/internal/versions/features.go b/internal/versions/features.go new file mode 100644 index 00000000000..b53f1786161 --- /dev/null +++ b/internal/versions/features.go @@ -0,0 +1,43 @@ +// Copyright 2023 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 versions + +// This file contains predicates for working with file versions to +// decide when a tool should consider a language feature enabled. + +// GoVersions that features in x/tools can be gated to. +const ( + Go1_18 = "go1.18" + Go1_19 = "go1.19" + Go1_20 = "go1.20" + Go1_21 = "go1.21" + Go1_22 = "go1.22" +) + +// Future is an invalid unknown Go version sometime in the future. +// Do not use directly with Compare. +const Future = "" + +// AtLeast reports whether the file version v comes after a Go release. +// +// Use this predicate to enable a behavior once a certain Go release +// has happened (and stays enabled in the future). +func AtLeast(v, release string) bool { + if v == Future { + return true // an unknown future version is always after y. + } + return Compare(Lang(v), Lang(release)) >= 0 +} + +// Before reports whether the file version v is strictly before a Go release. +// +// Use this predicate to disable a behavior once a certain Go release +// has happened (and stays enabled in the future). +func Before(v, release string) bool { + if v == Future { + return false // an unknown future version happens after y. + } + return Compare(Lang(v), Lang(release)) < 0 +} diff --git a/internal/versions/toolchain.go b/internal/versions/toolchain.go new file mode 100644 index 00000000000..377bf7a53b4 --- /dev/null +++ b/internal/versions/toolchain.go @@ -0,0 +1,14 @@ +// 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 versions + +// toolchain is maximum version (<1.22) that the go toolchain used +// to build the current tool is known to support. +// +// When a tool is built with >=1.22, the value of toolchain is unused. +// +// x/tools does not support building with go <1.18. So we take this +// as the minimum possible maximum. +var toolchain string = Go1_18 diff --git a/internal/versions/toolchain_go119.go b/internal/versions/toolchain_go119.go new file mode 100644 index 00000000000..f65beed9d83 --- /dev/null +++ b/internal/versions/toolchain_go119.go @@ -0,0 +1,14 @@ +// 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. + +//go:build go1.19 +// +build go1.19 + +package versions + +func init() { + if Compare(toolchain, Go1_19) < 0 { + toolchain = Go1_19 + } +} diff --git a/internal/versions/toolchain_go120.go b/internal/versions/toolchain_go120.go new file mode 100644 index 00000000000..1a9efa126cd --- /dev/null +++ b/internal/versions/toolchain_go120.go @@ -0,0 +1,14 @@ +// 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. + +//go:build go1.20 +// +build go1.20 + +package versions + +func init() { + if Compare(toolchain, Go1_20) < 0 { + toolchain = Go1_20 + } +} diff --git a/internal/versions/toolchain_go121.go b/internal/versions/toolchain_go121.go new file mode 100644 index 00000000000..b7ef216dfec --- /dev/null +++ b/internal/versions/toolchain_go121.go @@ -0,0 +1,14 @@ +// 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. + +//go:build go1.21 +// +build go1.21 + +package versions + +func init() { + if Compare(toolchain, Go1_21) < 0 { + toolchain = Go1_21 + } +} diff --git a/internal/versions/types_go121.go b/internal/versions/types_go121.go index a7b79207aee..b4345d3349e 100644 --- a/internal/versions/types_go121.go +++ b/internal/versions/types_go121.go @@ -12,9 +12,19 @@ import ( "go/types" ) -// FileVersions always reports the a file's Go version as the -// zero version at this Go version. -func FileVersions(info *types.Info, file *ast.File) string { return "" } +// FileVersion returns a language version (<=1.21) derived from runtime.Version() +// or an unknown future version. +func FileVersion(info *types.Info, file *ast.File) string { + // In x/tools built with Go <= 1.21, we do not have Info.FileVersions + // available. We use a go version derived from the toolchain used to + // compile the tool by default. + // This will be <= go1.21. We take this as the maximum version that + // this tool can support. + // + // There are no features currently in x/tools that need to tell fine grained + // differences for versions <1.22. + return toolchain +} -// InitFileVersions is a noop at this Go version. +// InitFileVersions is a noop when compiled with this Go version. func InitFileVersions(*types.Info) {} diff --git a/internal/versions/types_go122.go b/internal/versions/types_go122.go index 7b9ba89a822..e8180632a52 100644 --- a/internal/versions/types_go122.go +++ b/internal/versions/types_go122.go @@ -12,10 +12,27 @@ import ( "go/types" ) -// FileVersions maps a file to the file's semantic Go version. -// The reported version is the zero version if a version cannot be determined. -func FileVersions(info *types.Info, file *ast.File) string { - return info.FileVersions[file] +// FileVersions returns a file's Go version. +// The reported version is an unknown Future version if a +// version cannot be determined. +func FileVersion(info *types.Info, file *ast.File) string { + // In tools built with Go >= 1.22, the Go version of a file + // follow a cascades of sources: + // 1) types.Info.FileVersion, which follows the cascade: + // 1.a) file version (ast.File.GoVersion), + // 1.b) the package version (types.Config.GoVersion), or + // 2) is some unknown Future version. + // + // File versions require a valid package version to be provided to types + // in Config.GoVersion. Config.GoVersion is either from the package's module + // or the toolchain (go run). This value should be provided by go/packages + // or unitchecker.Config.GoVersion. + if v := info.FileVersions[file]; IsValid(v) { + return v + } + // Note: we could instead return runtime.Version() [if valid]. + // This would act as a max version on what a tool can support. + return Future } // InitFileVersions initializes info to record Go versions for Go files. diff --git a/internal/versions/types_test.go b/internal/versions/types_test.go index 0ffdd468dcf..59f6d18b45f 100644 --- a/internal/versions/types_test.go +++ b/internal/versions/types_test.go @@ -52,11 +52,11 @@ func Test(t *testing.T) { if got, want := versions.GoVersion(pkg), item.pversion; versions.Compare(got, want) != 0 { t.Errorf("GoVersion()=%q. expected %q", got, want) } - if got := versions.FileVersions(info, nil); got != "" { + if got := versions.FileVersion(info, nil); got != "" { t.Errorf(`FileVersions(nil)=%q. expected ""`, got) } for i, test := range item.tests { - if got, want := versions.FileVersions(info, files[i]), test.want; got != want { + if got, want := versions.FileVersion(info, files[i]), test.want; got != want { t.Errorf("FileVersions(%s)=%q. expected %q", test.fname, got, want) } } diff --git a/internal/versions/versions.go b/internal/versions/versions.go index f8982841b7b..8d1f7453dbf 100644 --- a/internal/versions/versions.go +++ b/internal/versions/versions.go @@ -4,7 +4,9 @@ package versions -import "strings" +import ( + "strings" +) // Note: If we use build tags to use go/versions when go >=1.22, // we run into go.dev/issue/53737. Under some operations users would see an diff --git a/internal/versions/versions_test.go b/internal/versions/versions_test.go index 997de2a8a61..dbc1c555d22 100644 --- a/internal/versions/versions_test.go +++ b/internal/versions/versions_test.go @@ -5,8 +5,13 @@ package versions_test import ( + "go/ast" + "go/parser" + "go/token" + "go/types" "testing" + "golang.org/x/tools/internal/testenv" "golang.org/x/tools/internal/versions" ) @@ -137,3 +142,115 @@ func TestLang(t *testing.T) { } } + +func TestKnown(t *testing.T) { + for _, v := range [...]string{ + versions.Go1_18, + versions.Go1_19, + versions.Go1_20, + versions.Go1_21, + versions.Go1_22, + } { + if !versions.IsValid(v) { + t.Errorf("Expected known version %q to be valid.", v) + } + if v != versions.Lang(v) { + t.Errorf("Expected known version %q == Lang(%q).", v, versions.Lang(v)) + } + } +} + +func TestAtLeast(t *testing.T) { + for _, item := range [...]struct { + v, release string + want bool + }{ + {versions.Future, versions.Go1_22, true}, + {versions.Go1_22, versions.Go1_22, true}, + {"go1.21", versions.Go1_22, false}, + {"invalid", versions.Go1_22, false}, + } { + if got := versions.AtLeast(item.v, item.release); got != item.want { + t.Errorf("AtLeast(%q, %q)=%v. wanted %v", item.v, item.release, got, item.want) + } + } +} + +func TestBefore(t *testing.T) { + for _, item := range [...]struct { + v, release string + want bool + }{ + {versions.Future, versions.Go1_22, false}, + {versions.Go1_22, versions.Go1_22, false}, + {"go1.21", versions.Go1_22, true}, + {"invalid", versions.Go1_22, true}, // invalid < Go1_22 + } { + if got := versions.Before(item.v, item.release); got != item.want { + t.Errorf("Before(%q, %q)=%v. wanted %v", item.v, item.release, got, item.want) + } + } +} + +func TestFileVersions122(t *testing.T) { + testenv.NeedsGo1Point(t, 22) + + const source = ` + package P + ` + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "hello.go", source, 0) + if err != nil { + t.Fatal(err) + } + + for _, conf := range []types.Config{ + {GoVersion: versions.Go1_22}, + {}, // GoVersion is unset. + } { + info := &types.Info{} + versions.InitFileVersions(info) + + _, err = conf.Check("P", fset, []*ast.File{f}, info) + if err != nil { + t.Fatal(err) + } + + v := versions.FileVersion(info, f) + if !versions.AtLeast(v, versions.Go1_22) { + t.Errorf("versions.AtLeast(%q, %q) expected to hold", v, versions.Go1_22) + } + + if versions.Before(v, versions.Go1_22) { + t.Errorf("versions.AtLeast(%q, %q) expected to be false", v, versions.Go1_22) + } + + if conf.GoVersion == "" && v != versions.Future { + t.Error("Expected the FileVersion to be the Future when conf.GoVersion is unset") + } + } +} + +func TestFileVersions121(t *testing.T) { + testenv.SkipAfterGo1Point(t, 21) + + // If <1.22, info and file are ignored. + v := versions.FileVersion(nil, nil) + oneof := map[string]bool{ + versions.Go1_18: true, + versions.Go1_19: true, + versions.Go1_20: true, + versions.Go1_21: true, + } + if !oneof[v] { + t.Errorf("FileVersion(...)=%q expected to be a known go version <1.22", v) + } + + if versions.AtLeast(v, versions.Go1_22) { + t.Errorf("versions.AtLeast(%q, %q) expected to be false", v, versions.Go1_22) + } + + if !versions.Before(v, versions.Go1_22) { + t.Errorf("versions.Before(%q, %q) expected to hold", v, versions.Go1_22) + } +}