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: Overflow methods should be on Type #60427

Closed
mvdan opened this issue May 25, 2023 · 19 comments
Closed

reflect: Overflow methods should be on Type #60427

mvdan opened this issue May 25, 2023 · 19 comments
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. Proposal Proposal-Accepted
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented May 25, 2023

See these four methods on reflect.Value:

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.

@mvdan mvdan added the Proposal label May 25, 2023
@gopherbot gopherbot added this to the Proposal milestone May 25, 2023
@gazerro
Copy link
Contributor

gazerro commented May 25, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

gazerro commented May 26, 2023

@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
Copy link
Contributor

Merovius commented May 26, 2023

@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
Copy link
Contributor

gazerro commented May 26, 2023

@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
Copy link
Contributor

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
Copy link
Member Author

mvdan commented May 26, 2023

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
Copy link
Contributor

gazerro commented May 26, 2023

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
Copy link
Member Author

mvdan commented Aug 17, 2023

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
Copy link
Contributor

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

@rsc
Copy link
Contributor

rsc commented Oct 11, 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

@rsc
Copy link
Contributor

rsc commented Oct 24, 2023

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
Copy link
Contributor

rsc commented Oct 26, 2023

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
Copy link
Contributor

rsc commented Nov 2, 2023

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

@rsc rsc changed the title proposal: reflect: Overflow methods should be on Type reflect: Overflow methods should be on Type Nov 2, 2023
@rsc rsc modified the milestones: Proposal, Backlog Nov 2, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Nov 4, 2023

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.

callthingsoff added a commit to callthingsoff/go that referenced this issue Feb 27, 2024
This CL adds new methods synonymous with the method of the same name
in reflect.Value to reflect.Type: OverflowComplex, OverflowFloat,
OverflowInt, OverflowUint.

Fixes golang#60427
@gopherbot
Copy link

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

@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Feb 27, 2024
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Feb 27, 2024
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Feb 29, 2024
CL 567296 added {OverflowComplex, OverflowFloat, OverflowInt, OverflowUint}
to reflect.Type, this CL uses these methods to simplify code.

For #60427

Change-Id: I229aef9e4095a2f025afd782081f6c9e6d7710f3
GitHub-Last-Rev: c824e5a
GitHub-Pull-Request: #66000
Reviewed-on: https://go-review.googlesource.com/c/go/+/567775
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
gopherbot pushed a commit that referenced this issue Mar 4, 2024
For consistency, this CL cleans up unnecessary comments,
and moves these Overflow methods to exported area.

For #60427

Change-Id: I14d4ffbc3552d31c211ea1e0b7a0f7090a4a8b89
GitHub-Last-Rev: acdc6ad
GitHub-Pull-Request: #66019
Reviewed-on: https://go-review.googlesource.com/c/go/+/567917
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. Proposal Proposal-Accepted
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

7 participants