reflect: Overflow methods should be on Type

[mvdan]

See these four methods on reflect.Value:

• https://pkg.go.dev/reflect#Value.OverflowComplex
• https://pkg.go.dev/reflect#Value.OverflowFloat
• https://pkg.go.dev/reflect#Value.OverflowInt
• https://pkg.go.dev/reflect#Value.OverflowUint

For example, looking at OverflowInt's godoc:

OverflowInt reports whether the int64 x cannot be represented by v's type. It panics if v's Kind is not Int, Int8, Int16, Int32, or Int64.

None of that needs a value, in my opinion - a type would suffice. The implementation seems to confirm that - only the "kind" and "type" are used, not the actual value.

So I think the methods should be added to Type, with the same signature.

I noticed this because quite a few projects have code like reflect.Zero(typ).OverflowInt(x), when in fact it could just be typ.OverflowInt(x), saving the creation of a new value. For example, in encoding/json: https://cs.opensource.google/go/go/+/refs/tags/go1.20.4:src/encoding/json/decode.go;l=799

As for users who have a reflect.Value rather than a reflect.Type, they could also replace val.OverflowInt(x) with val.Type().OverflowInt(x), and we could consider deprecating the Value methods in favor of the Type methods after a few Go releases. I don't personally see a point in keeping eight methods around; .Type() isn't enough characters to warrant a shortcut, and in my experience, most reflect users will need to get the reflect.Type for other reasons.

[gazerro]

It may also be implemented as a single generic function:

// Overflow reports whether x cannot be represented by type t.
func Overflow[T constraints.Integer | constraints.Float | constraints.Complex](t Type, x T) bool

and used in this way:

reflect.Overflow(t, x)

[Merovius]

@gazerro I'd prefer reflect to stay relatively consistent. We don't use top-level functions for this kind of stuff so far. And using generics wouldn't add type-safety here and I'd rather four methods on Type than one package-scoped function, in terms of namespace pollution. YMMV.

[Merovius]

Oh, also, I don't think that API is actually sufficiently flexible. It wouldn't allow you to check if a reflect.Value would overflow a given reflect.Type. Arguably, the method could be Type.Overflow(reflect.Value) bool, though.

[gazerro]

@Merovius

Arguably, the method could be Type.Overflow(reflect.Value) bool, though.

It can simply be written as Value.Overflow() bool.

But that doesn't solve the problem mentioned in this proposal, which is about checking for overflow using a reflect.Type and a value directly, instead of a reflect.Value.

[Merovius]

@gazerro To be clear: I was suggesting making that a method on Type. Maybe I should have written func (Type) Overflow(reflect.Value) bool.

I think there are a couple of bikeshed colors (say v is a reflect.Value, x is an intN, floatN… and t is a reflect.Type):

1. func (t Type) OverflowInt(x int64) bool; func (t Type) OverflowFloat(x float64) bool; …. Used as t.OverflowInt(x) or t.OverflowInt(v.Int()). This is the proposal.
2. func (t Type) Overflow(v Value) bool. Used as t.Overflow(reflect.ValueOf(x)) or t.Overflow(v).
3. func (v Value) Overflow(t Type) bool. Used as reflect.ValueOf(x).Overflow(t) or v.Overflow(t).
4. func Overflow[T constraints.Integer | constraints.Float | constraints.Complex | Value](t Type, v T). Used as reflect.Overflow(t, x) or reflect.Overflow(t, v).

The last three all have the advantage of minimizing API surface - they only add one exported function/method. The last one has the advantage that it makes the call look uniform and requires the least boilerplate no matter if you have a reflect.Value already or not. It has the disadvantage, that it's different from the existing reflect APIs.

Number 2 could be combined with 1 as well, to give t.OverflowInt(x) or t.Overflow(v).

Also, while we bikeshed, I think Overflow is the wrong name. To overflow something is to make it flow over, not to check if something overflows. I think the name should maybe be Overflows, suggesting the third option. i.e. v.Overflows(t) reads nicely. The methods on Type should arguably be IsOverflowedBy or Fits or something. So t.Fits(v)/t.FitsInt(x)/…. Naming the function nicely is hard, maybe CheckOverflow.

The JSON example from the proposal text notably packs the int64 (etc.) into a reflect.Value anyways. Though there are probably examples where that's not necessary (e.g. when setting a struct field, you can use Value.SetInt).

[gazerro]

@Merovius

I think the name should maybe be Overflows, suggesting the third option. i.e. v.Overflows(t) reads nicely.

It looks great to me. v.Overflows(t) looks nice and appears to handle all cases.

The code in encoding/json from reflect.Zero(kt).OverflowInt(n) becomes reflect.ValueOf(n).Overflows(kt). It looks better and no longer allocates.

[Merovius]

To add one more color of bikeshed: func (t Type) Overflow(any) bool, as the method-equivalent of the generic function. It looks less type-safe, but the generic function isn't actually very safe anyways (the type checked for overflow must match the type argument in kind). Possibly allocates for int64 etc., though.

[mvdan]

We need to be careful not to add any allocations or other cost. This call would need to happen every single time the JSON decoder encounters a number, for example. cc @dsnet

[gazerro]

There are no other methods of Type that accept or return a Value value, so func (t Type) Overflow(any) bool would be an exception. Perhaps this is the reason why the methods were added to Value instead of Type.

[mvdan]

This seems like a fairly uncontroversial proposal - @ianlancetaylor do you think we could consider it for Go 1.22? I am more than happy to send the CL.

[ianlancetaylor]

I'll add it to the list to review soonish.

[rsc]

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]

This seems straightforward. I believe the new API is:

// OverflowComplex reports whether the complex128 x cannot be represented by type t. 
// It panics if t's Kind is not Complex64 or Complex128.
func (t Type) OverflowComplex(x complex128) bool

// OverflowFloat reports whether the float64 x cannot be represented by type t. 
// It panics if t's Kind is not Float32 or Float64.
func (t Type) OverflowFloat(x float64) bool

// OverflowInt reports whether the int64 x cannot be represented by type t. 
// It panics if t's Kind is not Int, Int8, Int16, Int32, or Int64.
func (t Type) OverflowInt(x int64) bool

// OverflowUint reports whether the uint64 x cannot be represented by type t. 
// It panics if t's Kind is not Uint, Uintptr, Uint8, Uint16, Uint32, or Uint64.
func (t Value) OverflowUint(x uint64) bool

Do I have that right?

[rsc]

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

Adding to package reflect:

// OverflowComplex reports whether the complex128 x cannot be represented by type t. 
// It panics if t's Kind is not Complex64 or Complex128.
func (t Type) OverflowComplex(x complex128) bool

// OverflowFloat reports whether the float64 x cannot be represented by type t. 
// It panics if t's Kind is not Float32 or Float64.
func (t Type) OverflowFloat(x float64) bool

// OverflowInt reports whether the int64 x cannot be represented by type t. 
// It panics if t's Kind is not Int, Int8, Int16, Int32, or Int64.
func (t Type) OverflowInt(x int64) bool

// OverflowUint reports whether the uint64 x cannot be represented by type t. 
// It panics if t's Kind is not Uint, Uintptr, Uint8, Uint16, Uint32, or Uint64.
func (t Value) OverflowUint(x uint64) bool

[rsc]

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

Adding to package reflect:

// OverflowComplex reports whether the complex128 x cannot be represented by type t. 
// It panics if t's Kind is not Complex64 or Complex128.
func (t Type) OverflowComplex(x complex128) bool

// OverflowFloat reports whether the float64 x cannot be represented by type t. 
// It panics if t's Kind is not Float32 or Float64.
func (t Type) OverflowFloat(x float64) bool

// OverflowInt reports whether the int64 x cannot be represented by type t. 
// It panics if t's Kind is not Int, Int8, Int16, Int32, or Int64.
func (t Type) OverflowInt(x int64) bool

// OverflowUint reports whether the uint64 x cannot be represented by type t. 
// It panics if t's Kind is not Uint, Uintptr, Uint8, Uint16, Uint32, or Uint64.
func (t Value) OverflowUint(x uint64) bool

[dmitshur]

It looks like there's a typo in the 4th method, fixable by applying the following diff:

-func (t Value) OverflowUint(x uint64) bool
+func (t Type) OverflowUint(x uint64) bool

The Value.Overflow(uint64) bool method already exists, so there's no risk of it being accidentally added a second time. Pointing it out for posterity.

[gopherbot]

Change https://go.dev/cl/567296 mentions this issue: reflect: add Overflow methods to Type

[gopherbot]

Change https://go.dev/cl/567775 mentions this issue: encoding/json: make use of reflect.Type.{OverflowInt, OverflowUint}

[gopherbot]

Change https://go.dev/cl/567917 mentions this issue: reflect: clean up unnecessary comments for rtype