go/types: data races resulting from package mutation in NewChecker

[adonovan]

#!watchflakes
post <- log ~ `WARNING: DATA RACE` && log ~ `\s+go/types\.NewChecker\(\)` && date < "2023-07-08"

https://storage.googleapis.com/go-build-log/449ef379/linux-amd64-race_bbcd4378.log

package nosuchpkg is not in std (/workdir/go/src/nosuchpkg)
package nosuchpkg is not in std (/workdir/go/src/nosuchpkg)
open /workdir/gopath/src/golang.org/x/tools/go/loader/missing.go: no such file or directory
open /workdir/gopath/src/golang.org/x/tools/go/loader/missing.go: no such file or directory
/workdir/gopath/src/golang.org/x/tools/go/loader/testdata/badpkgdecl.go:1:34: expected 'package', found 'EOF'
/workdir/gopath/src/golang.org/x/tools/go/loader/testdata/badpkgdecl.go:1:34: expected 'package', found 'EOF'
/go/src/b/x.go:1:21: could not import c (cannot find package "c" in any of:
	/go/src/c (from $GOROOT)
	($GOPATH not set. For more details see: 'go help gopath'))
/go/src/b/x.go:1:21: could not import c (cannot find package "c" in any of:
	/go/src/c (from $GOROOT)
	($GOPATH not set. For more details see: 'go help gopath'))
/go/src/b/x.go:1:21: could not import c (/go/src/c/x.go:1:8: expected 'IDENT', found 'EOF')
/go/src/c/x.go:1:20: expected operand, found 'EOF'
cannot find package "two/three" in any of:
	/go/src/two/three (from $GOROOT)
	($GOPATH not set. For more details see: 'go help gopath')
cannot find package "http" in any of:
	/go/src/vendor/http (vendor tree)
	/go/src/http (from $GOROOT)
	($GOPATH not set. For more details see: 'go help gopath')
/go/src/c/x.go:1:31: cannot convert false (untyped bool constant) to type int
==================
WARNING: DATA RACE
Write at 0x00c00007c288 by goroutine 1882:
  go/types.NewChecker()
      /workdir/go/src/go/types/check.go:240 +0xfd
  golang.org/x/tools/go/loader.(*importer).newPackageInfo()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:1053 +0x836
  golang.org/x/tools/go/loader.(*importer).load()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:961 +0xa4
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:949 +0x45

Previous write at 0x00c00007c288 by goroutine 1888:
  go/types.NewChecker()
      /workdir/go/src/go/types/check.go:240 +0xfd
  golang.org/x/tools/go/loader.(*importer).newPackageInfo()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:1053 +0x836
  golang.org/x/tools/go/loader.(*importer).load()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:961 +0xa4
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:949 +0x45

Goroutine 1882 (running) created at:
  golang.org/x/tools/go/loader.(*importer).startLoad()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:948 +0x329
  golang.org/x/tools/go/loader.(*importer).importAll()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:897 +0x53d
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:992 +0x127
  golang.org/x/tools/go/loader.(*importer).load()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:968 +0x199
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:949 +0x45

Goroutine 1888 (finished) created at:
  golang.org/x/tools/go/loader.(*importer).startLoad()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:948 +0x329
  golang.org/x/tools/go/loader.(*importer).importAll()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:897 +0x53d
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:992 +0x127
  golang.org/x/tools/go/loader.(*importer).load()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:968 +0x199
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:949 +0x45
==================
--- FAIL: TestLoad1 (4.03s)
    testing.go:1465: race detected during execution of test
--- FAIL: TestLoad2 (4.14s)
    testing.go:1465: race detected during execution of test
FAIL
FAIL	golang.org/x/tools/go/loader	20.833s

[findleyr]

I'm seeing this flake with pretty high frequency as of yesterday, yet go/loader has not changed recently AFAIK.

A change to the race detector, perhaps?

[adonovan]

Yeah, I had the same thought. But I can't reproduce it locally with 1.20, or tip, or tip from Jun 23. I just noticed that https://go.dev/cl/508437 is also failing in -race even though the change is benign. I've asked the runtime folks.

[findleyr]

Possibly related: https://go.dev/cl/505055

[findleyr]

This is actually resulting from https://go.dev/cl/507975, where NewChecker(...pkg...) was updated to mutate pkg.

As @prattmic pointed out in chat, importer.newPackageInfo calls NewChecker concurrently with types.Unsafe.

There are probably two bugs here: go/loader should not call NewChecker with types.Unsafe, but also we should probably not mutate pkg in NewChecker. Certainly the checker owns the package, but lets defer mutation until type-checking actually begins.

[adonovan]

Well spotted. NewChecker should have an internal comment stating that it may be called with types.Unsafe, and that it should not mutate pkg until CheckFiles (which is never called for Unsafe). And CheckFiles should either return immediately or panic if pkg==Unsafe.

[findleyr]

Yes, I'll fix.