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

reflect: add StructField.IsExported and Method.IsExported #41563

Closed
dsnet opened this issue Sep 22, 2020 · 9 comments
Closed

reflect: add StructField.IsExported and Method.IsExported #41563

dsnet opened this issue Sep 22, 2020 · 9 comments

Comments

@dsnet
Copy link
Member

dsnet commented Sep 22, 2020

The StructField.PkgPath field is populated if and only if the field is unexported. However, there is a hit to readability:

for i := 0; i < t.NumField(); i++ {
    f := t.Field(i)
    if f.PkgPath != "" {
        continue
    }
    ...
}

It's not obvious to most readers why the f.PkgPath != "" check exist. For this reason, a vast majority of cases have a comment that says:

    // Skip unexported fields.
    if f.PkgPath != "" {
        continue
    }

This documentation seems unnecessary if we had just added a helper method on StructField like:

// IsExported reports whether the struct field is exported.
func (f StructField) IsExported() bool {
    return f.PkgPath == ""
}

Thus, use of it becomes obvious from a readability perspective:

for i := 0; i < t.NumField(); i++ {
    f := t.Field(i)
    if !f.IsExported() {
        continue
    }
    ...
}

Bikeshedding: I don't have a preference whether this should be called IsExported or simply Exported. The former matches IsValid, IsNil, or IsZero, while the latter matches StructField.Anonymous. It seems that the reflect package is already internally inconsistent about naming of predicate functionality.

@dsnet dsnet changed the title reflect: add StructField.IsExported proposal: reflect: add StructField.IsExported Sep 22, 2020
@gopherbot gopherbot added this to the Proposal milestone Sep 22, 2020
@ianlancetaylor
Copy link
Contributor

Minor side note: the field name Anonymous is already wrong, as we now refer to that concept as an embedded field. So although we can't change Anonymous we shouldn't take it as a precedent for anything.

@rsc
Copy link
Contributor

rsc commented Sep 23, 2020

I'm OK with adding IsExported() bool.
I wouldn't want to add a new struct field that could be out of sync with PkgPath.

@rsc
Copy link
Contributor

rsc commented Oct 14, 2020

Based on the discussion above, this seems like a likely accept.

@dsnet
Copy link
Member Author

dsnet commented Oct 15, 2020

I should note that we probably want to add an IsExported method to the Method type as well since it has a PkgPath field with the same semantics.

@rsc
Copy link
Contributor

rsc commented Oct 17, 2020

SGTM

@rsc rsc changed the title proposal: reflect: add StructField.IsExported proposal: reflect: add StructField.IsExported and Method.IsExported Oct 17, 2020
@dsnet
Copy link
Member Author

dsnet commented Oct 20, 2020

Another note: #22075 makes it such that NumMethod no longer counts unexported methods, which subsequently means that you can't retrieve unexported methods. If this is the case, we should mark Method.PkgPath as deprecated instead of adding a Method.IsExported method.

\cc @mdempsky

@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

That change to NumMethod is likely to be rolled back.

@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

No change in consensus, so accepted.

@rsc rsc modified the milestones: Proposal, Backlog Oct 21, 2020
@rsc rsc changed the title proposal: reflect: add StructField.IsExported and Method.IsExported reflect: add StructField.IsExported and Method.IsExported Oct 21, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/266197 mentions this issue: reflect: add Method.IsExported and StructField.IsExported methods

@rsc rsc moved this to Accepted in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc removed this from Proposals Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants