Fix the return type of Enumerable#sum for union elements

[rvprasad]

Fixes #15269

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].sum will result in an OverflowError. A safer alternative is to flag/disallows 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.

[BlobCodes]

While checking this PR, I noticed that the Enumerable#sum method generally seems to have issues.

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 .zero on String.

---

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 #sum(Union) is just to raise an error in a minor release without any warnings beforehand.
There's the .warning macro function instead, for example.

[rvprasad]

David, thanks for the input!

I agree the doc should be updated. How about updating the document about .additive_identity as follows (being consistent with other bits of the document)?

This method calls .additive_identity on the yielded type to determine the type of the sum call. Hence, it can fail to compile if .additive_identity fails to determine a safe type, e.g., in case of union types. In such cases, use sum(initial) with an initial value of the expected type of the sum call.

As for warning vs raise, based on the simplicity of the fix (i.e., sum() to sum(initial)) and assuming there are very few uses of this feature in the wild, I doubt this change will cause a lot of toil/churn. That said, I am fine with gradually introducing this change by using warning instead of raise.

[straight-shoota]

Let's discuss further improving the documentation of Enumerable#sum in a separate issue.

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 overflow

So 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]

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.

[rvprasad]

Good catch! Done :)

[straight-shoota]

Specs that take a long time (e.g. run the compiler) should be marked as slow:

Suggested change

	it "raises if union types are summed" do
	it "raises if union types are summed", tags: %w[slow] do

[rvprasad]

Done!

[rvprasad]

I messed up my local repo while syncing it with recent upstream changes. When I tried to clean it up, I incidentally closed this PR :-/

I have addressed all of the comments in this PR thread and created a new PR; #15314. Please review it.

Apologies for any inconvenience.

[Blacksmoke16]

For future reference, try to avoid force pushing as per https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#making-good-pull-requests.