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 · 22 comments · May be fixed by #71639
Open

reflect: add TypeAssert #62121

dsnet opened this issue Aug 17, 2023 · 22 comments · May be fixed by #71639

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?

@mateusz834
Copy link
Member

mateusz834 commented Feb 10, 2025

@dsnet I have only just realized that you self-assigned this issue, but as i already have a working implementation, decided to send a CL for it. CL 648056

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/648056 mentions this issue: reflect: add TypeAssert[T]

@mateusz834
Copy link
Member

While looking at the diff i realized that, TypeAssert is documented as semantically equivalent to: v2, ok := v.Interface().(T)

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

Do we really want that? Consider:

func TestTypeAssert2(t *testing.T) {
	val := ValueOf(&net.DNSError{})
	_, ok := TypeAssert[error](val)
	t.Log(ok) // false
	_, ok = val.Interface().(error)
	t.Log(ok) // true
}

The implementation in CL 648056 returns false, true, but based on the docs it should return 2xtrue.

@ianlancetaylor
Copy link
Member

The goal is for the reflect package to more or less duplicate the language. Since the language permits type assertions to interface types, I would expect reflect.TypeAssert to do the same. I think we would need a special argument for any other behavior, and I don't see that argument in this proposal.

Is there a good reason that we shouldn't implement the language behavior?

@dsnet
Copy link
Member Author

dsnet commented Feb 10, 2025

@mateusz834, no worries. I had a local CL for this that I was gonna send out after the freeze. I don't mind going with your CL so long as it's fast for concrete types and I get to use it in Go 1.25 😄.

TypeAssert is documented as semantically equivalent to: v2, ok := v.Interface().(T) ... Do we really want that?

When I implemented this myself, I noticed this difference as well. Implementation-wise, it seems easier to violate the documented behavior, but on the other hand, it's very easy to explain TypeAssert being equivalent to v2, ok := v.Interface().(T).

My approach (CL/648235) only special cases concrete types and delegates the handling of interfaces to v.Interface().(T).

@gopherbot
Copy link
Contributor

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

@mateusz834
Copy link
Member

mateusz834 commented Feb 11, 2025

There is also this kind of behavior in the Interface() logic:

func newPtr[T any](t T) *T {
	return &t
}

func TestTypeAssert2(t *testing.T) {
	val := reflect.ValueOf(newPtr(any(1))).Elem()
	_, ok := val.Interface().(int)
	t.Log(ok) // true

	val = reflect.ValueOf(newPtr(error(&net.DNSError{}))).Elem()
	_, ok = val.Interface().(*net.DNSError)
	t.Log(ok) // true
}

go/src/reflect/value.go

Lines 1498 to 1508 in 7c1a413

if v.kind() == Interface {
// Special case: return the element inside the interface.
// Empty interface has one layout, all interfaces with
// methods have a second layout.
if v.NumMethod() == 0 {
return *(*any)(v.ptr)
}
return *(*interface {
M()
})(v.ptr)
}

I don't know whether TypeAssert should follow it, it had to be this way for Interface(), because of how interfaces work, but for TypeAssert we don't have to do this kind of logic.

EDIT: It makes sense in terms of Interface().(T), but i wonder whether this is the expected behavior of a generic function. It might be tempting to use TypeAssert as a helper to check whether Value has the same type as T and to extract the value of it. If we make it the same as Interface().(T), then it still requires the use of reflect.TypeFor[T]() == val to check whether this is the type we expect, but because it is generic it might be easy to miss that.

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

Successfully merging a pull request may close this issue.

10 participants