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 TypeFor #60088

Closed
josharian opened this issue May 9, 2023 · 43 comments
Closed

reflect: add TypeFor #60088

josharian opened this issue May 9, 2023 · 43 comments

Comments

@josharian
Copy link
Contributor

josharian commented May 9, 2023

reflect.TypeOf is one of a few fundamental reflect APIs. It returns the dynamic type of a value. This is, strictly speaking, sufficient. However, it is non-obvious how to use it to get a static type. The correct code is long, awkward, and hard to explain.

Pre-generics, there was no way to write a nice API to get a static reflect.Type. Now there is.

// MakeType returns the reflection Type that represents the static type of T.
func MakeType[T any]() reflect.Type {
	return reflect.TypeOf((*T)(nil)).Elem()
}

I propose that we add it.

It is a single line implementation, but it fills an obvious gap in the package reflect API and makes call sites clear.

I strongly suspect that almost all uses would be in conjunction with reflect.Type.Implements. An alternative would be make a generic version of implements that accepts an interface type as a type parameter, but fundamental building blocks compose better.

EDIT: this originally proposed StaticTypeOf; renamed to MakeType, per discussion.

@gopherbot gopherbot added this to the Proposal milestone May 9, 2023
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals May 9, 2023
@earthboundkid
Copy link
Contributor

Compare #50741

@rsc
Copy link
Contributor

rsc commented May 11, 2023

Gob uses tricks like

binaryMarshalerInterfaceType   = reflect.TypeOf((*encoding.BinaryMarshaler)(nil)).Elem()

and this would let us write

binaryMarshalerInterfaceType   = reflect.StaticTypeOf[encoding.BinaryMarshaler]()

which is definitely nicer. Static is probably not the right word - that's a C word with lots of overloaded meanings. But something with that behavior would help.

@DeedleFake
Copy link

I've wanted something like this on more than one occasion as well. There have been several talks about adding a nicer generic way to get a zero value of a type, something similar to nil but for any value. If one of those actually goes somewhere, let's say using _ as a value, then combining that with #50741 would let you do something like reflect.GenericTypeOf[int](_).

@earthboundkid
Copy link
Contributor

earthboundkid commented May 11, 2023

Can someone do a corpus analysis to see what the relative frequencies of different reflect operations are (GenericTypeOf vs StaticTypeOf equivalents)?

@mvdan
Copy link
Member

mvdan commented May 11, 2023

To bikeshed on shorter names: TypeOfT, TypeFrom.

@rogpeppe
Copy link
Contributor

or even reflect.T ?

@earthboundkid
Copy link
Contributor

earthboundkid commented May 11, 2023

A cursory look at just the standard library reveals dozens of places where reflect.T[whatever]() could be substituted in for greater clarity. reflect.GenericValueOf is harder to qualify because there are a few places where the interface type was clearly what was wanted, but in a lot of cases, the underlying concrete type is the target. Adding reflect.GenericValueOf will add a level of ambiguity when reading code: Does the author really want the interface type here or did they just see generics and assume this function is newer and therefore better? It adds a second way of doing things without being 100% clear about what's intended. So I'm leaning towards +1 on reflect.T but +0 on reflect.GenericValueOf.

@ianlancetaylor
Copy link
Contributor

reflect.TypeOf is a good name. I hope I'm not missing anything, but it seems to me that the new proposed function will always return the same value as reflect.TypeOf except when passing an argument of interface type. So since we already have reflect.TypeOf, and it works fine, what we need is a function that returns that reflect.Type for an interface type. I suggest reflect.TypeOfInterface.

@mvdan
Copy link
Member

mvdan commented May 11, 2023

@ianlancetaylor I'm not sure I agree; I would personally want to use the generic API consistently. There would be little reason to use a mix of both APIs if the new one is more than enough. Consistency is the same reason why I use reflect.TypeOf((*string)(nil)).Elem() instead of reflect.TypeOf(""), for example.

Put another way, I much prefer reflect.NewAPI[string] (whatever its name) over either of the existing options above.

@ianlancetaylor
Copy link
Contributor

Thanks, that kind of makes this an instance of #48287. It's hard to think of a good name, since the right name is already taken.

@apparentlymart
Copy link

apparentlymart commented May 12, 2023

