Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: go/types: add Var.Kind method and enum #70250

Open
adonovan opened this issue Nov 7, 2024 · 12 comments
Open

proposal: go/types: add Var.Kind method and enum #70250

adonovan opened this issue Nov 7, 2024 · 12 comments

Comments

@adonovan
Copy link
Member

adonovan commented Nov 7, 2024

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

@adonovan adonovan moved this to Incoming in Proposals Nov 7, 2024
@gopherbot gopherbot added this to the Proposal milestone Nov 7, 2024
@timothy-king
Copy link
Contributor

  1. The spec does not mention "global". We may want to write this as "PackageVar" instead.
  2. 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?

@adonovan
Copy link
Member Author

adonovan commented Nov 8, 2024

  1. I agree, PackageVar is better. I have updated the proposal.
  2. 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.

@mvdan
Copy link
Member

mvdan commented Nov 8, 2024

Ooh, I've wanted this before, and it would make some code of mine simpler.

@timothy-king
Copy link
Contributor

@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.)

@adonovan
Copy link
Member Author

adonovan commented Nov 8, 2024

@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.)

@griesemer
Copy link
Contributor

What about RecvVar?

@adonovan
Copy link
Member Author

What about RecvVar?

Good point. We should add that too.

@findleyr
Copy link
Member

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

@adonovan
Copy link
Member Author

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.

@rsc rsc moved this from Incoming to Active in Proposals Dec 4, 2024
@rsc
Copy link
Contributor

rsc commented Dec 4, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@aclements
Copy link
Member

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.

@aclements
Copy link
Member

Based on the discussion above, this proposal seems like a likely accept.
— 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

@aclements aclements moved this from Active to Likely Accept in Proposals Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Likely Accept
Development

No branches or pull requests

8 participants