internal/aliases: expose Enabled

The predicate that determines whether the type checker
creates types.Alias nodes is complex: it depends on an
environment variable (GODEBUG) that is somewhat tricky
to parse correctly, since it is a rightmost-wins list.

Critically, however, its default value is a function of
the go directive in the application's go.mod file, which
is inaccessible to logic in x/tools (per-file build tags
can detect the toolchain version, but not the go.mod version).
Equally critically, the current effective value of the
gotypesalias variable changes when os.Setenv(GODEBUG) is
called, which happens in tests, and there is no way to
detect those events; therefore any attempt to cache the
value is wrong.

This change exposes a simplified version of the Enabled
function from aliases, which always computes the ground
truth by invoking the type checker, regardless of cost.
It also adds an 'enabled' parameter to NewAlias so that
callers can amortize this cost.

Also, various minor cleanups related to aliases.

Updates golang/go#64581

Change-Id: If926edefb8e1c1f63c17e4fad0a808e27bac6d5b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/580455
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

go.mod

@@ -1,18 +1,14 @@
 module golang.org/x/tools
 
-go 1.19
+go 1.19 // => default GODEBUG has gotypesalias=0
 
 require (
+	github.com/google/go-cmp v0.6.0
 	github.com/yuin/goldmark v1.4.13
 	golang.org/x/mod v0.17.0
 	golang.org/x/net v0.24.0
-)
-
-require golang.org/x/sync v0.7.0
-
-require github.com/google/go-cmp v0.6.0 // indirect
-
-require (
-	golang.org/x/sys v0.19.0 // indirect
+	golang.org/x/sync v0.7.0
 	golang.org/x/telemetry v0.0.0-20240228155512-f48c80bd79b2
 )
+
+require golang.org/x/sys v0.19.0 // indirect

gopls/doc/generate.go