I feel a little confused by the discussion above because the proposal I see at the time of writing does not take a value and return its type, but instead takes a type directly (via a type parameter) and returns the reflect equivalent of that type. Has the proposal changed since the discussion started?

I suppose the assumption of the subsequent discussion is that it's idiomatic to use the zero value of a type with TypeOf to obtain the reflect value of that type, in which case indeed it would be true that the zero value of any type other than an interface type would produce the same result as passing the same type as the type parameter to the proposed function.

However, I think even then this proposal is defensible, because directly asking for the reflect representation of a type is a clearer representation of author intent than asking for the type of the zero value of a type:

reflect.TypeOf(0)
reflect.StaticTypeOf[int]()

Of course this is subjective, but the second one reads more clearly to me as "get the reflect representation of int" than the first one does. The first one just reads as a workaround for the lack of generics in Go.

In some codebases I'm responsible for I can see expressions very similar to the ones found in the standard library which would benefit from this proposal, like this one:

// (intAlias is a named type based on int)
reflect.TypeOf((*intAlias)(nil)).Elem()

// (I don't recall why this one uses a pointer to a separate zero-valued variable rather
// than a nil pointer, but perhaps this just demonstrates that it's non-obvious how
// to get a reflect.Type of an interface type, as the proposal claims.)
var victimExpr hcl.Expression // hcl.Expression is an interface type
var exprType = reflect.TypeOf(&victimExpr).Elem()

I can also see some examples like the one I showed above where the zero value to TypeOf doesn't directly name the type, so the reader just has to know what type the expression has:

// this is from a test of an encoding/json-like marshalling library, testing the string case
reflect.TypeOf("")

// (cty.NilType is a variable containing the zero value of a struct type,
// so this is the same as writing an empty composite literal for that type.)
reflect.TypeOf(cty.NilType)

The amount of variation in these examples suggests to me that having one obvious way to do it would also reduce the deviation caused by different developers inventing slightly different approaches to the same problem.


I do agree that the name StaticTypeOf feels a bit unfortunate. My first idea would've been reflect.GetType[T] since it seems like we're literally getting the type itself, and not getting the type of something. Though of course I'll concede that it isn't typical to use the Get prefix on getter-shaped functions in Go, so that would itself be a little unfamiliar.

I feel okay about reflect.T too, although I feel unsure about whether I would've guessed correctly what that represents if I hadn't already been primed by reading this discussion. On the other hand, obtaining reflect.Type values is one of the two main operations in the reflect API -- obtaining reflect.Value values being the other -- so it does seem defensible to give it a short, concise name. I feel ambivalent about whether T is too short or not. 😀

I don't see so much value in a generic version of reflect.ValueOf, because indeed that seems only marginally different to the interface-based version, and the difference between the two would be pretty subtle for less experienced gophers as described above.

@ianlancetaylor
Copy link
Contributor

It's a little odd, but perhaps reflect.MakeType[userType]() to make a reflect.Type from some other type.

@rogpeppe
Copy link
Contributor

I think I'd be happy with reflect.MakeType. Every intantiation of reflect.MakeType is a func() reflect.Type that "makes" some specific type from nothing. That seems reasonable to me.

@josharian josharian changed the title proposal: reflect: add StaticTypeOf proposal: reflect: add MakeType May 12, 2023
@rsc rsc moved this from Incoming to Active in Proposals May 17, 2023
@rsc
Copy link
Contributor

rsc commented May 17, 2023

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

@bradfitz
Copy link
Contributor

I love this proposal, but the "make" in MakeType makes it sound way more expensive than it is. It will likely compile to approximately nothing. It's certainly not as expensive as the existing MakeMap, MakeMapWithSize, MakeSlice, New, NewAt.

I know nobody loves the word "Get" but @apparentlymart's reflect.GetType[T] sounds closer to the weightiness I'd expect.

@ianlancetaylor
Copy link
Contributor

Yet another color is reflect.ToType[userType]() returning a reflect.Type.

@zephyrtronium
Copy link
Contributor

If we're listing all possible names, #60274 suggests the name Reflect[T ...]() for a related purpose. reflect.Reflect[userType]() stutters, but I don't find it unnatural.

@bcmills
Copy link
Contributor

bcmills commented May 19, 2023

Maybe reflect.AsType[userType](), meaning “userType as a reflect.Type”?

@earthboundkid
Copy link
Contributor

