Fix the return type of Enumerable#sum/product for union elements
#15314
Conversation
Blacksmoke16
added
breaking-change
topic:stdlib:numeric
kind:bug
|
This PR expands on the (accidentally closed) PR 15308. Tagging @Blacksmoke16, @straight-shoota, and @BlobCodes to continue the discussion here. |
Using the first type of a union type as the type of the result of `Enumerable#sum/product()` call can cause runtime failures, e.g. `[1, 10000000000_u64].sum/product` will result in an ``OverflowError``. A safer alternative is to flag/disallow the use of union types with `Enumerable#sum/product()` and recommend the use of `Enumerable#sum/product(initial)` with an initial value of the expected type of the sum/product call.
src/enumerable.cr
Outdated
| raise("Enumerable#sum/product() does support Union types. " + | ||
| "Instead, use Enumerable#sum/product(initial) with an " + | ||
| "initial value of the expected type of the sum/product call.") |
There was a problem hiding this comment.
suggestion: Polish the error message:
| raise("Enumerable#sum/product() does support Union types. " + | |
| "Instead, use Enumerable#sum/product(initial) with an " + | |
| "initial value of the expected type of the sum/product call.") | |
| raise("`Enumerable#sum` and `#product` cannot determine the initial value from a union type. " + | |
| "Please pass an initial value of the intended type to the call.") |
There was a problem hiding this comment.
After reading your fix, I realized a missing negation in the first sentence of error message I committed :)
Also, how about we update the error message as follows?
raise("Enumerable#sum` and #product do not support union types. Instead, use Enumerable#sum/product(initial) with an initial value of the expected type of the sum/product call.")
Following is my reasoning for the above change.
- The existing logic fails to identify an appropriate/safe type that can hold the final value of the call.
- Using the identity is a way of determining the appropriate/safe type. Since it is an implementation detail, I think surface such details in documentation while mentioning what (not how) is wrong and its fix in the error message may be better.
There was a problem hiding this comment.
"do not support union types" isn't entirely accurate. They do support them. Just without automatically determining the return type, hence the need to specify explicitly.
There was a problem hiding this comment.
With this change, "sum()" and "product()" will not support union types (but "sum(initial)" and "product(initial)" will support union types). So, do you mean that we should be precise in the error message and the doc by referring to the no-arg variant of the methods?
There was a problem hiding this comment.
Yes, we could differentiate between the different overloads by signature.
Enumerable#sum()and#product()do not support union types.`
It's a bit of a subtle detail though. Hence my suggestion to write out the issue explicitly:
Enumerable#sumand#productcannot determine the initial value from a union type.
I'm fine either way though.
Using the first type of a union type as the type of the result of
Enumerable#sum/product()call can cause runtime failures, e.g.[1, 10000000000_u64].sum/productwill result in anOverflowError. A safer alternative is to flag/disallow the use of union types withEnumerable#sum/product()and recommend the use ofEnumerable#sum/product(initial)with an initial value of the expected type of the sum/product call.