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: reflect: add Type.IsEmptyInterface #41896

Closed
cuonglm opened this issue Oct 10, 2020 · 9 comments
Closed

proposal: reflect: add Type.IsEmptyInterface #41896

cuonglm opened this issue Oct 10, 2020 · 9 comments

Comments

@cuonglm
Copy link
Member

cuonglm commented Oct 10, 2020

Coming from #41882, @ianlancetaylor suggests adding:

func IsEmptyInterface(t reflect.Type) bool

to reflect package to detect empty interface.

Currently, users (including standard libraries, Google internal code, are relying on reflect.Type.NumMethod() == 0 to check empty interface, which is wrong. After #22075 is fixed, code which uses wrong behavior should be changed to:

reflect.TypeOf((*interface{})(nil)).Elem().Implements(T)

Adding reflect.IsEmptyInterface will help user's migration easier.

@gopherbot gopherbot added this to the Proposal milestone Oct 10, 2020
@cuonglm cuonglm changed the title Proposal: Add reflect.IsEmptyInterface proposal: Add reflect.IsEmptyInterface Oct 10, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: Add reflect.IsEmptyInterface proposal: add reflect.IsEmptyInterface Oct 10, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Oct 10, 2020
@ianlancetaylor
Copy link
Contributor

See discussion in #41882.

@mdempsky
Copy link
Member

A few thoughts:

  1. Adding a new function to package reflect makes sense for long-term stable state, but users during transition who want to support both Go 1.15 and Go 1.16 will need a solution outside of the standard library. The solution is simple enough to just copy/paste into their own code if necessary. But it might be nice to save them this hassle.

  2. Do we declare it as a standalone function or as a method? (I don't immediately see any significant technical arguments either way, except that it can only be a method if declared in package reflect.)

  3. I recommend IsEmptyInterface(T) return false if T is not an interface type. The function has to check this anyway, and it seemed like a handful of users were checking t.Kind() == reflect.Interface && t.NumMethod() == 0 together. So it's slightly nicer to them (and marginally more efficient) if they can just write IsEmptyInterface(t).

@gopherbot
Copy link

Change https://golang.org/cl/263078 mentions this issue: internal/reflectutil: add helper function to detect empty interface

@ianlancetaylor
Copy link
Contributor

See also #42099.

@rsc rsc changed the title proposal: add reflect.IsEmptyInterface proposal: reflect: add Type.IsEmptyInterface Oct 21, 2020
@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

We are probably going to roll back that change - see #42123 - so it looks like we don't need this.

@cuonglm
Copy link
Member Author

cuonglm commented Oct 21, 2020

We are probably going to roll back that change - see #42123 - so it looks like we don't need this.

I still think we should add this method, to provide a better/correct way for user to detect empty interface.

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 21, 2020
@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

It's certainly easy to go overboard on helpers.
We've come a long way without this one, and NumMethod() == 0 is both clear and correct.

@rsc
Copy link
Contributor

rsc commented Oct 28, 2020

Since we rolled back that change, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Oct 28, 2020
@rsc
Copy link
Contributor

rsc commented Nov 4, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Nov 4, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Nov 4, 2020
@golang golang locked and limited conversation to collaborators Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants