]> Cypherpunks repositories - gostls13.git/commitdiff
[release-branch.go1.24] go/types,types2: externalize used objects
authorRob Findley <rfindley@google.com>
Wed, 19 Feb 2025 22:07:09 +0000 (22:07 +0000)
committerGopher Robot <gobot@golang.org>
Tue, 25 Mar 2025 16:45:16 +0000 (09:45 -0700)
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
(cherry picked from commit 9189921e4759055141b51fdbb8b7b69ee4fdd477)
Reviewed-on: https://go-review.googlesource.com/c/go/+/657675
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>

16 files changed:
src/cmd/compile/internal/types2/assignments.go
src/cmd/compile/internal/types2/call.go
src/cmd/compile/internal/types2/check.go
src/cmd/compile/internal/types2/object.go
src/cmd/compile/internal/types2/resolver.go
src/cmd/compile/internal/types2/sizeof_test.go
src/cmd/compile/internal/types2/stmt.go
src/cmd/compile/internal/types2/typexpr.go
src/go/types/assignments.go
src/go/types/call.go
src/go/types/check.go
src/go/types/object.go
src/go/types/resolver.go
src/go/types/sizeof_test.go
src/go/types/stmt.go
src/go/types/typexpr.go

index ebe9ef11cb30b83a9b0b5facad6336722d21d865..3b5befcb8d01ee6afee43946463b2f0ebdea6290 100644 (file)
@@ -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) {
index 897c846d8ff98de099b76b01ef1837d01fb85a53..e3da8353c4ea9c4fcc39d72d52a0324ad10fdeca 100644 (file)
@@ -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)
index 52ff2ea032fa92a474d26783025834797b5e5492..43dbca7448a6327241847994016ffaaaa28048cb 100644 (file)
@@ -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.
 }
 
index f968f652aac3802e3f11f0ca8e661765c678f8e9..259481e65ecbc3bc5ae9b1bafbabf2f1244b8d18 100644 (file)
@@ -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.
index 694c035ab5b36b4bd3c9e33673d2970ae8f46042..1c4f22bfd149127ec3f482e2ed3f2293152307c8 100644 (file)
@@ -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)
                }
        }
index 740dbc92762aff8b6419660ca2c29d27eba3411b..d435c049c5b87dc44706d2c99f17d30ca79d82f7 100644 (file)
@@ -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},
index c46ea7a091afd2df4827f5c353209c5e2ea1b878..727891345f70a447158fa86130b2072ab45a64ef 100644 (file)
@@ -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
                        }
 
index e9b5ca9aa6c329068d2917da2dbf5dd96fca3ff9..fc885b6213e532d556e7050769c2ea12faaf91d7 100644 (file)
@@ -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) {
index 20d400bf1e894795fad0845a05d97bf4faaba8f5..31d165b6fd556a06619e72f1acdbefbf21520db6 100644 (file)
@@ -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) {
index 4e8dfc0d6bdc9d09e439643502f7d3e38c6edf8c..8a664b88ba09a7f6f800bcfdc62fdb2fcaab72a4 100644 (file)
@@ -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)
index 8c68a1aafd10901d48e0cea0a7f807937905e9be..eda0a58ad0fdf3aa22f1a33c2bee1a8c0dd1adda 100644 (file)
@@ -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.
index 80cd650ff154db6e77f25171b8838b2ff625c254..ee5483763d62f0ebb039645ef1fddffdf781b708 100644 (file)
@@ -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.
index ef9ffb5013e0445aa6a14a5c7376dd4c59a049b9..2353b638378287f83bd19e27ee0990d395675662 100644 (file)
@@ -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)
                }
        }
index 9e5b5f8b20398a243416e5a6cb84e4bce9c2022c..fa07eb10f1907c8671699110ca0557623b75eaf9 100644 (file)
@@ -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},
index de3d01e8dd2b43f0494da54816802caaffb55adf..2efaeee30eb7235b8322d59e08dd96b6e50d62ee 100644 (file)
@@ -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
                        }
 
index 7928ed8ef368de6363b5376faf9c633fc1808b9b..bd5cf9d40cb8833c4fb5ec2011b3ebbf6448f572 100644 (file)
@@ -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) {