earthboundkid commented May 19, 2023

  • TypeOf: would be best, but it's taken.
  • GetType: Considered un-Go-ish
  • MakeType: Feels more heavyweight than the operation is. MakeFunc makes a new function (similarly MakeMap, MakeChan etc.), but this returns a reflect.Type of an existing type.
  • AsType: "As" usually means some kind of dynamic conversion or unwrapping
  • ToType: Usually a To function starts with something and converts it into something else.
  • NewType: Has similar problems to Make, but I think slightly less so.

@jimmyfrasche
Copy link
Member

reflect.TheType[T]()?

@gazerro
Copy link
Contributor

gazerro commented May 19, 2023

Maybe reflect.Types[userType]().

@randall77
Copy link
Contributor

reflect.ConstantType[string]()?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/514639 mentions this issue: encoding/xml: use reflect.TypeFor for known types

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/514640 mentions this issue: text/template, html/template: use reflect.TypeFor for known types

gopherbot pushed a commit that referenced this issue Aug 1, 2023
This avoids several mildly confusing Elem calls.

For #60088

Change-Id: If7b83d2ab10537c7e886a035b43cb272130c1669
Reviewed-on: https://go-review.googlesource.com/c/go/+/514455
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Rob Pike <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
gopherbot pushed a commit that referenced this issue Aug 1, 2023
For #60088

Change-Id: Ib2589b994d304cca1f2e2081639959d80818ac7f
Reviewed-on: https://go-review.googlesource.com/c/go/+/514639
Run-TryBot: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: David Chase <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Joseph Tsai <[email protected]>
gopherbot pushed a commit that referenced this issue Aug 1, 2023
For #60088

Change-Id: I2e471c76de62944b14472966b63f5778124b9b8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/514655
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Run-TryBot: Joseph Tsai <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Joseph Tsai <[email protected]>
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
gopherbot pushed a commit that referenced this issue Aug 1, 2023
For #60088

Change-Id: Ibc3983ca5cfe396087ddfa96c43cfe32ca47129a
Reviewed-on: https://go-review.googlesource.com/c/go/+/514640
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Rob Pike <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/514975 mentions this issue: database/sql: use reflect.TypeFor for known types

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/514995 mentions this issue: net/rpc: use reflect.TypeFor for known types

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/515175 mentions this issue: cmd/fix: use reflect.TypeFor for known types

gopherbot pushed a commit that referenced this issue Aug 3, 2023
For #60088

Change-Id: I56586b68d5e38a46560f4ced19214f1d2db2850e
Reviewed-on: https://go-review.googlesource.com/c/go/+/514995
Run-TryBot: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Rob Pike <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
gopherbot pushed a commit that referenced this issue Aug 7, 2023
For #60088

Change-Id: Id19435e864bcfd2adbb1492db3f8cdf2ee3c915e
Reviewed-on: https://go-review.googlesource.com/c/go/+/515175
Auto-Submit: Ian Lance Taylor <[email protected]>
Run-TryBot: shuang cui <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
gopherbot pushed a commit that referenced this issue Aug 8, 2023
For #60088

Change-Id: Ib05ba3d456b22f7370459037b3d263c4b3ebe3b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/514975
Reviewed-by: Daniel Theophanes <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/522338 mentions this issue: net/http: use reflect.TypeFor for known types

gopherbot pushed a commit that referenced this issue Aug 24, 2023
For #60088

Change-Id: I9e4044d9c2694fe86aab1f5220622c8d952b1a90
Reviewed-on: https://go-review.googlesource.com/c/go/+/522338
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
apocelipes added a commit to apocelipes/go that referenced this issue Aug 30, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/524259 mentions this issue: encoding/asn1: use reflect.TypeFor for known types

gopherbot pushed a commit that referenced this issue Aug 30, 2023
For #60088

Change-Id: I4b2a5c6c59ef26361f343052a4ddaabde5d3bc94
GitHub-Last-Rev: d519835
GitHub-Pull-Request: #62370
Reviewed-on: https://go-review.googlesource.com/c/go/+/524259
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555597 mentions this issue: reflect: optimize TypeFor for non-interface types

gopherbot pushed a commit that referenced this issue Feb 12, 2024
The reflect.Type.Elem method is somewhat slow,
which is unfortunate since the reflect.TypeOf((*T)(nil)).Elem()
trick is only needed if T is an interface.

