From e9162e7e22e6030f52c275fdafe789d8ef73d882 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 19 Feb 2025 22:07:09 +0000 Subject: [PATCH] [release-branch.go1.24] go/types,types2: externalize used objects The 'used' field on Var and PkgName is fundamentally an aspect of the type checking pass: it records when objects are used, for the purposes of reporting errors for unused variables or package names. While expedient and performant, recording this information in the types.Object instances themselves increases the memory footprint of type-checked packages, and (as we saw in golang/go#71817) can lead to data races when Objects are reused in follow-up type checking, such as is done with the CheckExpr and Eval APIs. Fix this by externalizing the 'used' information into two maps (one for variables and one for packages) on the types.Checker, so that they are garbage-collected after type checking, and cannot be a source of data races. Benchmarks showed essentially no change in performance. For #72826 Change-Id: I40daeabe4ecaca3bcb494e2f1c62a04232098e49 Reviewed-on: https://go-review.googlesource.com/c/go/+/650796 LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley Reviewed-by: Robert Griesemer (cherry picked from commit 9189921e4759055141b51fdbb8b7b69ee4fdd477) Reviewed-on: https://go-review.googlesource.com/c/go/+/657675 Auto-Submit: Dmitri Shuralyov --- .../compile/internal/types2/assignments.go | 4 +-- src/cmd/compile/internal/types2/call.go | 6 ++--- src/cmd/compile/internal/types2/check.go | 21 +++++++++++----- src/cmd/compile/internal/types2/object.go | 9 +++---- src/cmd/compile/internal/types2/resolver.go | 4 +-- .../compile/internal/types2/sizeof_test.go | 2 +- src/cmd/compile/internal/types2/stmt.go | 8 +++--- src/cmd/compile/internal/types2/typexpr.go | 6 ++--- src/go/types/assignments.go | 4 +-- src/go/types/call.go | 6 ++--- src/go/types/check.go | 25 +++++++++++++------ src/go/types/object.go | 9 +++---- src/go/types/resolver.go | 4 +-- src/go/types/sizeof_test.go | 2 +- src/go/types/stmt.go | 11 +++++--- src/go/types/typexpr.go | 6 ++--- 16 files changed, 73 insertions(+), 54 deletions(-) diff --git a/src/cmd/compile/internal/types2/assignments.go b/src/cmd/compile/internal/types2/assignments.go index ebe9ef11cb..3b5befcb8d 100644 --- a/src/cmd/compile/internal/types2/assignments.go +++ b/src/cmd/compile/internal/types2/assignments.go @@ -204,7 +204,7 @@ func (check *Checker) lhsVar(lhs syntax.Expr) Type { // dot-imported variables. if w, _ := obj.(*Var); w != nil && w.pkg == check.pkg { v = w - v_used = v.used + v_used = check.usedVars[v] } } } @@ -213,7 +213,7 @@ func (check *Checker) lhsVar(lhs syntax.Expr) Type { check.expr(nil, &x, lhs) if v != nil { - v.used = v_used // restore v.used + check.usedVars[v] = v_used // restore v.used } if x.mode == invalid || !isValid(x.typ) { diff --git a/src/cmd/compile/internal/types2/call.go b/src/cmd/compile/internal/types2/call.go index 897c846d8f..e3da8353c4 100644 --- a/src/cmd/compile/internal/types2/call.go +++ b/src/cmd/compile/internal/types2/call.go @@ -687,7 +687,7 @@ func (check *Checker) selector(x *operand, e *syntax.SelectorExpr, def *TypeName if pname, _ := obj.(*PkgName); pname != nil { assert(pname.pkg == check.pkg) check.recordUse(ident, pname) - pname.used = true + check.usedPkgNames[pname] = true pkg := pname.imported var exp Object @@ -972,13 +972,13 @@ func (check *Checker) use1(e syntax.Expr, lhs bool) bool { // dot-imported variables. if w, _ := obj.(*Var); w != nil && w.pkg == check.pkg { v = w - v_used = v.used + v_used = check.usedVars[v] } } } check.exprOrType(&x, n, true) if v != nil { - v.used = v_used // restore v.used + check.usedVars[v] = v_used // restore v.used } case *syntax.ListExpr: return check.useN(n.ElemList, lhs) diff --git a/src/cmd/compile/internal/types2/check.go b/src/cmd/compile/internal/types2/check.go index 52ff2ea032..43dbca7448 100644 --- a/src/cmd/compile/internal/types2/check.go +++ b/src/cmd/compile/internal/types2/check.go @@ -162,6 +162,8 @@ type Checker struct { dotImportMap map[dotImportKey]*PkgName // maps dot-imported objects to the package they were dot-imported through brokenAliases map[*TypeName]bool // set of aliases with broken (not yet determined) types unionTypeSets map[*Union]*_TypeSet // computed type sets for union types + usedVars map[*Var]bool // set of used variables + usedPkgNames map[*PkgName]bool // set of used package names mono monoGraph // graph for detecting non-monomorphizable instantiation loops firstErr error // first error encountered @@ -285,12 +287,14 @@ func NewChecker(conf *Config, pkg *Package, info *Info) *Checker { // (previously, pkg.goVersion was mutated here: go.dev/issue/61212) return &Checker{ - conf: conf, - ctxt: conf.Context, - pkg: pkg, - Info: info, - 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), + usedVars: make(map[*Var]bool), + usedPkgNames: make(map[*PkgName]bool), } } @@ -298,6 +302,8 @@ func NewChecker(conf *Config, pkg *Package, info *Info) *Checker { // The provided files must all belong to the same package. func (check *Checker) initFiles(files []*syntax.File) { // start with a clean slate (check.Files may be called multiple times) + // TODO(gri): what determines which fields are zeroed out here, vs at the end + // of checkFiles? check.files = nil check.imports = nil check.dotImportMap = nil @@ -482,8 +488,11 @@ func (check *Checker) checkFiles(files []*syntax.File) { check.seenPkgMap = nil check.brokenAliases = nil check.unionTypeSets = nil + check.usedVars = nil + check.usedPkgNames = nil check.ctxt = nil + // TODO(gri): shouldn't the cleanup above occur after the bailout? // TODO(gri) There's more memory we should release at this point. } diff --git a/src/cmd/compile/internal/types2/object.go b/src/cmd/compile/internal/types2/object.go index f968f652aa..259481e65e 100644 --- a/src/cmd/compile/internal/types2/object.go +++ b/src/cmd/compile/internal/types2/object.go @@ -242,13 +242,12 @@ func (a *object) cmp(b *object) int { type PkgName struct { object imported *Package - used bool // set if the package was used } // NewPkgName returns a new PkgName object representing an imported package. // The remaining arguments set the attributes found with all Objects. func NewPkgName(pos syntax.Pos, pkg *Package, name string, imported *Package) *PkgName { - return &PkgName{object{nil, pos, pkg, name, Typ[Invalid], 0, black, nopos}, imported, false} + return &PkgName{object{nil, pos, pkg, name, Typ[Invalid], 0, black, nopos}, imported} } // Imported returns the package that was imported. @@ -331,10 +330,10 @@ func (obj *TypeName) IsAlias() bool { // A Variable represents a declared variable (including function parameters and results, and struct fields). type Var struct { object + origin *Var // if non-nil, the Var from which this one was instantiated embedded bool // if set, the variable is an embedded struct field, and name is the type name isField bool // var is struct field - used bool // set if the variable was used - origin *Var // if non-nil, the Var from which this one was instantiated + isParam bool // var is a param, for backport of 'used' check to go1.24 (go.dev/issue/72826) } // NewVar returns a new variable. @@ -345,7 +344,7 @@ func NewVar(pos syntax.Pos, pkg *Package, name string, typ Type) *Var { // NewParam returns a new variable representing a function parameter. func NewParam(pos syntax.Pos, pkg *Package, name string, typ Type) *Var { - return &Var{object: object{nil, pos, pkg, name, typ, 0, colorFor(typ), nopos}, used: true} // parameters are always 'used' + return &Var{object: object{nil, pos, pkg, name, typ, 0, colorFor(typ), nopos}, isParam: true} } // NewField returns a new variable representing a struct field. diff --git a/src/cmd/compile/internal/types2/resolver.go b/src/cmd/compile/internal/types2/resolver.go index 694c035ab5..1c4f22bfd1 100644 --- a/src/cmd/compile/internal/types2/resolver.go +++ b/src/cmd/compile/internal/types2/resolver.go @@ -295,7 +295,7 @@ func (check *Checker) collectObjects() { if imp.fake { // match 1.17 cmd/compile (not prescribed by spec) - pkgName.used = true + check.usedPkgNames[pkgName] = true } // add import to file scope @@ -715,7 +715,7 @@ func (check *Checker) unusedImports() { // (initialization), use the blank identifier as explicit package name." for _, obj := range check.imports { - if !obj.used && obj.name != "_" { + if obj.name != "_" && !check.usedPkgNames[obj] { check.errorUnusedPkg(obj) } } diff --git a/src/cmd/compile/internal/types2/sizeof_test.go b/src/cmd/compile/internal/types2/sizeof_test.go index 740dbc9276..d435c049c5 100644 --- a/src/cmd/compile/internal/types2/sizeof_test.go +++ b/src/cmd/compile/internal/types2/sizeof_test.go @@ -36,7 +36,7 @@ func TestSizeof(t *testing.T) { {term{}, 12, 24}, // Objects - {PkgName{}, 64, 104}, + {PkgName{}, 60, 96}, {Const{}, 64, 104}, {TypeName{}, 56, 88}, {Var{}, 64, 104}, diff --git a/src/cmd/compile/internal/types2/stmt.go b/src/cmd/compile/internal/types2/stmt.go index c46ea7a091..727891345f 100644 --- a/src/cmd/compile/internal/types2/stmt.go +++ b/src/cmd/compile/internal/types2/stmt.go @@ -58,7 +58,7 @@ func (check *Checker) usage(scope *Scope) { var unused []*Var for name, elem := range scope.elems { elem = resolve(name, elem) - if v, _ := elem.(*Var); v != nil && !v.used { + if v, _ := elem.(*Var); v != nil && !v.isParam && !check.usedVars[v] { unused = append(unused, v) } } @@ -824,10 +824,10 @@ func (check *Checker) typeSwitchStmt(inner stmtContext, s *syntax.SwitchStmt, gu if lhs != nil { var used bool for _, v := range lhsVars { - if v.used { + if check.usedVars[v] { used = true } - v.used = true // avoid usage error when checking entire function + check.usedVars[v] = true // avoid usage error when checking entire function } if !used { check.softErrorf(lhs, UnusedVar, "%s declared and not used", lhs.Value) @@ -934,7 +934,7 @@ func (check *Checker) rangeStmt(inner stmtContext, s *syntax.ForStmt, rclause *s if typ == nil || typ == Typ[Invalid] { // typ == Typ[Invalid] can happen if allowVersion fails. obj.typ = Typ[Invalid] - obj.used = true // don't complain about unused variable + check.usedVars[obj] = true // don't complain about unused variable continue } diff --git a/src/cmd/compile/internal/types2/typexpr.go b/src/cmd/compile/internal/types2/typexpr.go index e9b5ca9aa6..fc885b6213 100644 --- a/src/cmd/compile/internal/types2/typexpr.go +++ b/src/cmd/compile/internal/types2/typexpr.go @@ -55,7 +55,7 @@ func (check *Checker) ident(x *operand, e *syntax.Name, def *TypeName, wantType // avoid "declared but not used" errors // (don't use Checker.use - we don't want to evaluate too much) if v, _ := obj.(*Var); v != nil && v.pkg == check.pkg /* see Checker.use1 */ { - v.used = true + check.usedVars[v] = true } return } @@ -83,7 +83,7 @@ func (check *Checker) ident(x *operand, e *syntax.Name, def *TypeName, wantType // (This code is only needed for dot-imports. Without them, // we only have to mark variables, see *Var case below). if pkgName := check.dotImportMap[dotImportKey{scope, obj.Name()}]; pkgName != nil { - pkgName.used = true + check.usedPkgNames[pkgName] = true } switch obj := obj.(type) { @@ -120,7 +120,7 @@ func (check *Checker) ident(x *operand, e *syntax.Name, def *TypeName, wantType // from other packages to avoid potential race conditions with // dot-imported variables. if obj.pkg == check.pkg { - obj.used = true + check.usedVars[obj] = true } check.addDeclDep(obj) if !isValid(typ) { diff --git a/src/go/types/assignments.go b/src/go/types/assignments.go index 20d400bf1e..31d165b6fd 100644 --- a/src/go/types/assignments.go +++ b/src/go/types/assignments.go @@ -207,7 +207,7 @@ func (check *Checker) lhsVar(lhs ast.Expr) Type { // dot-imported variables. if w, _ := obj.(*Var); w != nil && w.pkg == check.pkg { v = w - v_used = v.used + v_used = check.usedVars[v] } } } @@ -216,7 +216,7 @@ func (check *Checker) lhsVar(lhs ast.Expr) Type { check.expr(nil, &x, lhs) if v != nil { - v.used = v_used // restore v.used + check.usedVars[v] = v_used // restore v.used } if x.mode == invalid || !isValid(x.typ) { diff --git a/src/go/types/call.go b/src/go/types/call.go index 4e8dfc0d6b..8a664b88ba 100644 --- a/src/go/types/call.go +++ b/src/go/types/call.go @@ -689,7 +689,7 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr, def *TypeName, w if pname, _ := obj.(*PkgName); pname != nil { assert(pname.pkg == check.pkg) check.recordUse(ident, pname) - pname.used = true + check.usedPkgNames[pname] = true pkg := pname.imported var exp Object @@ -1020,13 +1020,13 @@ func (check *Checker) use1(e ast.Expr, lhs bool) bool { // dot-imported variables. if w, _ := obj.(*Var); w != nil && w.pkg == check.pkg { v = w - v_used = v.used + v_used = check.usedVars[v] } } } check.exprOrType(&x, n, true) if v != nil { - v.used = v_used // restore v.used + check.usedVars[v] = v_used // restore v.used } default: check.rawExpr(nil, &x, e, nil, true) diff --git a/src/go/types/check.go b/src/go/types/check.go index 8c68a1aafd..eda0a58ad0 100644 --- a/src/go/types/check.go +++ b/src/go/types/check.go @@ -182,6 +182,8 @@ type Checker struct { dotImportMap map[dotImportKey]*PkgName // maps dot-imported objects to the package they were dot-imported through brokenAliases map[*TypeName]bool // set of aliases with broken (not yet determined) types unionTypeSets map[*Union]*_TypeSet // computed type sets for union types + usedVars map[*Var]bool // set of used variables + usedPkgNames map[*PkgName]bool // set of used package names mono monoGraph // graph for detecting non-monomorphizable instantiation loops firstErr error // first error encountered @@ -308,13 +310,15 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch conf._EnableAlias = gotypesalias.Value() != "0" return &Checker{ - conf: conf, - ctxt: conf.Context, - fset: fset, - pkg: pkg, - Info: info, - 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), + usedVars: make(map[*Var]bool), + usedPkgNames: make(map[*PkgName]bool), } } @@ -322,6 +326,8 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch // The provided files must all belong to the same package. func (check *Checker) initFiles(files []*ast.File) { // start with a clean slate (check.Files may be called multiple times) + // TODO(gri): what determines which fields are zeroed out here, vs at the end + // of checkFiles? check.files = nil check.imports = nil check.dotImportMap = nil @@ -507,9 +513,12 @@ func (check *Checker) checkFiles(files []*ast.File) { check.seenPkgMap = nil check.brokenAliases = nil check.unionTypeSets = nil + check.usedVars = nil + check.usedPkgNames = nil check.ctxt = nil - // TODO(rFindley) There's more memory we should release at this point. + // TODO(gri): shouldn't the cleanup above occur after the bailout? + // TODO(gri) There's more memory we should release at this point. } // processDelayed processes all delayed actions pushed after top. diff --git a/src/go/types/object.go b/src/go/types/object.go index 80cd650ff1..ee5483763d 100644 --- a/src/go/types/object.go +++ b/src/go/types/object.go @@ -245,13 +245,12 @@ func (a *object) cmp(b *object) int { type PkgName struct { object imported *Package - used bool // set if the package was used } // NewPkgName returns a new PkgName object representing an imported package. // The remaining arguments set the attributes found with all Objects. func NewPkgName(pos token.Pos, pkg *Package, name string, imported *Package) *PkgName { - return &PkgName{object{nil, pos, pkg, name, Typ[Invalid], 0, black, nopos}, imported, false} + return &PkgName{object{nil, pos, pkg, name, Typ[Invalid], 0, black, nopos}, imported} } // Imported returns the package that was imported. @@ -334,10 +333,10 @@ func (obj *TypeName) IsAlias() bool { // A Variable represents a declared variable (including function parameters and results, and struct fields). type Var struct { object + origin *Var // if non-nil, the Var from which this one was instantiated embedded bool // if set, the variable is an embedded struct field, and name is the type name isField bool // var is struct field - used bool // set if the variable was used - origin *Var // if non-nil, the Var from which this one was instantiated + isParam bool // var is a param, for backport of 'used' check to go1.24 (go.dev/issue/72826) } // NewVar returns a new variable. @@ -348,7 +347,7 @@ func NewVar(pos token.Pos, pkg *Package, name string, typ Type) *Var { // NewParam returns a new variable representing a function parameter. func NewParam(pos token.Pos, pkg *Package, name string, typ Type) *Var { - return &Var{object: object{nil, pos, pkg, name, typ, 0, colorFor(typ), nopos}, used: true} // parameters are always 'used' + return &Var{object: object{nil, pos, pkg, name, typ, 0, colorFor(typ), nopos}, isParam: true} } // NewField returns a new variable representing a struct field. diff --git a/src/go/types/resolver.go b/src/go/types/resolver.go index ef9ffb5013..2353b63837 100644 --- a/src/go/types/resolver.go +++ b/src/go/types/resolver.go @@ -310,7 +310,7 @@ func (check *Checker) collectObjects() { if imp.fake { // match 1.17 cmd/compile (not prescribed by spec) - pkgName.used = true + check.usedPkgNames[pkgName] = true } // add import to file scope @@ -710,7 +710,7 @@ func (check *Checker) unusedImports() { // (initialization), use the blank identifier as explicit package name." for _, obj := range check.imports { - if !obj.used && obj.name != "_" { + if obj.name != "_" && !check.usedPkgNames[obj] { check.errorUnusedPkg(obj) } } diff --git a/src/go/types/sizeof_test.go b/src/go/types/sizeof_test.go index 9e5b5f8b20..fa07eb10f1 100644 --- a/src/go/types/sizeof_test.go +++ b/src/go/types/sizeof_test.go @@ -35,7 +35,7 @@ func TestSizeof(t *testing.T) { {term{}, 12, 24}, // Objects - {PkgName{}, 48, 88}, + {PkgName{}, 44, 80}, {Const{}, 48, 88}, {TypeName{}, 40, 72}, {Var{}, 48, 88}, diff --git a/src/go/types/stmt.go b/src/go/types/stmt.go index de3d01e8dd..2efaeee30e 100644 --- a/src/go/types/stmt.go +++ b/src/go/types/stmt.go @@ -59,7 +59,7 @@ func (check *Checker) usage(scope *Scope) { var unused []*Var for name, elem := range scope.elems { elem = resolve(name, elem) - if v, _ := elem.(*Var); v != nil && !v.used { + if v, _ := elem.(*Var); v != nil && !v.isParam && !check.usedVars[v] { unused = append(unused, v) } } @@ -777,13 +777,16 @@ func (check *Checker) stmt(ctxt stmtContext, s ast.Stmt) { } // If lhs exists, we must have at least one lhs variable that was used. + // (We can't use check.usage because that only looks at one scope; and + // we don't want to use the same variable for all scopes and change the + // variable type underfoot.) if lhs != nil { var used bool for _, v := range lhsVars { - if v.used { + if check.usedVars[v] { used = true } - v.used = true // avoid usage error when checking entire function + check.usedVars[v] = true // avoid usage error when checking entire function } if !used { check.softErrorf(lhs, UnusedVar, "%s declared and not used", lhs.Name) @@ -952,7 +955,7 @@ func (check *Checker) rangeStmt(inner stmtContext, s *ast.RangeStmt) { if typ == nil || typ == Typ[Invalid] { // typ == Typ[Invalid] can happen if allowVersion fails. obj.typ = Typ[Invalid] - obj.used = true // don't complain about unused variable + check.usedVars[obj] = true // don't complain about unused variable continue } diff --git a/src/go/types/typexpr.go b/src/go/types/typexpr.go index 7928ed8ef3..bd5cf9d40c 100644 --- a/src/go/types/typexpr.go +++ b/src/go/types/typexpr.go @@ -54,7 +54,7 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *TypeName, wantType bo // avoid "declared but not used" errors // (don't use Checker.use - we don't want to evaluate too much) if v, _ := obj.(*Var); v != nil && v.pkg == check.pkg /* see Checker.use1 */ { - v.used = true + check.usedVars[v] = true } return } @@ -82,7 +82,7 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *TypeName, wantType bo // (This code is only needed for dot-imports. Without them, // we only have to mark variables, see *Var case below). if pkgName := check.dotImportMap[dotImportKey{scope, obj.Name()}]; pkgName != nil { - pkgName.used = true + check.usedPkgNames[pkgName] = true } switch obj := obj.(type) { @@ -119,7 +119,7 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *TypeName, wantType bo // from other packages to avoid potential race conditions with // dot-imported variables. if obj.pkg == check.pkg { - obj.used = true + check.usedVars[obj] = true } check.addDeclDep(obj) if !isValid(typ) { -- 2.48.1