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>

diff --git a/src/cmd/compile/internal/types2/check.go b/src/cmd/compile/internal/types2/check.go
index b2a9eb0dbc4c1de5d78f5f77edca0db76e267f51..0a2a49062b5ec001e6e8a7fa853e84e73f3256c5 100644 (file)
--- a/src/cmd/compile/internal/types2/check.go
+++ b/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

diff --git a/src/cmd/compile/internal/types2/version_test.go b/src/cmd/compile/internal/types2/version_test.go
new file mode 100644 (file)
index 0000000..651758e
--- /dev/null
+++ b/src/cmd/compile/internal/types2/version_test.go
@@ -0,0 +1,24 @@
+// 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)
+               }
+       }
+}

diff --git a/src/go/types/check.go b/src/go/types/check.go
index 591de5f329c0b7cfed42d5b8b63d687e46cfb1fc..3b0f5e4fdffbdc7dad16023bd8d8494697f6f043 100644 (file)
--- a/src/go/types/check.go
+++ b/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

diff --git a/src/go/types/generate_test.go b/src/go/types/generate_test.go
index 05a848be31d9cd968950d77012fc657b002944e7..75fda025ee1e31f216e96a2f664f6a518a8568f6 100644 (file)
--- a/src/go/types/generate_test.go
+++ b/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.

diff --git a/src/go/types/version_test.go b/src/go/types/version_test.go
index dc9becf9e18eafe4eb6b4362710d1a1d950b6f6c..d25f7f5e670ae4a64daee7a88b8a0379674bb371 100644 (file)
--- a/src/go/types/version_test.go
+++ b/src/go/types/version_test.go
@@ -1,3 +1,5 @@
+// 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.