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

Fix the return type of Enumerable#sum/product for union elements #15314

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions spec/std/enumerable_spec.cr
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require "spec"
require "../spec_helper"
require "spec/helpers/iterate"

module SomeInterface; end
Expand Down Expand Up @@ -1364,6 +1365,18 @@ describe "Enumerable" do
it { [1, 2, 3].sum(4.5).should eq(10.5) }
it { (1..3).sum { |x| x * 2 }.should eq(12) }
it { (1..3).sum(1.5) { |x| x * 2 }.should eq(13.5) }
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", tags: %w[slow] do
exc = assert_error <<-CRYSTAL,
require "prelude"
[1, 10000000000_u64].sum
CRYSTAL
"Enumerable#sum() and #product() do not support Union types. " +
"Instead, use Enumerable#sum(initial) and #product(initial), " +
"respectively, with an initial value of the intended type of the call."
end

it "uses additive_identity from type" do
typeof([1, 2, 3].sum).should eq(Int32)
Expand Down Expand Up @@ -1405,6 +1418,19 @@ describe "Enumerable" do
typeof([1.5, 2.5, 3.5].product).should eq(Float64)
typeof([1, 2, 3].product(&.to_f)).should eq(Float64)
end

it { [1, 3_u64].product(3_i32).should eq(9_u32) }
it { [1, 3].product(3_u64).should eq(9_u64) }
it { [1, 10000000000_u64].product(3_u64).should eq(30000000000_u64) }
it "raises if union types are multiplied", tags: %w[slow] do
exc = assert_error <<-CRYSTAL,
require "prelude"
[1, 10000000000_u64].product
CRYSTAL
"Enumerable#sum() and #product() do not support Union types. " +
"Instead, use Enumerable#sum(initial) and #product(initial), " +
"respectively, with an initial value of the intended type of the call."
end
end

describe "first" do
Expand Down
19 changes: 15 additions & 4 deletions src/enumerable.cr
Original file line number Diff line number Diff line change
Expand Up @@ -1808,7 +1808,10 @@ module Enumerable(T)
# Expects all types returned from the block to respond to `#+` method.
#
# This method calls `.additive_identity` on the yielded type to determine the
# type of the sum value.
# type of the sum value. 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 value.
#
# If the collection is empty, returns `additive_identity`.
#
Expand Down Expand Up @@ -1886,8 +1889,11 @@ module Enumerable(T)
#
# Expects all types returned from the block to respond to `#*` method.
#
# This method calls `.multiplicative_identity` on the element type to determine the
# type of the sum value.
# This method calls `.multiplicative_identity` on the element type to
# determine the type of the product value. Hence, it can fail to compile if
# `.multiplicative_identity` fails to determine a safe type, e.g., in case
# of union types. In such cases, use `product(initial)` with an initial
# value of the expected type of the product value.
#
# If the collection is empty, returns `multiplicative_identity`.
#
Expand Down Expand Up @@ -2292,7 +2298,12 @@ module Enumerable(T)
# if the type is a union.
def self.first
{% if X.union? %}
{{X.union_types.first}}
{{
raise("Enumerable#sum() and #product() do not support Union types. " +
"Instead, use Enumerable#sum(initial) and #product(initial), " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistic suggestion:

Suggested change
raise("Enumerable#sum() and #product() do not support Union types. " +
"Instead, use Enumerable#sum(initial) and #product(initial), " +
raise("`Enumerable#sum` and `#product` do not support Union types. " +
"Instead, use `Enumerable#sum(initial)` and `#product(initial)`, " +

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make this change.

Out of curiosity, assuming the back ticks are Markdown's code syntax, what is the effect of including them in in error messages (which I assume are processed and rendered as plain text)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my eyes it makes them more standout and a clear sign we're referencing the method/type definitions, as in the other, doc-related places.

"respectively, with an initial value of the intended type " +
"of the call.")
}}
{% else %}
X
{% end %}
Expand Down