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 TypeAssert #62121

Open
dsnet opened this issue Aug 17, 2023 · 15 comments
Open

reflect: add TypeAssert #62121

dsnet opened this issue Aug 17, 2023 · 15 comments

Comments

@dsnet
Copy link
Member

dsnet commented Aug 17, 2023

Consider the following benchmark:

func Benchmark(b *testing.B) {
	v := reflect.ValueOf(new(time.Time)).Elem()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		_ = v.Interface().(time.Time)
	}
}

This currently prints:

Benchmark-24    	37673833	        29.74 ns/op	      24 B/op	       1 allocs/op

Since the underlying time.Time is addressable, the Interface method must make a copy of it.

However, one primary reason to use the Interface method is to then assert it to a particular concrete type.

I propose the addition of:

// AssertTo is semantically equivalent to:
//    v2, ok := v.Interface().(T)
func AssertTo[T any](v reflect.Value) (T, bool)

which avoids the allocation since the value is never boxed in an interface.

Usage would then look like:

t, ok := reflect.AssertTo[time.Time](v)

and naturally matches type-assertion construct in the Go language itself.


Ideally, this would be a method on Value, but we don't have #49085.
If we did, the signature would be:

func (Value) AssertTo[T any]() (T, bool)

and usage would be more natural as:

t, ok := v.AssertTo[time.Time]()
@dsnet dsnet added the Proposal label Aug 17, 2023
@gopherbot gopherbot added this to the Proposal milestone Aug 17, 2023
@dsnet
Copy link
Member Author

dsnet commented Aug 17, 2023

#57060 is a compiler optimization that may obviate the need for this API as this falls under the "short-lived heap allocated value" problem.

@dsnet
Copy link
Member Author

dsnet commented Aug 17, 2023

Empirical evidence based on module proxy data on 2023-07-01:

  • ~560k calls to Interface() (not all may be reflect.Value.Interface method calls)
  • ~217k are immediately asserted to a single type (e.g., ... := v.Interface().(T))
  • ~8k are immediately used in a type-switch (e.g., switch ... := v.Interface().(type) { ... })

This means at least ~40% usages of the Interface method could directly benefit from this API.

@dsnet
Copy link
Member Author

dsnet commented Aug 17, 2023

As a performance workaround, you could do the following today:

func AssertTo[T any](rv reflect.Value) (v T, ok bool) {
    if v.CanAddr() {
        v, ok = *v.Addr().Interface().(*T)
    } else {
        v, ok = v.Interface().(T)
    }
    return v, ok
}

It's gross, but works.

@dsnet dsnet changed the title proposal: reflect: add AssertAs proposal: reflect: add AssertTo Aug 17, 2023
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Aug 18, 2023
@neild
Copy link
Contributor

neild commented Aug 18, 2023

Bikeshed: reflect.ValueAs seems clearer to me.

@dsnet
Copy link
Member Author

dsnet commented Aug 18, 2023

I like ValueAs more for readability, but also choose AssertTo to match the terminology of the language.

@mvdan
Copy link
Member

mvdan commented Apr 2, 2024

@ianlancetaylor could I gently nudge this proposal for consideration? It would very slightly help with the encoding/json/v2 work :)

@rsc rsc moved this from Incoming to Active in Proposals Apr 10, 2024
@rsc
Copy link
Contributor

rsc commented Apr 10, 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

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

Plain 'Assert' might be confusing with C-style asserts. Should we call it TypeAssert? With the Type prefix we may not need the To suffix either.

t, ok := reflect.TypeAssert[time.Time](v)

I think this reads better than TypeAssertTo.

@rsc rsc changed the title proposal: reflect: add AssertTo proposal: reflect: add TypeAssert May 8, 2024
@rsc
Copy link
Contributor

rsc commented May 8, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add to package reflect:

// TypeAssert is semantically equivalent to:
//    v2, ok := v.Interface().(T)
func TypeAssert[T any](v reflect.Value) (T, bool)

@rsc rsc moved this from Active to Likely Accept in Proposals May 15, 2024
@rsc
Copy link
Contributor

rsc commented May 15, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add to package reflect:

// TypeAssert is semantically equivalent to:
//    v2, ok := v.Interface().(T)
func TypeAssert[T any](v reflect.Value) (T, bool)

@zigo101
Copy link

zigo101 commented May 17, 2024

How about also adding

func TypeAssertInto[T any](v reflect.Value, t *T) bool

So that sometimes, we can use

var x string
...
if reflect.TypeAssertInto(v, &x) {...}

instead of

var x string
...
x, ok := reflect.TypeAssert[string](v)
if ok {...}

Both of the two functions have their own ideal use cases.

@rsc
Copy link
Contributor

rsc commented May 23, 2024

Let's start with just one.

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 23, 2024
@rsc
Copy link
Contributor

rsc commented May 23, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add to package reflect:

// TypeAssert is semantically equivalent to:
//    v2, ok := v.Interface().(T)
func TypeAssert[T any](v reflect.Value) (T, bool)

@rsc rsc changed the title proposal: reflect: add TypeAssert reflect: add TypeAssert May 23, 2024
@rsc rsc modified the milestones: Proposal, Backlog May 23, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/591495 mentions this issue: reflect: add TypeAssert

@earthboundkid
Copy link
Contributor

The CL for this got abandoned. @dsnet did you want to get it in before the freeze?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

8 participants