Fix the return type of Enumerable#sum for union elements
#15308
Conversation
Using the first type of a union type as the type of the result of `Enumerable#sum()` call can cause runtime failures. A safer alternative is to flag the use of union types with `Enumerable#sum()` and suggest the use of `Enumerable#sum(initial)` with an initial value of the expected type of the `sum` call.
|
While checking this PR, I noticed that the The method has an "optimized path" for a sum of strings, but it wouldn't even compile without that optimization because it would otherwise try to call The docs should be updated to mention that the method won't work with specific types. Also, I don't think the best way of removing |
|
David, thanks for the input! I agree the doc should be updated. How about updating the document about
As for |
|
Let's discuss further improving the documentation of With regards to warning vs. error, I think we usually prefer the latter because warnings tend to be overlooked anyways. The justification for such a breaking change would that the current behaviour is quite unexpected and can lead to weird subtle bugs. E.g. the behaviour may change depending on the addition or removal of types from the union, even when the actual values are identical: ([12345678_u32] of UInt8 | UInt32).sum # => 12345678
([12345678_u32] of UInt16 | UInt32).sum # OverflowError: Arithmethc overflowSo I believe it could be acceptable to release this change in a minor release. It might even still go into 1.15, though we should take some time to check if there are any significant consequences in the ecosystem that we may not be aware of. |
straight-shoota
added
kind:bug
| raise("Enumerable#sum() does support Union types. Instead, " + | ||
| "use Enumerable#sum(initial) with an initial value of " + | ||
| "the expected type of the sum call.") |
There was a problem hiding this comment.
issue: Reflect#first is used for Enumerable#product as well as #sum (this is mentioned in the issue). So the error message should reflect that. And we should have a spec for #product.
| it { [1, 3_u64].sum(0_i32).should eq(4_u32) } | ||
| it { [1, 3].sum(0_u64).should eq(4_u64) } | ||
| it { [1, 10000000000_u64].sum(0_u64).should eq(10000000001) } | ||
| it "raises if union types are summed" do |
There was a problem hiding this comment.
Specs that take a long time (e.g. run the compiler) should be marked as slow:
| it "raises if union types are summed" do | |
| it "raises if union types are summed", tags: %w[slow] do |
oprypin
changed the title
Enumerable#sum for union elements
Using the first type of a union type as the type of the result of
Enumerable#sum()call can cause runtime failures, e.g.[1, 10000000000_u64].sumwill result in anOverflowError. A safer alternative is to flag/disallows the use of union types withEnumerable#sum()and suggest the use ofEnumerable#sum(initial)with an initial value of the expected type of thesumcall.