Return type of Enumerable#sum and #product for union elements
#15269
Labels
Comments
straight-shoota
added
kind:bug
rvprasad
added a commit
to rvprasad/crystal
that referenced
this issue
Dec 21, 2024
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.
|
@straight-shoota I have a fix along with a couple of tests (see https://github.com/rvprasad/crystal) but I am facing one issue: how do I test for compiler errors in automated tests? I tried using |
|
assert_error <<-CRYSTAL, "does support Union types"
require "prelude"
[1_i8, 2_i16].sum
CRYSTAL |
rvprasad
added a commit
to rvprasad/crystal
that referenced
this issue
Dec 22, 2024
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.
rvprasad
added a commit
to rvprasad/crystal
that referenced
this issue
Dec 22, 2024
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.
|
Thanks. That worked! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Enumerable(T)#sumand#productusually use the element type (T) as the type of the sum/product unless specified.This is usually a good default but raises the question what should happen when the element type is not a single type, but a union?
Turns out, the implementations picks the first type in the union (note: order of types in a union is lexicographic by the type name):
I think this is quite unexpected and can lead to surprising behaviour: For example
[1_i16, 12345678_i32].sumraises an overflow error when the result would fit perfectly intoInt32.If the implementation picks one type from the union, I figure it should rather be the biggest type of the union. For number types this might be relatively easy, but not without issues (e.g. when combinging signed and unsigned types of the same width). And there would be no way this could work for custom types. I don't have a real use cases for math operations on unions outside of
Number, but there is probably some with related values types.In my opinion, it would probably be best to not automatically pick a type if there are multiple options. It's better to make this a compile time error instead of running into weird overflows at runtime because the sum type is smaller than expected.
It's trivial to specify the desired target type by passing an initial value in that type.
For example:
Of course this would be a breaking change. But IMO that's acceptable because it corrects a confusing behaviour. This is a rarely used feature anyway, so impact should be minimal.
This repo doesn't seem to use this and test-ecosystem doesn't show anything breaking either: https://github.com/crystal-lang/test-ecosystem/actions/runs/12285549227/job/34286237286
This issue was originally reported at https://fosstodon.org/@[email protected]/113626739326812835
The text was updated successfully, but these errors were encountered: