From e39b4831ef873086af8d843391c220ae47df685a Mon Sep 17 00:00:00 2001 From: Venkatesh-Prasad Ranganath Date: Thu, 26 Dec 2024 14:13:15 -0600 Subject: [PATCH 01/10] 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 02/10] 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 03/10] 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 04/10] 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 05/10] 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 06/10] 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 " + From 946cf6afcd10864f76d8fb731131de3432e136a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Sat, 22 Feb 2025 14:38:51 +0100 Subject: [PATCH 07/10] Apply suggestions from code review Co-authored-by: Sijawusz Pur Rahnama --- src/enumerable.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/enumerable.cr b/src/enumerable.cr index f341409dc66c..ac1f9a7f9095 100644 --- a/src/enumerable.cr +++ b/src/enumerable.cr @@ -2298,7 +2298,7 @@ module Enumerable(T) def self.type {% if X.union? %} {{ - raise("`Enumerable#sum()` and `#product()` do not support 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.") From d8c7c736b3ed495f51406c03c244c62bbcf99685 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Sat, 22 Feb 2025 15:24:30 +0100 Subject: [PATCH 08/10] Apply suggestions from code review Co-authored-by: Sijawusz Pur Rahnama --- spec/std/enumerable_spec.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index baffa410110e..443c654dea98 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1373,7 +1373,7 @@ describe "Enumerable" do require "prelude" [1, 10000000000_u64].sum CRYSTAL - "`Enumerable#sum()` and `#product()` do not support Union " + + "`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." @@ -1428,7 +1428,7 @@ describe "Enumerable" do require "prelude" [1, 10000000000_u64].product CRYSTAL - "`Enumerable#sum()` and `#product()` do not support Union " + + "`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." From e9387437d8984952191216292e98b5ac7a32f462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Mon, 24 Feb 2025 10:26:56 +0100 Subject: [PATCH 09/10] Use `assert_compile_error` --- spec/std/enumerable_spec.cr | 6 +++--- spec/std/spec_helper.cr | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index 443c654dea98..cd4fd593b013 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1,5 +1,5 @@ require "spec" -require "../spec_helper" +require "./spec_helper" require "spec/helpers/iterate" module SomeInterface; end @@ -1369,7 +1369,7 @@ describe "Enumerable" do 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, + assert_compile_error <<-CRYSTAL, require "prelude" [1, 10000000000_u64].sum CRYSTAL @@ -1424,7 +1424,7 @@ describe "Enumerable" do 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, + assert_compile_error <<-CRYSTAL, require "prelude" [1, 10000000000_u64].product CRYSTAL diff --git a/spec/std/spec_helper.cr b/spec/std/spec_helper.cr index 8bb0a9e1a2f2..f0c03d05ece5 100644 --- a/spec/std/spec_helper.cr +++ b/spec/std/spec_helper.cr @@ -101,6 +101,33 @@ def compile_file(source_file, *, bin_name = "executable_file", flags = %w(), fil end end +def assert_compile_error(source, expected_error, *, flags = %w(), file = __FILE__, line = __LINE__) + # can't use backtick in interpreted code (#12241) + pending_interpreted! "Unable to compile Crystal code in interpreted code" + + with_tempfile("source_file", file: file) do |source_file| + File.write(source_file, source) + + bin_name = "executable_file" + with_temp_executable(bin_name, file: file) do |executable_file| + compiler = ENV["CRYSTAL_SPEC_COMPILER_BIN"]? || "bin/crystal" + args = ["build"] + flags + ["-o", executable_file, source_file] + output = IO::Memory.new + status = Process.run(compiler, args, env: { + "CRYSTAL_PATH" => Crystal::PATH, + "CRYSTAL_LIBRARY_PATH" => Crystal::LIBRARY_PATH, + "CRYSTAL_CACHE_DIR" => Crystal::CACHE_DIR, + "NO_COLOR" => "1", + }, output: output, error: output) + + output.to_s.should contain(expected_error) + + status.success?.should be_false + File.exists?(executable_file).should be_false + end + end +end + def compile_source(source, flags = %w(), file = __FILE__, &) with_tempfile("source_file", file: file) do |source_file| File.write(source_file, source) From 93962a16d96aa6cc34fcb16465fb5ccbbf4dc1c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Wed, 26 Feb 2025 10:54:38 +0100 Subject: [PATCH 10/10] pending_wasm32 --- spec/std/enumerable_spec.cr | 4 ++-- spec/support/wasm32.cr | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/std/enumerable_spec.cr b/spec/std/enumerable_spec.cr index cd4fd593b013..78c7fa8bc7da 100644 --- a/spec/std/enumerable_spec.cr +++ b/spec/std/enumerable_spec.cr @@ -1368,7 +1368,7 @@ describe "Enumerable" do 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 + pending_wasm32 "raises if union types are summed", tags: %w[slow] do assert_compile_error <<-CRYSTAL, require "prelude" [1, 10000000000_u64].sum @@ -1423,7 +1423,7 @@ describe "Enumerable" do 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 + pending_wasm32 "raises if union types are multiplied", tags: %w[slow] do assert_compile_error <<-CRYSTAL, require "prelude" [1, 10000000000_u64].product diff --git a/spec/support/wasm32.cr b/spec/support/wasm32.cr index 98c9de69ea9e..9beeaa43fb7e 100644 --- a/spec/support/wasm32.cr +++ b/spec/support/wasm32.cr @@ -1,16 +1,16 @@ require "spec" {% if flag?(:wasm32) %} - def pending_wasm32(description = "assert", file = __FILE__, line = __LINE__, end_line = __END_LINE__, &block) - pending("#{description} [wasm32]", file, line, end_line) + def pending_wasm32(description = "assert", file = __FILE__, line = __LINE__, end_line = __END_LINE__, focus : Bool = false, tags : String | Enumerable(String) | Nil = nil, &block) + pending("#{description} [wasm32]", file, line, end_line, focus: focus, tags: tags) end def pending_wasm32(*, describe, file = __FILE__, line = __LINE__, end_line = __END_LINE__, &block) pending_wasm32(describe, file, line, end_line) { } end {% else %} - def pending_wasm32(description = "assert", file = __FILE__, line = __LINE__, end_line = __END_LINE__, &block) - it(description, file, line, end_line, &block) + def pending_wasm32(description = "assert", file = __FILE__, line = __LINE__, end_line = __END_LINE__, focus : Bool = false, tags : String | Enumerable(String) | Nil = nil, &block) + it(description, file, line, end_line, focus: focus, tags: tags, &block) end def pending_wasm32(*, describe, file = __FILE__, line = __LINE__, end_line = __END_LINE__, &block)