@@ -47,6 +47,10 @@ func main() {
 func doMain(write bool) (bool, error) {
 	// TODO(adonovan): when we can rely on go1.23,
 	// switch to gotypesalias=1 behavior.
+	//
+	// (Since this program is run by 'go run',
+	// the gopls/go.mod file's go 1.19 directive doesn't
+	// have its usual effect of setting gotypesalias=0.)
 	os.Setenv("GODEBUG", "gotypesalias=0")
 
 	api, err := loadAPI()

gopls/go.mod

@@ -1,6 +1,6 @@
 module golang.org/x/tools/gopls
 
-go 1.19
+go 1.19 // => default GODEBUG has gotypesalias=0
 
 require (
 	github.com/google/go-cmp v0.6.0

gopls/internal/analysis/fillreturns/fillreturns_test.go

@@ -9,13 +9,16 @@ import (
 
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/gopls/internal/analysis/fillreturns"
-	"golang.org/x/tools/internal/testenv"
+	"golang.org/x/tools/internal/aliases"
 )
 
 func Test(t *testing.T) {
-	// TODO(golang/go#65294): delete (and update expectations)
-	// once gotypesalias=1 is the default.
-	testenv.SkipMaterializedAliases(t, "expectations need updating for materialized aliases")
+	// TODO(golang/go#65294): update expectations and delete this
+	// check once we update go.mod to go1.23 so that
+	// gotypesalias=1 becomes the only supported mode.
+	if aliases.Enabled() {
+		t.Skip("expectations need updating for materialized aliases")
+	}
 
 	testdata := analysistest.TestData()
 	analysistest.RunWithSuggestedFixes(t, testdata, fillreturns.Analyzer, "a", "typeparams")

gopls/internal/test/integration/misc/staticcheck_test.go

@@ -7,6 +7,7 @@ package misc
 import (
 	"testing"
 
+	"golang.org/x/tools/internal/aliases"
 	"golang.org/x/tools/internal/testenv"
 
 	. "golang.org/x/tools/gopls/internal/test/integration"
@@ -15,8 +16,11 @@ import (
 func TestStaticcheckGenerics(t *testing.T) {
 	testenv.NeedsGo1Point(t, 20) // staticcheck requires go1.20+
 
-	// TODO(golang/go#65249): re-enable and fix this test with gotypesalias=1.
-	testenv.SkipMaterializedAliases(t, "staticcheck needs updates for materialized aliases")
+	// TODO(golang/go#65249): re-enable and fix this test once we
+	// update go.mod to go1.23 so that gotypesalias=1 becomes the default.
+	if aliases.Enabled() {
+		t.Skip("staticheck doesn't yet support aliases (dominikh/go-tools#1523)")
+	}
 
 	const files = `
 -- go.mod --
@@ -83,8 +87,11 @@ var FooErr error = errors.New("foo")
 func TestStaticcheckRelatedInfo(t *testing.T) {
 	testenv.NeedsGo1Point(t, 20) // staticcheck is only supported at Go 1.20+
 
-	// TODO(golang/go#65249): re-enable and fix this test with gotypesalias=1.
-	testenv.SkipMaterializedAliases(t, "staticcheck needs updates for materialized aliases")
+	// TODO(golang/go#65249): re-enable and fix this test once we
+	// update go.mod to go1.23 so that gotypesalias=1 becomes the default.
+	if aliases.Enabled() {
+		t.Skip("staticheck doesn't yet support aliases (dominikh/go-tools#1523)")
+	}
 
 	const files = `
 -- go.mod --

internal/aliases/aliases.go

@@ -16,10 +16,14 @@ import (
 // NewAlias creates a new TypeName in Package pkg that
 // is an alias for the type rhs.
 //
-// When GoVersion>=1.22 and GODEBUG=gotypesalias=1 (or unset),
-// the Type() of the return value is a *types.Alias.
-func NewAlias(pos token.Pos, pkg *types.Package, name string, rhs types.Type) *types.TypeName {
-	if enabled() {
+// The enabled parameter determines whether the resulting [TypeName]'s
+// type is an [types.Alias]. Its value must be the result of a call to
+// [Enabled], which computes the effective value of
+// GODEBUG=gotypesalias=... by invoking the type checker. The Enabled
+// function is expensive and should be called once per task (e.g.
+// package import), not once per call to NewAlias.
+func NewAlias(enabled bool, pos token.Pos, pkg *types.Package, name string, rhs types.Type) *types.TypeName {
+	if enabled {
 		tname := types.NewTypeName(pos, pkg, name, nil)
 		newAlias(tname, rhs)
 		return tname

internal/aliases/aliases_go121.go

@@ -24,7 +24,9 @@ func (*Alias) Obj() *types.TypeName { panic("unreachable") }
 // Unalias returns the type t for go <=1.21.
 func Unalias(t types.Type) types.Type { return t }
 
-// Always false for go <=1.21. Ignores GODEBUG.
-func enabled() bool { return false }
-
 func newAlias(name *types.TypeName, rhs types.Type) *Alias { panic("unreachable") }
+
+// Enabled reports whether [NewAlias] should create [types.Alias] types.
+//
+// Before go1.22, this function always returns false.
+func Enabled() bool { return false }

internal/aliases/aliases_go122.go

@@ -12,9 +12,6 @@ import (
 	"go/parser"
 	"go/token"
 	"go/types"
-	"os"
-	"strings"
-	"sync"
 )
 
 // Alias is an alias of types.Alias.
@@ -33,40 +30,23 @@ func newAlias(tname *types.TypeName, rhs types.Type) *Alias {
 	return a
 }
 
-// enabled returns true when types.Aliases are enabled.
-func enabled() bool {
-	// Use the gotypesalias value in GODEBUG if set.
-	godebug := os.Getenv("GODEBUG")
-	value := -1 // last set value.
-	for _, f := range strings.Split(godebug, ",") {
-		switch f {
-		case "gotypesalias=1":
-			value = 1
-		case "gotypesalias=0":
-			value = 0
-		}
-	}
-	switch value {
-	case 0:
-		return false
-	case 1:
-		return true
-	default:
-		return aliasesDefault()
-	}
+// Enabled reports whether [NewAlias] should create [types.Alias] types.
+//
+// This function is expensive! Call it sparingly.
+func Enabled() bool {
+	// The only reliable way to compute the answer is to invoke go/types.
+	// We don't parse the GODEBUG environment variable, because
+	// (a) it's tricky to do so in a manner that is consistent
+	//     with the godebug package; in particular, a simple
+	//     substring check is not good enough. The value is a
+	//     rightmost-wins list of options. But more importantly:
+	// (b) it is impossible to detect changes to the effective
+	//     setting caused by os.Setenv("GODEBUG"), as happens in
+	//     many tests. Therefore any attempt to cache the result
+	//     is just incorrect.
+	fset := token.NewFileSet()
+	f, _ := parser.ParseFile(fset, "a.go", "package p; type A = int", 0)
+	pkg, _ := new(types.Config).Check("p", fset, []*ast.File{f}, nil)
+	_, enabled := pkg.Scope().Lookup("A").Type().(*types.Alias)
+	return enabled
 }
-
-// aliasesDefault reports if aliases are enabled by default.
-func aliasesDefault() bool {
-	// Dynamically check if Aliases will be produced from go/types.
-	aliasesDefaultOnce.Do(func() {
-		fset := token.NewFileSet()
-		f, _ := parser.ParseFile(fset, "a.go", "package p; type A = int", 0)
-		pkg, _ := new(types.Config).Check("p", fset, []*ast.File{f}, nil)
-		_, gotypesaliasDefault = pkg.Scope().Lookup("A").Type().(*types.Alias)
-	})
-	return gotypesaliasDefault
-}
-
-var gotypesaliasDefault bool
-var aliasesDefaultOnce sync.Once

internal/aliases/aliases_test.go

@@ -53,7 +53,9 @@ func TestNewAlias(t *testing.T) {
 		t.Run(godebug, func(t *testing.T) {
 			t.Setenv("GODEBUG", godebug)
 
-			A := aliases.NewAlias(token.NoPos, pkg, "A", tv.Type)
+			enabled := aliases.Enabled()
+
+			A := aliases.NewAlias(enabled, token.NoPos, pkg, "A", tv.Type)
 			if got, want := A.Name(), "A"; got != want {
 				t.Errorf("Expected A.Name()==%q. got %q", want, got)
 			}

internal/gcimporter/iimport.go

@@ -210,6 +210,7 @@ func iimportCommon(fset *token.FileSet, getPackages GetPackagesFunc, data []byte
 	p := iimporter{
 		version: int(version),
 		ipath:   path,
+		aliases: aliases.Enabled(),
 		shallow: shallow,
 		reportf: reportf,
 
@@ -369,6 +370,7 @@ type iimporter struct {
 	version int
 	ipath   string
 
+	aliases bool
 	shallow bool
 	reportf ReportFunc // if non-nil, used to report bugs
 
@@ -567,7 +569,7 @@ func (r *importReader) obj(name string) {
 		// 	tparams := r.tparamList()
 		// 	alias.SetTypeParams(tparams)
 		// }
-		r.declare(aliases.NewAlias(pos, r.currPkg, name, typ))
+		r.declare(aliases.NewAlias(r.p.aliases, pos, r.currPkg, name, typ))
 
 	case constTag:
 		typ, val := r.value()

internal/gcimporter/ureader_yes.go

@@ -26,6 +26,7 @@ type pkgReader struct {
 
 	ctxt    *types.Context
 	imports map[string]*types.Package // previously imported packages, indexed by path
+	aliases bool                      // create types.Alias nodes
 
 	// lazily initialized arrays corresponding to the unified IR
 	// PosBase, Pkg, and Type sections, respectively.
@@ -99,6 +100,7 @@ func readUnifiedPackage(fset *token.FileSet, ctxt *types.Context, imports map[st
 
 		ctxt:    ctxt,
 		imports: imports,
+		aliases: aliases.Enabled(),
 
 		posBases: make([]string, input.NumElems(pkgbits.RelocPosBase)),
 		pkgs:     make([]*types.Package, input.NumElems(pkgbits.RelocPkg)),
@@ -524,7 +526,7 @@ func (pr *pkgReader) objIdx(idx pkgbits.Index) (*types.Package, string) {
 		case pkgbits.ObjAlias:
 			pos := r.pos()
 			typ := r.typ()
-			declare(aliases.NewAlias(pos, objPkg, objName, typ))
+			declare(aliases.NewAlias(r.p.aliases, pos, objPkg, objName, typ))
 
 		case pkgbits.ObjConst:
 			pos := r.pos()

internal/pkgbits/decoder.go

@@ -23,6 +23,9 @@ type PkgDecoder struct {
 	// version is the file format version.
 	version uint32
 
+	// aliases determines whether types.Aliases should be created
+	aliases bool
+
 	// sync indicates whether the file uses sync markers.
 	sync bool
 
@@ -73,6 +76,7 @@ func (pr *PkgDecoder) SyncMarkers() bool { return pr.sync }
 func NewPkgDecoder(pkgPath, input string) PkgDecoder {
 	pr := PkgDecoder{
 		pkgPath: pkgPath,
+		//aliases: aliases.Enabled(),
 	}
 
 	// TODO(mdempsky): Implement direct indexing of input string to

internal/testenv/exec.go

@@ -12,7 +12,6 @@ import (
 	"reflect"
 	"runtime"
 	"strconv"
-	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -191,22 +190,3 @@ func Command(t testing.TB, name string, args ...string) *exec.Cmd {
 	t.Helper()
 	return CommandContext(t, context.Background(), name, args...)
 }
-
-// SkipMaterializedAliases skips the test if go/types would create
-// instances of types.Alias, which some tests do not yet handle
-// correctly.
-func SkipMaterializedAliases(t *testing.T, message string) {
-	if hasMaterializedAliases(Go1Point()) {
-		t.Skipf("%s", message)
-	}
-}
-
-func hasMaterializedAliases(minor int) bool {
-	if minor >= 23 && !strings.Contains(os.Getenv("GODEBUG"), "gotypesalias=0") {
-		return true // gotypesalias=1 became the default in go1.23
-	}
-	if minor == 22 && strings.Contains(os.Getenv("GODEBUG"), "gotypesalias=1") {
-		return true // gotypesalias=0 was the default in go1.22
-	}
-	return false // types.Alias didn't exist in go1.21
-}

refactor/rename/rename_test.go

@@ -18,6 +18,7 @@ import (
 	"testing"
 
 	"golang.org/x/tools/go/buildutil"
+	"golang.org/x/tools/internal/aliases"
 	"golang.org/x/tools/internal/testenv"
 )
 
@@ -1320,12 +1321,12 @@ func main() {
 			return nil
 		}
 
-		if !test.alias {
-			t.Setenv("GODEBUG", "gotypesalias=0")
-		} else if !strings.Contains(fmt.Sprint(build.Default.ReleaseTags), " go1.22") {
-			t.Skip("skipping test that requires materialized type aliases")
-		} else {
-			t.Setenv("GODEBUG", "gotypesalias=1")
+		// Skip tests that require aliases when not enables.
+		// (No test requires _no_ aliases,
+		// so there is no contrapositive case.)
+		if test.alias && !aliases.Enabled() {
+			t.Log("test requires aliases")
+			continue
 		}
 
 		err := Main(ctxt, test.offset, test.from, test.to)