From e39b4831ef873086af8d843391c220ae47df685a Mon Sep 17 00:00:00 2001 From: Venkatesh-Prasad Ranganath Date: Thu, 26 Dec 2024 14:13:15 -0600 Subject: [PATCH 1/6] Fixes #15269 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. --- spec/std/enumerable_spec.cr | 26 ++++++++++++++++++++++++++ src/enumerable.cr | 18 ++++++++++++++---- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index 084fe80dcf96..31984e2ff7a5 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1,4 +1,5 @@ require "spec" +require "../spec_helper" require "spec/helpers/iterate" module SomeInterface; end @@ -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/product() does support Union types. Instead, " + + "use Enumerable#sum/product(initial) with an initial value of " + + "the expected type of the sum/product call." + end it "uses additive_identity from type" do typeof([1, 2, 3].sum).should eq(Int32) @@ -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/product() does support Union types. Instead, " + + "use Enumerable#sum/product(initial) with an initial value of " + + "the expected type of the sum/product call." + end end describe "first" do diff --git a/src/enumerable.cr b/src/enumerable.cr index 0993f38bbc4d..cc65e8c7ce3e 100644 --- a/src/enumerable.cr +++ b/src/enumerable.cr @@ -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`. # @@ -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`. # @@ -2292,7 +2298,11 @@ module Enumerable(T) # if the type is a union. def self.first {% if X.union? %} - {{X.union_types.first}} + {{ + 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.") + }} {% else %} X {% end %} From 6b4eea8ffe3fb809ccb12305ec8baa4268cc161c Mon Sep 17 00:00:00 2001 From: Venkatesh-Prasad Ranganath Date: Fri, 27 Dec 2024 13:52:08 -0600 Subject: [PATCH 2/6] Fix typo in error messages --- spec/std/enumerable_spec.cr | 8 ++++---- src/enumerable.cr | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index 31984e2ff7a5..8847959d9aa8 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1373,9 +1373,9 @@ describe "Enumerable" do require "prelude" [1, 10000000000_u64].sum CRYSTAL - "Enumerable#sum/product() does support Union types. Instead, " + + "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." + "the expected type of the call." end it "uses additive_identity from type" do @@ -1427,9 +1427,9 @@ describe "Enumerable" do require "prelude" [1, 10000000000_u64].product CRYSTAL - "Enumerable#sum/product() does support Union types. Instead, " + + "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." + "the expected type of the call." end end diff --git a/src/enumerable.cr b/src/enumerable.cr index cc65e8c7ce3e..2ec4b7456ae1 100644 --- a/src/enumerable.cr +++ b/src/enumerable.cr @@ -2299,9 +2299,9 @@ module Enumerable(T) def self.first {% if X.union? %} {{ - raise("Enumerable#sum/product() does support Union types. " + + 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.") + "initial value of the expected type of the call.") }} {% else %} X From a09f57b4828ed03fe2242517dd494740893260e1 Mon Sep 17 00:00:00 2001 From: Venkatesh-Prasad Ranganath Date: Fri, 27 Dec 2024 19:36:13 -0600 Subject: [PATCH 3/6] Use precise language The error message and documentation specifically refers to the variants of Enumerable#sum/product that do not support union types. --- spec/std/enumerable_spec.cr | 12 ++++++------ src/enumerable.cr | 7 ++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index 8847959d9aa8..5c66ddf55062 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1373,9 +1373,9 @@ describe "Enumerable" do require "prelude" [1, 10000000000_u64].sum CRYSTAL - "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 call." + "Enumerable#sum() and #product() do not support Union types. " + + "Instead, use Enumerable#sum and #product(initial), respectively, " + + "with an initial value of the intended type of the call." end it "uses additive_identity from type" do @@ -1427,9 +1427,9 @@ describe "Enumerable" do require "prelude" [1, 10000000000_u64].product CRYSTAL - "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 call." + "Enumerable#sum() and #product() do not support Union types. " + + "Instead, use Enumerable#sum and #product(initial), respectively, " + + "with an initial value of the intended type of the call." end end diff --git a/src/enumerable.cr b/src/enumerable.cr index 2ec4b7456ae1..fb78d5c569ce 100644 --- a/src/enumerable.cr +++ b/src/enumerable.cr @@ -2299,9 +2299,10 @@ module Enumerable(T) def self.first {% if X.union? %} {{ - 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 call.") + raise("Enumerable#sum() and #product() do not support Union types. " + + "Instead, use Enumerable#sum and #product(initial), " + + "respectively, with an initial value of the intended type " + + "of the call.") }} {% else %} X From 2dd0012858338ef40b8decf0d9b751676e598262 Mon Sep 17 00:00:00 2001 From: Venkatesh-Prasad Ranganath Date: Sat, 28 Dec 2024 14:55:46 -0600 Subject: [PATCH 4/6] Fix a typo in the error message --- spec/std/enumerable_spec.cr | 8 ++++---- src/enumerable.cr | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index 5c66ddf55062..09d89ddcdf6d 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1374,8 +1374,8 @@ describe "Enumerable" do [1, 10000000000_u64].sum CRYSTAL "Enumerable#sum() and #product() do not support Union types. " + - "Instead, use Enumerable#sum and #product(initial), respectively, " + - "with an initial value of the intended type of the call." + "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 @@ -1428,8 +1428,8 @@ describe "Enumerable" do [1, 10000000000_u64].product CRYSTAL "Enumerable#sum() and #product() do not support Union types. " + - "Instead, use Enumerable#sum and #product(initial), respectively, " + - "with an initial value of the intended type of the call." + "Instead, use Enumerable#sum(initial) and #product(initial), " + + "respectively, with an initial value of the intended type of the call." end end diff --git a/src/enumerable.cr b/src/enumerable.cr index fb78d5c569ce..0e87867b28dd 100644 --- a/src/enumerable.cr +++ b/src/enumerable.cr @@ -2300,7 +2300,7 @@ module Enumerable(T) {% if X.union? %} {{ raise("Enumerable#sum() and #product() do not support Union types. " + - "Instead, use Enumerable#sum and #product(initial), " + + "Instead, use Enumerable#sum(initial) and #product(initial), " + "respectively, with an initial value of the intended type " + "of the call.") }} From 51afe63d0bf22f18b5d520dd3ec36da5130c29da Mon Sep 17 00:00:00 2001 From: Venkatesh-Prasad Ranganath Date: Sat, 28 Dec 2024 18:20:19 -0600 Subject: [PATCH 5/6] Format code fragments/references in the error message --- spec/std/enumerable_spec.cr | 14 ++++++++------ src/enumerable.cr | 8 ++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index 09d89ddcdf6d..baffa410110e 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1373,9 +1373,10 @@ describe "Enumerable" do 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." + "`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 @@ -1427,9 +1428,10 @@ describe "Enumerable" do 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." + "`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 diff --git a/src/enumerable.cr b/src/enumerable.cr index 0e87867b28dd..df0e3b9c5652 100644 --- a/src/enumerable.cr +++ b/src/enumerable.cr @@ -2299,10 +2299,10 @@ module Enumerable(T) def self.first {% if X.union? %} {{ - raise("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.") + raise("`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.") }} {% else %} X From 1389213d11155b41ea2141c266225302e41cf0ef Mon Sep 17 00:00:00 2001 From: Venkatesh-Prasad Ranganath Date: Tue, 31 Dec 2024 14:32:57 -0600 Subject: [PATCH 6/6] Align Reflect's method and related comments with the class' purpose With this PR, Reflect is not used to determine the first type of a union type. This commit aligns Reflect's method and related comments with the class' purpose. --- src/enumerable.cr | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/enumerable.cr b/src/enumerable.cr index df0e3b9c5652..f341409dc66c 100644 --- a/src/enumerable.cr +++ b/src/enumerable.cr @@ -1771,7 +1771,7 @@ module Enumerable(T) end private def additive_identity(reflect) - type = reflect.first + type = reflect.type if type.responds_to? :additive_identity type.additive_identity else @@ -1850,7 +1850,7 @@ module Enumerable(T) # ``` # # This method calls `.multiplicative_identity` on the element type to determine the - # type of the sum value. + # type of the product value. # # If the collection is empty, returns `multiplicative_identity`. # @@ -1858,7 +1858,7 @@ module Enumerable(T) # ([] of Int32).product # => 1 # ``` def product - product Reflect(T).first.multiplicative_identity + product Reflect(T).type.multiplicative_identity end # Multiplies *initial* and all the elements in the collection @@ -1901,7 +1901,7 @@ module Enumerable(T) # ([] of Int32).product { |x| x + 1 } # => 1 # ``` def product(& : T -> _) - product(Reflect(typeof(yield Enumerable.element_type(self))).first.multiplicative_identity) do |value| + product(Reflect(typeof(yield Enumerable.element_type(self))).type.multiplicative_identity) do |value| yield value end end @@ -2293,10 +2293,9 @@ module Enumerable(T) # :nodoc: private struct Reflect(X) - # For now it's just a way to implement `Enumerable#sum` in a way that the - # initial value given to it has the type of the first type in the union, - # if the type is a union. - def self.first + # For now, Reflect is used to reject union types in `#sum()` and + # `#product()` methods. + def self.type {% if X.union? %} {{ raise("`Enumerable#sum()` and `#product()` do not support Union " +