go/types, types2: do not mutate arguments in NewChecker

CL 507975 resulted in new data races (as reported in #61212), because
the pkg argument to NewChecker was mutated.

Fix this by deferring the recording of the goVersion in pkg until type
checking is actually initiated via a call to Checker.Files.
Additionally, modify types2/check.go to bring it in sync with the
changes in go/types/check.go, and generate the new version_test.go from
the types2 file.

Also move parsing the version into checkFiles, for simplicity.

Fixes #61212

Change-Id: I15edb6c2cff3085622fe7c6a3b0dab531d27bd04
Reviewed-on: https://go-review.googlesource.com/c/go/+/508439


Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>

src/cmd/compile/internal/types2/check.go

@@ -11,6 +11,7 @@ import (
 	"errors"
 	"fmt"
 	"go/constant"
+	"internal/goversion"
 	. "internal/types/errors"
 )
 
@@ -231,19 +232,19 @@ func NewChecker(conf *Config, pkg *Package, info *Info) *Checker {
 		info = new(Info)
 	}
 
-	version, err := parseGoVersion(conf.GoVersion)
-	if err != nil {
-		panic(fmt.Sprintf("invalid Go version %q (%v)", conf.GoVersion, err))
-	}
+	// Note: clients may call NewChecker with the Unsafe package, which is
+	// globally shared and must not be mutated. Therefore NewChecker must not
+	// mutate *pkg.
+	//
+	// (previously, pkg.goVersion was mutated here: go.dev/issue/61212)
 
 	return &Checker{
-		conf:    conf,
-		ctxt:    conf.Context,
-		pkg:     pkg,
-		Info:    info,
-		version: version,
-		objMap:  make(map[Object]*declInfo),
-		impMap:  make(map[importKey]*Package),
+		conf:   conf,
+		ctxt:   conf.Context,
+		pkg:    pkg,
+		Info:   info,
+		objMap: make(map[Object]*declInfo),
+		impMap: make(map[importKey]*Package),
 	}
 }
 
@@ -333,6 +334,20 @@ func (check *Checker) Files(files []*syntax.File) error { return check.checkFile
 var errBadCgo = errors.New("cannot use FakeImportC and go115UsesCgo together")
 
 func (check *Checker) checkFiles(files []*syntax.File) (err error) {
+	if check.pkg == Unsafe {
+		// Defensive handling for Unsafe, which cannot be type checked, and must
+		// not be mutated. See https://go.dev/issue/61212 for an example of where
+		// Unsafe is passed to NewChecker.
+		return nil
+	}
+
+	check.version, err = parseGoVersion(check.conf.GoVersion)
+	if err != nil {
+		return err
+	}
+	if check.version.after(version{1, goversion.Version}) {
+		return fmt.Errorf("package requires newer Go version %v", check.version)
+	}
 	if check.conf.FakeImportC && check.conf.go115UsesCgo {
 		return errBadCgo
 	}
@@ -377,6 +392,7 @@ func (check *Checker) checkFiles(files []*syntax.File) (err error) {
 		check.monomorph()
 	}
 
+	check.pkg.goVersion = check.conf.GoVersion
 	check.pkg.complete = true
 
 	// no longer needed - release memory

src/cmd/compile/internal/types2/version_test.go

+// 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 types2
+
+import "testing"
+
+var parseGoVersionTests = []struct {
+	in  string
+	out version
+}{
+	{"go1.21", version{1, 21}},
+	{"go1.21.0", version{1, 21}},
+	{"go1.21rc2", version{1, 21}},
+}
+
+func TestParseGoVersion(t *testing.T) {
+	for _, tt := range parseGoVersionTests {
+		if out, err := parseGoVersion(tt.in); out != tt.out || err != nil {
+			t.Errorf("parseGoVersion(%q) = %v, %v, want %v, nil", tt.in, out, err, tt.out)
+		}
+	}
+}

src/go/types/check.go

@@ -99,12 +99,11 @@ type Checker struct {
 	fset *token.FileSet
 	pkg  *Package
 	*Info
-	version    version                // accepted language version
-	versionErr error                  // version error, delayed from NewChecker
-	nextID     uint64                 // unique Id for type parameters (first valid Id is 1)
-	objMap     map[Object]*declInfo   // maps package-level objects and (non-interface) methods to declaration info
-	impMap     map[importKey]*Package // maps (import path, source directory) to (complete or fake) package
-	valids     instanceLookup         // valid *Named (incl. instantiated) types per the validType check
+	version version                // accepted language version
+	nextID  uint64                 // unique Id for type parameters (first valid Id is 1)
+	objMap  map[Object]*declInfo   // maps package-level objects and (non-interface) methods to declaration info
+	impMap  map[importKey]*Package // maps (import path, source directory) to (complete or fake) package
+	valids  instanceLookup         // valid *Named (incl. instantiated) types per the validType check
 
 	// pkgPathMap maps package names to the set of distinct import paths we've
 	// seen for that name, anywhere in the import graph. It is used for
@@ -235,21 +234,20 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch
 		info = new(Info)
 	}
 
-	version, versionErr := parseGoVersion(conf.GoVersion)
-	if pkg != nil {
-		pkg.goVersion = conf.GoVersion
-	}
+	// Note: clients may call NewChecker with the Unsafe package, which is
+	// globally shared and must not be mutated. Therefore NewChecker must not
+	// mutate *pkg.
+	//
+	// (previously, pkg.goVersion was mutated here: go.dev/issue/61212)
 
 	return &Checker{
-		conf:       conf,
-		ctxt:       conf.Context,
-		fset:       fset,
-		pkg:        pkg,
-		Info:       info,
-		version:    version,
-		versionErr: versionErr,
-		objMap:     make(map[Object]*declInfo),
-		impMap:     make(map[importKey]*Package),
+		conf:   conf,
+		ctxt:   conf.Context,
+		fset:   fset,
+		pkg:    pkg,
+		Info:   info,
+		objMap: make(map[Object]*declInfo),
+		impMap: make(map[importKey]*Package),
 	}
 }
 
@@ -345,8 +343,16 @@ func (check *Checker) Files(files []*ast.File) error { return check.checkFiles(f
 var errBadCgo = errors.New("cannot use FakeImportC and go115UsesCgo together")
 
 func (check *Checker) checkFiles(files []*ast.File) (err error) {
-	if check.versionErr != nil {
-		return check.versionErr
+	if check.pkg == Unsafe {
+		// Defensive handling for Unsafe, which cannot be type checked, and must
+		// not be mutated. See https://go.dev/issue/61212 for an example of where
+		// Unsafe is passed to NewChecker.
+		return nil
+	}
+
+	check.version, err = parseGoVersion(check.conf.GoVersion)
+	if err != nil {
+		return err
 	}
 	if check.version.after(version{1, goversion.Version}) {
 		return fmt.Errorf("package requires newer Go version %v", check.version)
@@ -395,6 +401,7 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
 		check.monomorph()
 	}
 
+	check.pkg.goVersion = check.conf.GoVersion
 	check.pkg.complete = true
 
 	// no longer needed - release memory

src/go/types/generate_test.go

@@ -141,6 +141,7 @@ var filemap = map[string]action{
 	"universe.go":      fixGlobalTypVarDecl,
 	"util_test.go":     fixTokenPos,
 	"validtype.go":     nil,
+	"version_test.go":  nil,
 }
 
 // TODO(gri) We should be able to make these rewriters more configurable/composable.

src/go/types/version_test.go

+// Code generated by "go test -run=Generate -write=all"; DO NOT EDIT.
+
 // 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.