From 6a3418ed36f25fca9198af7ae4f87cbde5dd00e6 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Thu, 2 May 2024 08:57:20 -0400 Subject: [PATCH 1/7] Allow rescuing exceptions that include a module --- spec/compiler/codegen/exception_spec.cr | 43 +++++++++++++++++++ src/compiler/crystal/semantic/main_visitor.cr | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/spec/compiler/codegen/exception_spec.cr b/spec/compiler/codegen/exception_spec.cr index 70c5e331aa00..965f63a19366 100644 --- a/spec/compiler/codegen/exception_spec.cr +++ b/spec/compiler/codegen/exception_spec.cr @@ -1319,4 +1319,47 @@ describe "Code gen: exception" do end )) end + + it "handles rescuing module type" do + run(%( + require "prelude" + + module Foo; end + + class Ex1 < Exception + include Foo + end + + x = 0 + begin + raise Ex1.new + rescue Foo + x = 1 + end + x + )).to_i.should eq(1) + end + + it "handles rescuing union between module type and class type" do + run(%( + require "prelude" + + module Foo; end + + abstract class BaseError < Exception; end + class Ex2 < BaseError; end + + class Ex1 < BaseError + include Foo + end + + x = 0 + begin + raise Ex1.new + rescue Foo | BaseError + x = 1 + end + x + )).to_i.should eq(1) + end end diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index 61d1aa508a46..b98823a8bb0f 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2775,7 +2775,7 @@ module Crystal types = node_types.map do |type| type.accept self instance_type = type.type.instance_type - unless instance_type.implements?(@program.exception) + unless instance_type.implements?(@program.exception) || instance_type.module? type.raise "#{instance_type} is not a subclass of Exception" end instance_type From b81c8f181005e92df0c8096b7fa5868f7bf8d75b Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Thu, 2 May 2024 09:47:40 -0400 Subject: [PATCH 2/7] Add spec for rescuing a union of module types --- spec/compiler/codegen/exception_spec.cr | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/spec/compiler/codegen/exception_spec.cr b/spec/compiler/codegen/exception_spec.cr index 965f63a19366..802daec946ad 100644 --- a/spec/compiler/codegen/exception_spec.cr +++ b/spec/compiler/codegen/exception_spec.cr @@ -1362,4 +1362,29 @@ describe "Code gen: exception" do x )).to_i.should eq(1) end + + it "handles rescuing union between module types" do + run(%( + require "prelude" + + module Foo; end + module Bar; end + + class Ex1 < Exception + include Foo + end + + class Ex2 < Exception + include Bar + end + + x = 0 + begin + raise Ex1.new + rescue Foo | Bar + x = 1 + end + x + )).to_i.should eq(1) + end end From 5fb4a1fd972be4e4295b913d926ea3d67df7109b Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Tue, 7 May 2024 09:10:25 -0400 Subject: [PATCH 3/7] Add spec to ensure it does not rescue just any module --- spec/compiler/codegen/exception_spec.cr | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/spec/compiler/codegen/exception_spec.cr b/spec/compiler/codegen/exception_spec.cr index 802daec946ad..fdf1c83c27bd 100644 --- a/spec/compiler/codegen/exception_spec.cr +++ b/spec/compiler/codegen/exception_spec.cr @@ -1387,4 +1387,29 @@ describe "Code gen: exception" do x )).to_i.should eq(1) end + + it "does not rescue just any module" do + run(%( + require "prelude" + + module Foo; end + module Bar; end + + class Ex < Exception + include Foo + end + + x = 0 + begin + begin + raise Ex.new("oh no") + rescue Bar + x = 1 + end + rescue ex + x = 2 + end + x + )).to_i.should eq(2) + end end From 36b22ff2144169cc87d9363c2f2542799d57174d Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Tue, 7 May 2024 20:44:34 -0400 Subject: [PATCH 4/7] Handle rescuing proper unions --- spec/compiler/codegen/exception_spec.cr | 45 +++++++++++++++++++ spec/compiler/semantic/exception_spec.cr | 4 ++ src/compiler/crystal/semantic/main_visitor.cr | 15 ++++++- 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/spec/compiler/codegen/exception_spec.cr b/spec/compiler/codegen/exception_spec.cr index fdf1c83c27bd..29ac3ed11d84 100644 --- a/spec/compiler/codegen/exception_spec.cr +++ b/spec/compiler/codegen/exception_spec.cr @@ -1412,4 +1412,49 @@ describe "Code gen: exception" do x )).to_i.should eq(2) end + + it "rescues a valid union" do + run(%( + require "prelude" + + module Foo; end + module Bar; end + + class Ex < Exception + include Foo + end + + x = 0 + begin + raise Ex.new("oh no") + rescue Union(Foo, Bar) + x = 1 + end + x + )).to_i.should eq(1) + end + + it "does not rescue just any union" do + run(%( + require "prelude" + + module Foo; end + module Bar; end + module Baz; end + + class Ex < Exception + include Foo + end + + x = 0 + begin + raise Ex.new("oh no") + rescue Union(Bar, Baz) + x = 1 + rescue + x = 2 + end + x + )).to_i.should eq(2) + end end diff --git a/spec/compiler/semantic/exception_spec.cr b/spec/compiler/semantic/exception_spec.cr index 1b5328e5edeb..80a78e466e22 100644 --- a/spec/compiler/semantic/exception_spec.cr +++ b/spec/compiler/semantic/exception_spec.cr @@ -148,6 +148,10 @@ describe "Semantic: exception" do assert_error "begin; rescue ex : Int32; end", "Int32 is not a subclass of Exception" end + it "errors if caught exception is a union but not all types are valid" do + assert_error "begin; rescue ex : Union(Exception, String); end", "(Exception | String) is not a subclass of Exception" + end + it "errors if caught exception is not a subclass of Exception without var" do assert_error "begin; rescue Int32; end", "Int32 is not a subclass of Exception" end diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index b98823a8bb0f..04962391e2f4 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2771,13 +2771,26 @@ module Crystal end def visit(node : Rescue) + is_exception_type_callback = Proc(Crystal::Type, Bool).new do |type| + type.implements?(@program.exception) || type.module? + end + if node_types = node.types types = node_types.map do |type| type.accept self instance_type = type.type.instance_type - unless instance_type.implements?(@program.exception) || instance_type.module? + + is_exception_type = case instance_type + when UnionType + instance_type.union_types.all? &is_exception_type_callback + else + is_exception_type_callback.call instance_type + end + + unless is_exception_type type.raise "#{instance_type} is not a subclass of Exception" end + instance_type end end From cd605ff5e9d98598f9861486ba27af8addda3390 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Tue, 7 May 2024 20:59:13 -0400 Subject: [PATCH 5/7] Emit more proper error messages --- spec/compiler/semantic/exception_spec.cr | 6 +++--- src/compiler/crystal/semantic/main_visitor.cr | 20 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/spec/compiler/semantic/exception_spec.cr b/spec/compiler/semantic/exception_spec.cr index 80a78e466e22..37302407b113 100644 --- a/spec/compiler/semantic/exception_spec.cr +++ b/spec/compiler/semantic/exception_spec.cr @@ -145,15 +145,15 @@ describe "Semantic: exception" do end it "errors if caught exception is not a subclass of Exception" do - assert_error "begin; rescue ex : Int32; end", "Int32 is not a subclass of Exception" + assert_error "begin; rescue ex : Int32; end", "Int32 is not a module type or subclass of Exception" end it "errors if caught exception is a union but not all types are valid" do - assert_error "begin; rescue ex : Union(Exception, String); end", "(Exception | String) is not a subclass of Exception" + assert_error "begin; rescue ex : Union(Exception, String); end", "Not all union members of (Exception | String) are a module type or subclass of Exception" end it "errors if caught exception is not a subclass of Exception without var" do - assert_error "begin; rescue Int32; end", "Int32 is not a subclass of Exception" + assert_error "begin; rescue Int32; end", "Int32 is not a module type or subclass of Exception" end assert_syntax_error "begin; rescue ex; rescue ex : Foo; end; ex", diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index 04962391e2f4..5f75b76b8df4 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2771,7 +2771,7 @@ module Crystal end def visit(node : Rescue) - is_exception_type_callback = Proc(Crystal::Type, Bool).new do |type| + is_exception_type = Proc(Crystal::Type, Bool).new do |type| type.implements?(@program.exception) || type.module? end @@ -2780,15 +2780,15 @@ module Crystal type.accept self instance_type = type.type.instance_type - is_exception_type = case instance_type - when UnionType - instance_type.union_types.all? &is_exception_type_callback - else - is_exception_type_callback.call instance_type - end - - unless is_exception_type - type.raise "#{instance_type} is not a subclass of Exception" + case instance_type + when UnionType + if !instance_type.union_types.all?(&is_exception_type) + type.raise "Not all union members of #{instance_type} are a module type or subclass of Exception" + end + else + unless is_exception_type.call instance_type + type.raise "#{instance_type} is not a module type or subclass of Exception" + end end instance_type From 4274222071d0ab78c7614b521e767caca037d55c Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Wed, 8 May 2024 08:34:54 -0400 Subject: [PATCH 6/7] Better handle nested unions --- spec/compiler/codegen/exception_spec.cr | 22 +++++++++++++++++++ spec/compiler/semantic/exception_spec.cr | 4 ++++ src/compiler/crystal/semantic/main_visitor.cr | 16 +++++++++----- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/spec/compiler/codegen/exception_spec.cr b/spec/compiler/codegen/exception_spec.cr index 29ac3ed11d84..1b3bb42115ee 100644 --- a/spec/compiler/codegen/exception_spec.cr +++ b/spec/compiler/codegen/exception_spec.cr @@ -1434,6 +1434,28 @@ describe "Code gen: exception" do )).to_i.should eq(1) end + it "rescues a valid nested union" do + run(%( + require "prelude" + + module Foo; end + module Bar; end + module Baz; end + + class Ex < Exception + include Foo + end + + x = 0 + begin + raise Ex.new("oh no") + rescue Union(Baz, Union(Foo, Bar)) + x = 1 + end + x + )).to_i.should eq(1) + end + it "does not rescue just any union" do run(%( require "prelude" diff --git a/spec/compiler/semantic/exception_spec.cr b/spec/compiler/semantic/exception_spec.cr index 37302407b113..95967c282a1e 100644 --- a/spec/compiler/semantic/exception_spec.cr +++ b/spec/compiler/semantic/exception_spec.cr @@ -152,6 +152,10 @@ describe "Semantic: exception" do assert_error "begin; rescue ex : Union(Exception, String); end", "Not all union members of (Exception | String) are a module type or subclass of Exception" end + it "errors if caught exception is a nested union but not all types are valid" do + assert_error "begin; rescue ex : Union(Exception, Union(Exception, String)); end", "Not all union members of (Exception | String) are a module type or subclass of Exception" + end + it "errors if caught exception is not a subclass of Exception without var" do assert_error "begin; rescue Int32; end", "Int32 is not a module type or subclass of Exception" end diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index 5f75b76b8df4..991c6c9e2b5e 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2770,11 +2770,17 @@ module Crystal false end - def visit(node : Rescue) - is_exception_type = Proc(Crystal::Type, Bool).new do |type| - type.implements?(@program.exception) || type.module? + private def allowed_type_in_rescue?(type : UnionType) : Bool + type.union_types.all? do |subtype| + allowed_type_in_rescue? subtype end + end + private def allowed_type_in_rescue?(type : Crystal::Type) : Bool + type.implements?(@program.exception) || type.module? + end + + def visit(node : Rescue) if node_types = node.types types = node_types.map do |type| type.accept self @@ -2782,11 +2788,11 @@ module Crystal case instance_type when UnionType - if !instance_type.union_types.all?(&is_exception_type) + unless self.allowed_type_in_rescue? instance_type type.raise "Not all union members of #{instance_type} are a module type or subclass of Exception" end else - unless is_exception_type.call instance_type + unless self.allowed_type_in_rescue? instance_type type.raise "#{instance_type} is not a module type or subclass of Exception" end end From 88f298f36d5c482964a4ad5a6156584b948758e3 Mon Sep 17 00:00:00 2001 From: George Dietrich Date: Wed, 8 May 2024 09:55:15 -0400 Subject: [PATCH 7/7] Do a pass on the error message --- spec/compiler/semantic/exception_spec.cr | 8 ++++---- src/compiler/crystal/semantic/main_visitor.cr | 11 ++--------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/spec/compiler/semantic/exception_spec.cr b/spec/compiler/semantic/exception_spec.cr index 95967c282a1e..7395999b0682 100644 --- a/spec/compiler/semantic/exception_spec.cr +++ b/spec/compiler/semantic/exception_spec.cr @@ -145,19 +145,19 @@ describe "Semantic: exception" do end it "errors if caught exception is not a subclass of Exception" do - assert_error "begin; rescue ex : Int32; end", "Int32 is not a module type or subclass of Exception" + assert_error "begin; rescue ex : Int32; end", "Int32 cannot be used for `rescue`. Only subclasses of `Exception` and modules, or unions thereof, are allowed." end it "errors if caught exception is a union but not all types are valid" do - assert_error "begin; rescue ex : Union(Exception, String); end", "Not all union members of (Exception | String) are a module type or subclass of Exception" + assert_error "begin; rescue ex : Union(Exception, String); end", "(Exception | String) cannot be used for `rescue`. Only subclasses of `Exception` and modules, or unions thereof, are allowed." end it "errors if caught exception is a nested union but not all types are valid" do - assert_error "begin; rescue ex : Union(Exception, Union(Exception, String)); end", "Not all union members of (Exception | String) are a module type or subclass of Exception" + assert_error "begin; rescue ex : Union(Exception, Union(Exception, String)); end", "(Exception | String) cannot be used for `rescue`. Only subclasses of `Exception` and modules, or unions thereof, are allowed." end it "errors if caught exception is not a subclass of Exception without var" do - assert_error "begin; rescue Int32; end", "Int32 is not a module type or subclass of Exception" + assert_error "begin; rescue Int32; end", "Int32 cannot be used for `rescue`. Only subclasses of `Exception` and modules, or unions thereof, are allowed." end assert_syntax_error "begin; rescue ex; rescue ex : Foo; end; ex", diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index 991c6c9e2b5e..c33c64e893ff 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2786,15 +2786,8 @@ module Crystal type.accept self instance_type = type.type.instance_type - case instance_type - when UnionType - unless self.allowed_type_in_rescue? instance_type - type.raise "Not all union members of #{instance_type} are a module type or subclass of Exception" - end - else - unless self.allowed_type_in_rescue? instance_type - type.raise "#{instance_type} is not a module type or subclass of Exception" - end + unless self.allowed_type_in_rescue? instance_type + type.raise "#{instance_type} cannot be used for `rescue`. Only subclasses of `Exception` and modules, or unions thereof, are allowed." end instance_type