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.

Sign up for GitHub

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

Jump to bottom

proposal: reflect: add AssertTo #62121

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

proposal: reflect: add AssertTo #62121

dsnet opened this issue Aug 17, 2023 · 8 comments
Labels
Performance Proposal
Milestone
Proposal

Comments

@dsnet
Copy link
Member

dsnet commented Aug 17, 2023 •
edited

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 •
edited

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
@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
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 •
edited

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Performance Proposal
Projects
Proposals
Status: Active
Milestone
Proposal
Development

No branches or pull requests
6 participants
@neild @rsc @mvdan @bcmills @dsnet @gopherbot