Optimize for concrete types by doing the faster reflect.TypeOf(v)
call first and only falling back on the Elem method if needed.

Performance:

	name              old time/op  new time/op  delta
	TypeForString-24  9.10ns ± 1%  1.78ns ± 2%  -80.49%  (p=0.000 n=10+10)
	TypeForError-24   9.55ns ± 1%  9.78ns ± 1%   +2.39%  (p=0.000 n=10+9)

Updates #60088

Change-Id: I2ae76988c9a3dbcbae10d2c19b55db3c8d4559bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/555597
Auto-Submit: Joseph Tsai <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
Run-TryBot: Joseph Tsai <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Mauri de Souza Meneguzzo <[email protected]>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
The reflect.Type.Elem method is somewhat slow,
which is unfortunate since the reflect.TypeOf((*T)(nil)).Elem()
trick is only needed if T is an interface.

Optimize for concrete types by doing the faster reflect.TypeOf(v)
call first and only falling back on the Elem method if needed.

Performance:

	name              old time/op  new time/op  delta
	TypeForString-24  9.10ns ± 1%  1.78ns ± 2%  -80.49%  (p=0.000 n=10+10)
	TypeForError-24   9.55ns ± 1%  9.78ns ± 1%   +2.39%  (p=0.000 n=10+9)

Updates golang#60088

Change-Id: I2ae76988c9a3dbcbae10d2c19b55db3c8d4559bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/555597
Auto-Submit: Joseph Tsai <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
Run-TryBot: Joseph Tsai <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Mauri de Souza Meneguzzo <[email protected]>
@dmitshur dmitshur modified the milestones: Backlog, Go1.22 May 23, 2024
@AlphaLxy
Copy link

Change https://go.dev/cl/555597 mentions this issue: reflect: optimize TypeFor for non-interface types

hi @dsnet, is this optimization necessary?

goos: darwin
goarch: arm64
pkg: code.byted.org/ecom/main
BenchmarkTypeForString
BenchmarkTypeForString-10     	568044871	         2.112 ns/op
BenchmarkTypeForError
BenchmarkTypeForError-10      	287675214	         4.177 ns/op
BenchmarkTypeForString2
BenchmarkTypeForString2-10    	914612360	         1.309 ns/op
BenchmarkTypeForError2
BenchmarkTypeForError2-10     	918391360	         1.299 ns/op
type Type struct{ /** copy of abi.Type **/ }
type PtrType struct { Type; Elem *Type }

var abiTypeITab uintptr

func init() {
	t := reflect.TypeOf(0)
	abiTypeITab = (*[2]uintptr)(unsafe.Pointer(&t))[0]
}

func TypeFor2[T any]() (t reflect.Type) {
	v := any((*T)(nil))
	vp := (*[2]uintptr)(unsafe.Pointer(&v))
	tp := (*[2]uintptr)(unsafe.Pointer(&t))
	tp[0] = abiTypeITab
	tp[1] = *(*uintptr)(unsafe.Pointer(vp[0] + unsafe.Offsetof(PtrType{}.Elem)))
	return
}
var sinkType reflect.Type

func BenchmarkTypeForString(b *testing.B) {
	for i := 0; i < b.N; i++ {
		sinkType = TypeFor[string]()
	}
}

func BenchmarkTypeForError(b *testing.B) {
	for i := 0; i < b.N; i++ {
		sinkType = TypeFor[error]()
	}
}

func BenchmarkTypeForString2(b *testing.B) {
	for i := 0; i < b.N; i++ {
		sinkType = TypeFor2[string]()
	}
}

func BenchmarkTypeForError2(b *testing.B) {
	for i := 0; i < b.N; i++ {
		sinkType = TypeFor2[error]()
	}
}

@AlphaLxy
Copy link

This function can be inlined.

func TypeFor2[T any]() (t reflect.Type) {
	v := any((*T)(nil))
	vp := (*[2]uintptr)(unsafe.Pointer(&v))
	tp := (*[2]uintptr)(unsafe.Pointer(&t))
	tp[0] = abiTypeITab
	tp[1] = *(*uintptr)(unsafe.Pointer(vp[0] + unsafe.Offsetof(PtrType{}.Elem)))
	return
}

@ianlancetaylor
Copy link
Contributor

Rather than do this kind of inscrutable unsafe data structure manipulation, let's improve the compiler. Thanks.

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

Successfully merging a pull request may close this issue.