Background: A types.Var represents a variable, broadly defined: a global, a local (including parameters and named results), or a struct field. Two of these cases can be discriminated thus:
- v.IsField() reports whether the var is a struct field.
- v.Parent() == v.Pkg().Scope() reports whether the var is a global.
But for the local variables, one is out of luck.
Proposal: We propose to add a Kind method and enum type that reports what kind of var this is.
package types
type VarKind uint8
const (
PackageVar VarKind = iota
LocalVar
RecvVar
ParamVar
ResultVar
FieldVar
)
// Kind reports what kind of variable v is.
func (v *Var) Kind() VarKind
// The setter is provided for use by importers.
func (*Var) SetKind(VarKind)
The actual implementation would replace the existing isField bool, so there is no space cost.
@griesemer @findleyr @timothy-king
Comment From: timothy-king
- The spec does not mention "global". We may want to write this as "PackageVar" instead.
- Would it be better to have a helper
func (v *Var) IsPackageScope() bool { return v.Parent() == v.Pkg().Scope()}than distinguishing PackageVar and LocalVar as different Kinds?
Comment From: adonovan
- I agree, PackageVar is better. I have updated the proposal.
- Package vars are already easy to distinguish. The real challenge is params (and results) versus ordinary locals, so I don't think IsPackageScope buys us anything.
Comment From: mvdan
Ooh, I've wanted this before, and it would make some code of mine simpler.
Comment From: timothy-king
@adonovan I guess a better way to phrase my question is do we need both PackageVar and LocalVar? The 1-line snippet you gave suggests we could do without both. (All of the other values LGTM.)
Comment From: adonovan
@adonovan I guess a better way to phrase my question is do we need both PackageVar and LocalVar? The 1-line snippet you gave suggests we could do without both. (All of the other values LGTM.)
If you had IsParam and IsResult, then you could deduce IsLocal by exclusion of the other five possibilities. But to implement that you would still need an enum internally, so why not expose it? v.Kind()==PackageVar is clearer than v.Parent() == v.Pkg().Scope().
(We have a set of accessors for the enum that is internal to TypeAndValue. I have often thought it would be more convenient if the mode itself were a first-class enum instead of a set of answers to overlapping boolean predicates. Let's not make that mistake again.)
Comment From: griesemer
What about RecvVar?
Comment From: adonovan
What about
RecvVar?
Good point. We should add that too.
Comment From: findleyr
This would be very useful, in general.
Bikeshedding, what is the heuristic for the ordering of these enum values? If it is approximate source order, perhaps the following order makes more sense (swapping FieldVar and LocalVar).
PackageVar VarKind = iota
FieldVar
RecvVar
ParamVar
ResultVar
LocalVar
```
**Comment From: adonovan**
I put FieldVar last because a field is not a variable according to the spec, only according to go/types. But I don't think it really matters.
**Comment From: rsc**
This proposal has been added to the [active column](https://go.dev/s/proposal-status#active) of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group
**Comment From: aclements**
Let's start the enum at 1 so the zero value is invalid. We don't have to give that a name (just `_`). A given variable will never have that kind, but it's useful for declaring "not yet initialized" values.
**Comment From: aclements**
Based on the discussion above, this proposal seems like a **[likely accept](https://go.dev/s/proposal-status#likely-accept)**.
— aclements for the proposal review group
The proposal is to add the following to package `types`:
```go
package types
type VarKind uint8
const (
_ VarKind = iota // The zero value is not meaningful
PackageVar
LocalVar
RecvVar
ParamVar
ResultVar
FieldVar
)
// Kind reports what kind of variable v is.
func (v *Var) Kind() VarKind
// The setter is provided for use by importers.
func (*Var) SetKind(VarKind)
func (VarKind) String() string
Comment From: aclements
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — aclements for the proposal review group
The proposal is to add the following to package types:
package types
type VarKind uint8
const (
_ VarKind = iota // The zero value is not meaningful
PackageVar
LocalVar
RecvVar
ParamVar
ResultVar
FieldVar
)
// Kind reports what kind of variable v is.
func (v *Var) Kind() VarKind
// The setter is provided for use by importers.
func (*Var) SetKind(VarKind)
func (VarKind) String() string
Comment From: gopherbot
Change https://go.dev/cl/645115 mentions this issue: go/types: add Var.Kind() VarKind method
Comment From: gopherbot
Change https://go.dev/cl/645656 mentions this issue: x/tools: use types.VarKind consistently
Comment From: adonovan
The implementation is now merged. @griesemer and I still have one question to resolve about whether EmbeddedFieldKind should be a distinct VarKind representing embedded fields. This would eliminate the need for Var.IsField and save a boolean in Var; but IMHO embeddedness is an an orthogonal concept to VarKind.
Comment From: aclements
@adonovan , did your question about EmbeddedFieldKind get resolved? Should this issue be closed now?
Comment From: adonovan
Not yet. I remain unconvinced that EmbeddedFieldKind is a good idea. An embedded field is still essentially a field in most places where the code cares to distinguish. I think we should proceed as is, but @griesemer should confirm.
Comment From: findleyr
@adonovan, in https://github.com/golang/go/issues/70250#issuecomment-2701736902 above you mean eliminate the need for Embedded, right? IsField is already obsolete.
I can't say whether Embeddedness is orthogonal to variable kind, because variable kind is not defined (or rather, its definition is given by the possible VarKind values). What is a VarKind?
If VarKind is an abstract concept that represents the semantics of a variable, we should clarify which semantics we're talking about. There are some ways that an embedded field is like a field, and other ways where it is not.
Comment From: griesemer
I'm not going to press for this, but if it was me, I would make the change because it provides a single, unified way to create variables (at least internally), which is with newVar.
If we wanted, we could even export a NewVar2 and deprecate NewField, NewParam; i.e., if we were defining the external API today, it would be simpler and more uniform.
Here's the diff (this also eliminates internal uses of NewParam):
diff --git a/src/cmd/compile/internal/types2/builtins.go b/src/cmd/compile/internal/types2/builtins.go
index fe46b4e997..2a1a80e7d0 100644
--- a/src/cmd/compile/internal/types2/builtins.go
+++ b/src/cmd/compile/internal/types2/builtins.go
@@ -1082,7 +1082,7 @@ func (check *Checker) applyTypeFunc(f func(Type) Type, x *operand, id builtinId)
func makeSig(res Type, args ...Type) *Signature {
list := make([]*Var, len(args))
for i, param := range args {
- list[i] = NewParam(nopos, nil, "", Default(param))
+ list[i] = newVar(ParamVar, nopos, nil, "", Default(param))
}
params := NewTuple(list...)
var result *Tuple
diff --git a/src/cmd/compile/internal/types2/call.go b/src/cmd/compile/internal/types2/call.go
index b7a2ebb41e..6935bf9374 100644
--- a/src/cmd/compile/internal/types2/call.go
+++ b/src/cmd/compile/internal/types2/call.go
@@ -94,7 +94,7 @@ func (check *Checker) funcInst(T *target, pos syntax.Pos, x *operand, inst *synt
}
}
gsig := NewSignatureType(nil, nil, nil, sig.params, sig.results, sig.variadic)
- params = []*Var{NewParam(x.Pos(), check.pkg, "", gsig)}
+ params = []*Var{newVar(ParamVar, x.Pos(), check.pkg, "", gsig)}
// The type of the argument operand is tsig, which is the type of the LHS in an assignment
// or the result type in a return statement. Create a pseudo-expression for that operand
// that makes sense when reported in error messages from infer, below.
@@ -496,7 +496,7 @@ func (check *Checker) arguments(call *syntax.CallExpr, sig *Signature, targs []T
last := sig.params.vars[npars-1]
typ := last.typ.(*Slice).elem
for len(vars) < nargs {
- vars = append(vars, NewParam(last.pos, last.pkg, last.name, typ))
+ vars = append(vars, newVar(ParamVar, last.pos, last.pkg, last.name, typ))
}
sigParams = NewTuple(vars...) // possibly nil!
adjusted = true
@@ -883,7 +883,7 @@ func (check *Checker) selector(x *operand, e *syntax.SelectorExpr, def *TypeName
name = "_"
}
}
- params = append([]*Var{NewParam(sig.recv.pos, sig.recv.pkg, name, x.typ)}, params...)
+ params = append([]*Var{newVar(ParamVar, sig.recv.pos, sig.recv.pkg, name, x.typ)}, params...)
x.mode = value
x.typ = &Signature{
tparams: sig.tparams,
diff --git a/src/cmd/compile/internal/types2/check.go b/src/cmd/compile/internal/types2/check.go
index 31a1aa2abe..50201a656b 100644
--- a/src/cmd/compile/internal/types2/check.go
+++ b/src/cmd/compile/internal/types2/check.go
@@ -617,8 +617,8 @@ func (check *Checker) recordCommaOkTypesInSyntax(x syntax.Expr, t0, t1 Type) {
assert(tv.Type != nil) // should have been recorded already
pos := x.Pos()
tv.Type = NewTuple(
- NewParam(pos, check.pkg, "", t0),
- NewParam(pos, check.pkg, "", t1),
+ newVar(ParamVar, pos, check.pkg, "", t0),
+ newVar(ParamVar, pos, check.pkg, "", t1),
)
x.SetTypeInfo(tv)
p, _ := x.(*syntax.ParenExpr)
diff --git a/src/cmd/compile/internal/types2/context_test.go b/src/cmd/compile/internal/types2/context_test.go
index 2c5c0d8ef3..76d0ae0a70 100644
--- a/src/cmd/compile/internal/types2/context_test.go
+++ b/src/cmd/compile/internal/types2/context_test.go
@@ -38,7 +38,7 @@ func TestContextHashCollisions(t *testing.T) {
{
// type unaryP = func[P any](_ P)
tparam := NewTypeParam(NewTypeName(nopos, nil, "P", nil), &emptyInterface)
- params := NewTuple(NewParam(nopos, nil, "_", tparam))
+ params := NewTuple(newVar(ParamVar, nopos, nil, "_", tparam))
unaryP = NewSignatureType(nil, nil, []*TypeParam{tparam}, params, nil, false)
}
diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go
index 624b510dc8..ccaae705c1 100644
--- a/src/cmd/compile/internal/types2/lookup.go
+++ b/src/cmd/compile/internal/types2/lookup.go
@@ -225,7 +225,7 @@ func lookupFieldOrMethodImpl(T Type, addressable bool, pkg *Package, name string
// T is a type name. If e.typ appeared multiple times at
// this depth, f.typ appears multiple times at the next
// depth.
- if obj == nil && f.embedded {
+ if obj == nil && f.Embedded() {
typ, isPtr := deref(f.typ)
// TODO(gri) optimization: ignore types that can't
// have fields or methods (only Named, Struct, and
@@ -540,7 +540,7 @@ func hasInvalidEmbeddedFields(T Type, seen map[*Struct]bool) bool {
}
seen[S] = true
for _, f := range S.fields {
- if f.embedded && (!isValid(f.typ) || hasInvalidEmbeddedFields(f.typ, seen)) {
+ if f.Embedded() && (!isValid(f.typ) || hasInvalidEmbeddedFields(f.typ, seen)) {
return true
}
}
diff --git a/src/cmd/compile/internal/types2/object.go b/src/cmd/compile/internal/types2/object.go
index 26752c44b0..e346f249fc 100644
--- a/src/cmd/compile/internal/types2/object.go
+++ b/src/cmd/compile/internal/types2/object.go
@@ -330,32 +330,33 @@ 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
- kind VarKind
- embedded bool // if set, the variable is an embedded struct field, and name is the type name
+ origin *Var // if non-nil, the Var from which this one was instantiated
+ kind VarKind
}
// A VarKind discriminates the various kinds of variables.
type VarKind uint8
const (
- _ VarKind = iota // (not meaningful)
- PackageVar // a package-level variable
- LocalVar // a local variable
- RecvVar // a method receiver variable
- ParamVar // a function parameter variable
- ResultVar // a function result variable
- FieldVar // a struct field
+ _ VarKind = iota // (not meaningful)
+ PackageVar // a package-level variable
+ LocalVar // a local variable
+ RecvVar // a method receiver variable
+ ParamVar // a function parameter variable
+ ResultVar // a function result variable
+ FieldVar // a struct field
+ EmbeddedVar // an embedded struct field
)
var varKindNames = [...]string{
- 0: "VarKind(0)",
- PackageVar: "PackageVar",
- LocalVar: "LocalVar",
- RecvVar: "RecvVar",
- ParamVar: "ParamVar",
- ResultVar: "ResultVar",
- FieldVar: "FieldVar",
+ 0: "VarKind(0)",
+ PackageVar: "PackageVar",
+ LocalVar: "LocalVar",
+ RecvVar: "RecvVar",
+ ParamVar: "ParamVar",
+ ResultVar: "ResultVar",
+ FieldVar: "FieldVar",
+ EmbeddedVar: "EmbeddedVar",
}
func (kind VarKind) String() string {
@@ -393,9 +394,11 @@ func NewParam(pos syntax.Pos, pkg *Package, name string, typ Type) *Var {
// For embedded fields, the name is the unqualified type name
// under which the field is accessible.
func NewField(pos syntax.Pos, pkg *Package, name string, typ Type, embedded bool) *Var {
- v := newVar(FieldVar, pos, pkg, name, typ)
- v.embedded = embedded
- return v
+ kind := FieldVar
+ if embedded {
+ kind = EmbeddedVar
+ }
+ return newVar(kind, pos, pkg, name, typ)
}
// newVar returns a new variable.
@@ -406,13 +409,13 @@ func newVar(kind VarKind, pos syntax.Pos, pkg *Package, name string, typ Type) *
// Anonymous reports whether the variable is an embedded field.
// Same as Embedded; only present for backward-compatibility.
-func (obj *Var) Anonymous() bool { return obj.embedded }
+func (obj *Var) Anonymous() bool { return obj.kind == EmbeddedVar }
// Embedded reports whether the variable is an embedded field.
-func (obj *Var) Embedded() bool { return obj.embedded }
+func (obj *Var) Embedded() bool { return obj.kind == EmbeddedVar }
// IsField reports whether the variable is a struct field.
-func (obj *Var) IsField() bool { return obj.kind == FieldVar }
+func (obj *Var) IsField() bool { return obj.kind == FieldVar || obj.kind == EmbeddedVar }
// Origin returns the canonical Var for its receiver, i.e. the Var object
// recorded in Info.Defs.
diff --git a/src/cmd/compile/internal/types2/predicates.go b/src/cmd/compile/internal/types2/predicates.go
index c157672ba5..8efac9b02b 100644
--- a/src/cmd/compile/internal/types2/predicates.go
+++ b/src/cmd/compile/internal/types2/predicates.go
@@ -291,7 +291,7 @@ func (c *comparer) identical(x, y Type, p *ifacePair) bool {
if x.NumFields() == y.NumFields() {
for i, f := range x.fields {
g := y.fields[i]
- if f.embedded != g.embedded ||
+ if f.kind != g.kind ||
!c.ignoreTags && x.Tag(i) != y.Tag(i) ||
!f.sameId(g.pkg, g.name, false) ||
!c.identical(f.typ, g.typ, p) {
diff --git a/src/cmd/compile/internal/types2/struct.go b/src/cmd/compile/internal/types2/struct.go
index f5cdc472f7..14e61302cb 100644
--- a/src/cmd/compile/internal/types2/struct.go
+++ b/src/cmd/compile/internal/types2/struct.go
@@ -80,7 +80,7 @@ func (check *Checker) structType(styp *Struct, e *syntax.StructType) {
// current field typ and tag
var typ Type
var tag string
- add := func(ident *syntax.Name, embedded bool) {
+ add := func(ident *syntax.Name, kind VarKind) {
if tag != "" && tags == nil {
tags = make([]string, len(fields))
}
@@ -90,7 +90,7 @@ func (check *Checker) structType(styp *Struct, e *syntax.StructType) {
pos := ident.Pos()
name := ident.Value
- fld := NewField(pos, check.pkg, name, typ, embedded)
+ fld := newVar(kind, pos, check.pkg, name, typ)
// spec: "Within a struct, non-blank field names must be unique."
if name == "_" || check.declareInSet(&fset, pos, fld) {
fields = append(fields, fld)
@@ -105,7 +105,7 @@ func (check *Checker) structType(styp *Struct, e *syntax.StructType) {
addInvalid := func(ident *syntax.Name) {
typ = Typ[Invalid]
tag = ""
- add(ident, true)
+ add(ident, EmbeddedVar)
}
var prev syntax.Expr
@@ -122,7 +122,7 @@ func (check *Checker) structType(styp *Struct, e *syntax.StructType) {
}
if f.Name != nil {
// named field
- add(f.Name, false)
+ add(f.Name, FieldVar)
} else {
// embedded field
// spec: "An embedded type must be specified as a type name T or as a
@@ -136,7 +136,7 @@ func (check *Checker) structType(styp *Struct, e *syntax.StructType) {
addInvalid(name)
continue
}
- add(name, true) // struct{p.T} field has position of T
+ add(name, EmbeddedVar) // struct{p.T} field has position of T
// Because we have a name, typ must be of the form T or *T, where T is the name
// of a (named or alias) type, and t (= deref(typ)) must be the type of T.
diff --git a/src/cmd/compile/internal/types2/typestring.go b/src/cmd/compile/internal/types2/typestring.go
index 47f53bc12d..c96f49a34c 100644
--- a/src/cmd/compile/internal/types2/typestring.go
+++ b/src/cmd/compile/internal/types2/typestring.go
@@ -161,7 +161,7 @@ func (w *typeWriter) typ(typ Type) {
// This doesn't do the right thing for embedded type
// aliases where we should print the alias name, not
// the aliased type (see go.dev/issue/44410).
- if !f.embedded {
+ if !f.Embedded() {
w.string(f.name)
w.byte(' ')
}
diff --git a/src/cmd/compile/internal/types2/unify.go b/src/cmd/compile/internal/types2/unify.go
index 9cd3af8607..92a95eec3d 100644
--- a/src/cmd/compile/internal/types2/unify.go
+++ b/src/cmd/compile/internal/types2/unify.go
@@ -607,7 +607,7 @@ func (u *unifier) nify(x, y Type, mode unifyMode, p *ifacePair) (result bool) {
if x.NumFields() == y.NumFields() {
for i, f := range x.fields {
g := y.fields[i]
- if f.embedded != g.embedded ||
+ if f.kind != g.kind ||
x.Tag(i) != y.Tag(i) ||
!f.sameId(g.pkg, g.name, false) ||
!u.nify(f.typ, g.typ, emode, p) {
(END)
Comment From: cherrymui
@griesemer mentioned that we could also leave EmbeddedFieldKind out for Go 1.25 for now, but add it later (in a backward compatible way) if we decide to. Suppose we add it later, are we expecting users to write certain code differently? Or any other issues?
Comment From: adonovan
I don't think adding to the enum later helps anyone. Whatever change we make, we should do it now.
Comment From: aclements
Given that it's not obvious whether we should or shouldn't add it, I think we should stick with the API we have now.
There's always the option of passing around a VarKind and an embeddedness bool where that matters. That would be a fine way to unify newVar. If we want to export that, we could add, say, VarFlags, which would have a bit for embeddedness. If we care about shrinking Var, we could squirrel away the embedded flag as a bit in the kind and just mask it out.
We're not cutting off our options if we go ahead with VarKind as-is.
Comment From: aclements
Given that there aren't strong opinions in favor of adding EmbeddedFieldKind, some skepticism over adding it, and that there seem to be perfectly fine alternatives, let's leave the API as accepted.