From bcc6fa839da540c775e41dcb3c9d11e22f9323fa Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 14 Oct 2021 17:49:44 -0400 Subject: [PATCH] internal/typeparams: guard against generics in stdlib tests Add a new package internal/typeparams/genericfeatures, factoring out the check for usage of generics from the usesgenerics checker. For tests operating on the standard library, this allows us to explicitly detect usage of generics, rather than relying on a hard-coded list. To make this work, I switched the go/ssa standard library test to use go/packages rather than go/loader. This resulted in finding 457 packages versus 676 (likely due to tests), but my assumption is that this is still enough coverage. Change-Id: I6ba32fb4068dd7f5eedb55c2081c642665ed124a Reviewed-on: https://go-review.googlesource.com/c/tools/+/355972 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Tim King --- .../passes/usesgenerics/usesgenerics.go | 110 +++--------------- go/internal/gcimporter/bexport_test.go | 15 ++- go/internal/gcimporter/iexport_test.go | 35 +++--- go/ssa/stdlib_test.go | 46 ++++---- internal/testenv/testenv.go | 6 - .../typeparams/genericfeatures/features.go | 105 +++++++++++++++++ 6 files changed, 179 insertions(+), 138 deletions(-) create mode 100644 internal/typeparams/genericfeatures/features.go diff --git a/go/analysis/passes/usesgenerics/usesgenerics.go b/go/analysis/passes/usesgenerics/usesgenerics.go index 73b53fbefa..4956e0e7e3 100644 --- a/go/analysis/passes/usesgenerics/usesgenerics.go +++ b/go/analysis/passes/usesgenerics/usesgenerics.go @@ -7,15 +7,12 @@ package usesgenerics import ( - "go/ast" - "go/types" "reflect" - "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" - "golang.org/x/tools/internal/typeparams" + "golang.org/x/tools/internal/typeparams/genericfeatures" ) var Analyzer = &analysis.Analyzer{ @@ -32,6 +29,16 @@ const Doc = `detect whether a package uses generics features The usesgenerics analysis reports whether a package directly or transitively uses certain features associated with generic programming in Go.` +type Features = genericfeatures.Features + +const ( + GenericTypeDecls = genericfeatures.GenericTypeDecls + GenericFuncDecls = genericfeatures.GenericFuncDecls + EmbeddedTypeSets = genericfeatures.EmbeddedTypeSets + TypeInstantiation = genericfeatures.TypeInstantiation + FuncInstantiation = genericfeatures.FuncInstantiation +) + // Result is the usesgenerics analyzer result type. The Direct field records // features used directly by the package being analyzed (i.e. contained in the // package source code). The Transitive field records any features used by the @@ -40,53 +47,6 @@ type Result struct { Direct, Transitive Features } -// Features is a set of flags reporting which features of generic Go code a -// package uses, or 0. -type Features int - -const ( - // GenericTypeDecls indicates whether the package declares types with type - // parameters. - GenericTypeDecls Features = 1 << iota - - // GenericFuncDecls indicates whether the package declares functions with - // type parameters. - GenericFuncDecls - - // EmbeddedTypeSets indicates whether the package declares interfaces that - // contain structural type restrictions, i.e. are not fully described by - // their method sets. - EmbeddedTypeSets - - // TypeInstantiation indicates whether the package instantiates any generic - // types. - TypeInstantiation - - // FuncInstantiation indicates whether the package instantiates any generic - // functions. - FuncInstantiation -) - -func (f Features) String() string { - var feats []string - if f&GenericTypeDecls != 0 { - feats = append(feats, "typeDecl") - } - if f&GenericFuncDecls != 0 { - feats = append(feats, "funcDecl") - } - if f&EmbeddedTypeSets != 0 { - feats = append(feats, "typeSet") - } - if f&TypeInstantiation != 0 { - feats = append(feats, "typeInstance") - } - if f&FuncInstantiation != 0 { - feats = append(feats, "funcInstance") - } - return "features{" + strings.Join(feats, ",") + "}" -} - type featuresFact struct { Features Features } @@ -95,7 +55,9 @@ func (f *featuresFact) AFact() {} func (f *featuresFact) String() string { return f.Features.String() } func run(pass *analysis.Pass) (interface{}, error) { - direct := directFeatures(pass) + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + direct := genericfeatures.ForPackage(inspect, pass.TypesInfo) transitive := direct | importedTransitiveFeatures(pass) if transitive != 0 { @@ -108,50 +70,6 @@ func run(pass *analysis.Pass) (interface{}, error) { }, nil } -// directFeatures computes which generic features are used directly by the -// package being analyzed. -func directFeatures(pass *analysis.Pass) Features { - inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - - nodeFilter := []ast.Node{ - (*ast.FuncType)(nil), - (*ast.InterfaceType)(nil), - (*ast.ImportSpec)(nil), - (*ast.TypeSpec)(nil), - } - - var direct Features - - inspect.Preorder(nodeFilter, func(node ast.Node) { - switch n := node.(type) { - case *ast.FuncType: - if tparams := typeparams.ForFuncType(n); tparams != nil { - direct |= GenericFuncDecls - } - case *ast.InterfaceType: - tv := pass.TypesInfo.Types[n] - if iface, _ := tv.Type.(*types.Interface); iface != nil && !typeparams.IsMethodSet(iface) { - direct |= EmbeddedTypeSets - } - case *ast.TypeSpec: - if tparams := typeparams.ForTypeSpec(n); tparams != nil { - direct |= GenericTypeDecls - } - } - }) - - instances := typeparams.GetInstances(pass.TypesInfo) - for _, inst := range instances { - switch inst.Type.(type) { - case *types.Named: - direct |= TypeInstantiation - case *types.Signature: - direct |= FuncInstantiation - } - } - return direct -} - // importedTransitiveFeatures computes features that are used transitively via // imports. func importedTransitiveFeatures(pass *analysis.Pass) Features { diff --git a/go/internal/gcimporter/bexport_test.go b/go/internal/gcimporter/bexport_test.go index 8a80427c06..a65cd1db3e 100644 --- a/go/internal/gcimporter/bexport_test.go +++ b/go/internal/gcimporter/bexport_test.go @@ -19,10 +19,12 @@ import ( "strings" "testing" + "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/buildutil" "golang.org/x/tools/go/internal/gcimporter" "golang.org/x/tools/go/loader" "golang.org/x/tools/internal/typeparams" + "golang.org/x/tools/internal/typeparams/genericfeatures" ) var isRace = false @@ -66,14 +68,22 @@ type UnknownType undefined } numPkgs := len(prog.AllPackages) - if want := 248; numPkgs < want { + if want := minStdlibPackages; numPkgs < want { t.Errorf("Loaded only %d packages, want at least %d", numPkgs, want) } + checked := 0 for pkg, info := range prog.AllPackages { if info.Files == nil { continue // empty directory } + // Binary export does not support generic code. + inspect := inspector.New(info.Files) + if genericfeatures.ForPackage(inspect, &info.Info) != 0 { + t.Logf("skipping package %q which uses generics", pkg.Path()) + continue + } + checked++ exportdata, err := gcimporter.BExportData(conf.Fset, pkg) if err != nil { t.Fatal(err) @@ -116,6 +126,9 @@ type UnknownType undefined } } } + if want := minStdlibPackages; checked < want { + t.Errorf("Checked only %d packages, want at least %d", checked, want) + } } func fileLine(fset *token.FileSet, obj types.Object) string { diff --git a/go/internal/gcimporter/iexport_test.go b/go/internal/gcimporter/iexport_test.go index f035217b4b..201a070b67 100644 --- a/go/internal/gcimporter/iexport_test.go +++ b/go/internal/gcimporter/iexport_test.go @@ -28,10 +28,11 @@ import ( "strings" "testing" + "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/buildutil" "golang.org/x/tools/go/internal/gcimporter" "golang.org/x/tools/go/loader" - "golang.org/x/tools/internal/testenv" + "golang.org/x/tools/internal/typeparams/genericfeatures" ) func readExportFile(filename string) ([]byte, error) { @@ -69,6 +70,8 @@ func isUnifiedBuilder() bool { return os.Getenv("GO_BUILDER_NAME") == "linux-amd64-unified" } +const minStdlibPackages = 248 + func TestIExportData_stdlib(t *testing.T) { if runtime.Compiler == "gccgo" { t.Skip("gccgo standard library is inaccessible") @@ -90,15 +93,8 @@ func TestIExportData_stdlib(t *testing.T) { Sizes: types.SizesFor(ctxt.Compiler, ctxt.GOARCH), }, } - // Temporarily skip packages that use generics on the unified builder, to fix - // TryBots. - // - // TODO(#48595): fix this test with GOEXPERIMENT=unified. - isUnified := isUnifiedBuilder() for _, path := range buildutil.AllPackages(conf.Build) { - if !(isUnified && testenv.UsesGenerics(path)) { - conf.Import(path) - } + conf.Import(path) } // Create a package containing type and value errors to ensure @@ -117,13 +113,19 @@ type UnknownType undefined t.Fatalf("Load failed: %v", err) } - numPkgs := len(prog.AllPackages) - if want := 248; numPkgs < want { - t.Errorf("Loaded only %d packages, want at least %d", numPkgs, want) - } - var sorted []*types.Package + isUnified := isUnifiedBuilder() for pkg, info := range prog.AllPackages { + // Temporarily skip packages that use generics on the unified builder, to + // fix TryBots. + // + // TODO(#48595): fix this test with GOEXPERIMENT=unified. + inspect := inspector.New(info.Files) + features := genericfeatures.ForPackage(inspect, &info.Info) + if isUnified && features != 0 { + t.Logf("skipping package %q which uses generics", pkg.Path()) + continue + } if info.Files != nil { // non-empty directory sorted = append(sorted, pkg) } @@ -133,6 +135,11 @@ type UnknownType undefined }) version := gcimporter.IExportVersion + numPkgs := len(sorted) + if want := minStdlibPackages; numPkgs < want { + t.Errorf("Loaded only %d packages, want at least %d", numPkgs, want) + } + for _, pkg := range sorted { if exportdata, err := iexport(conf.Fset, version, pkg); err != nil { t.Error(err) diff --git a/go/ssa/stdlib_test.go b/go/ssa/stdlib_test.go index 329f0b382c..aaa158076c 100644 --- a/go/ssa/stdlib_test.go +++ b/go/ssa/stdlib_test.go @@ -16,17 +16,17 @@ package ssa_test import ( "go/ast" - "go/build" "go/token" "runtime" "testing" "time" - "golang.org/x/tools/go/buildutil" - "golang.org/x/tools/go/loader" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/packages" "golang.org/x/tools/go/ssa" "golang.org/x/tools/go/ssa/ssautil" "golang.org/x/tools/internal/testenv" + "golang.org/x/tools/internal/typeparams/genericfeatures" ) func bytesAllocated() uint64 { @@ -46,23 +46,27 @@ func TestStdlib(t *testing.T) { t0 := time.Now() alloc0 := bytesAllocated() - // Load, parse and type-check the program. - ctxt := build.Default // copy - ctxt.GOPATH = "" // disable GOPATH - conf := loader.Config{Build: &ctxt} - for _, path := range buildutil.AllPackages(conf.Build) { - // Temporarily skip packages that use generics until supported by go/ssa. - // - // TODO(#48595): revert this once go/ssa supports generics. - if !testenv.UsesGenerics(path) { - conf.ImportWithTests(path) - } - } - - iprog, err := conf.Load() + cfg := &packages.Config{Mode: packages.LoadSyntax} + pkgs, err := packages.Load(cfg, "std", "cmd") if err != nil { - t.Fatalf("Load failed: %v", err) + t.Fatal(err) } + var nonGeneric int + for i := 0; i < len(pkgs); i++ { + pkg := pkgs[i] + inspect := inspector.New(pkg.Syntax) + features := genericfeatures.ForPackage(inspect, pkg.TypesInfo) + // Skip standard library packages that use generics. This won't be + // sufficient if any standard library packages start _importing_ packages + // that use generics. + if features != 0 { + t.Logf("skipping package %q which uses generics", pkg.PkgPath) + continue + } + pkgs[nonGeneric] = pkg + nonGeneric++ + } + pkgs = pkgs[:nonGeneric] t1 := time.Now() alloc1 := bytesAllocated() @@ -72,7 +76,7 @@ func TestStdlib(t *testing.T) { // Comment out these lines during benchmarking. Approx SSA build costs are noted. mode |= ssa.SanityCheckFunctions // + 2% space, + 4% time mode |= ssa.GlobalDebug // +30% space, +18% time - prog := ssautil.CreateProgram(iprog, mode) + prog, _ := ssautil.Packages(pkgs, mode) t2 := time.Now() @@ -87,8 +91,8 @@ func TestStdlib(t *testing.T) { t.Errorf("Loaded only %d packages, want at least %d", numPkgs, want) } - // Keep iprog reachable until after we've measured memory usage. - if len(iprog.AllPackages) == 0 { + // Keep pkgs reachable until after we've measured memory usage. + if len(pkgs) == 0 { panic("unreachable") } diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go index 0010ca4c5d..61735dc2c2 100644 --- a/internal/testenv/testenv.go +++ b/internal/testenv/testenv.go @@ -297,9 +297,3 @@ func SkipAfterGo1Point(t Testing, x int) { t.Skipf("running Go version %q is version 1.%d, newer than maximum 1.%d", runtime.Version(), Go1Point(), x) } } - -// UsesGenerics reports if the standard library package stdlibPkg uses -// generics. -func UsesGenerics(stdlibPkg string) bool { - return stdlibPkg == "constraints" -} diff --git a/internal/typeparams/genericfeatures/features.go b/internal/typeparams/genericfeatures/features.go new file mode 100644 index 0000000000..8ceef86745 --- /dev/null +++ b/internal/typeparams/genericfeatures/features.go @@ -0,0 +1,105 @@ +// Copyright 2021 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. + +// The genericfeatures package provides utilities for detecting usage of +// generic programming in Go packages. +package genericfeatures + +import ( + "go/ast" + "go/types" + "strings" + + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/internal/typeparams" +) + +// Features is a set of flags reporting which features of generic Go code a +// package uses, or 0. +type Features int + +const ( + // GenericTypeDecls indicates whether the package declares types with type + // parameters. + GenericTypeDecls Features = 1 << iota + + // GenericFuncDecls indicates whether the package declares functions with + // type parameters. + GenericFuncDecls + + // EmbeddedTypeSets indicates whether the package declares interfaces that + // contain structural type restrictions, i.e. are not fully described by + // their method sets. + EmbeddedTypeSets + + // TypeInstantiation indicates whether the package instantiates any generic + // types. + TypeInstantiation + + // FuncInstantiation indicates whether the package instantiates any generic + // functions. + FuncInstantiation +) + +func (f Features) String() string { + var feats []string + if f&GenericTypeDecls != 0 { + feats = append(feats, "typeDecl") + } + if f&GenericFuncDecls != 0 { + feats = append(feats, "funcDecl") + } + if f&EmbeddedTypeSets != 0 { + feats = append(feats, "typeSet") + } + if f&TypeInstantiation != 0 { + feats = append(feats, "typeInstance") + } + if f&FuncInstantiation != 0 { + feats = append(feats, "funcInstance") + } + return "features{" + strings.Join(feats, ",") + "}" +} + +// ForPackage computes which generic features are used directly by the +// package being analyzed. +func ForPackage(inspect *inspector.Inspector, info *types.Info) Features { + nodeFilter := []ast.Node{ + (*ast.FuncType)(nil), + (*ast.InterfaceType)(nil), + (*ast.ImportSpec)(nil), + (*ast.TypeSpec)(nil), + } + + var direct Features + + inspect.Preorder(nodeFilter, func(node ast.Node) { + switch n := node.(type) { + case *ast.FuncType: + if tparams := typeparams.ForFuncType(n); tparams != nil { + direct |= GenericFuncDecls + } + case *ast.InterfaceType: + tv := info.Types[n] + if iface, _ := tv.Type.(*types.Interface); iface != nil && !typeparams.IsMethodSet(iface) { + direct |= EmbeddedTypeSets + } + case *ast.TypeSpec: + if tparams := typeparams.ForTypeSpec(n); tparams != nil { + direct |= GenericTypeDecls + } + } + }) + + instances := typeparams.GetInstances(info) + for _, inst := range instances { + switch inst.Type.(type) { + case *types.Named: + direct |= TypeInstantiation + case *types.Signature: + direct |= FuncInstantiation + } + } + return direct +}