From 442daae235baaa3c821c0449844bf9069734a78b Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Tue, 1 Nov 2022 23:00:03 +0800 Subject: [PATCH 1/3] Revert "Compiler: refactor and slightly optimize merging two types (#12436)" This reverts commit bfc33dbda41ed4c3590d02bcdd781fcea5368fd3. --- .../semantic/type_declaration_processor.cr | 6 +- .../crystal/semantic/type_guess_visitor.cr | 2 +- src/compiler/crystal/semantic/type_merge.cr | 80 ++++++++++--------- 3 files changed, 48 insertions(+), 40 deletions(-) diff --git a/src/compiler/crystal/semantic/type_declaration_processor.cr b/src/compiler/crystal/semantic/type_declaration_processor.cr index 65451741fac3..3384ff82b00c 100644 --- a/src/compiler/crystal/semantic/type_declaration_processor.cr +++ b/src/compiler/crystal/semantic/type_declaration_processor.cr @@ -406,7 +406,7 @@ struct Crystal::TypeDeclarationProcessor when NonGenericModuleType type = type_info.type if nilable_instance_var?(owner, name) - type = Type.merge!(type, @program.nil) + type = Type.merge!([type, @program.nil]) end # Same as above, only Nil makes no sense @@ -423,7 +423,7 @@ struct Crystal::TypeDeclarationProcessor when GenericClassType type = type_info.type if nilable_instance_var?(owner, name) - type = Type.merge!(type, @program.nil) + type = Type.merge!([type, @program.nil]) end # Same as above, only Nil makes no sense @@ -442,7 +442,7 @@ struct Crystal::TypeDeclarationProcessor when GenericModuleType type = type_info.type if nilable_instance_var?(owner, name) - type = Type.merge!(type, @program.nil) + type = Type.merge!([type, @program.nil]) end declare_meta_type_var(owner.instance_vars, owner, name, type, type_info.location, instance_var: true, annotations: type_info.annotations) diff --git a/src/compiler/crystal/semantic/type_guess_visitor.cr b/src/compiler/crystal/semantic/type_guess_visitor.cr index 64c84ebcb888..b802558c7bdd 100644 --- a/src/compiler/crystal/semantic/type_guess_visitor.cr +++ b/src/compiler/crystal/semantic/type_guess_visitor.cr @@ -419,7 +419,7 @@ module Crystal info.add_annotations(annotations) if annotations vars[name] = info else - info.type = Type.merge!(info.type, type) + info.type = Type.merge!([info.type, type]) info.outside_def = true if @outside_def info.add_annotations(annotations) if annotations vars[name] = info diff --git a/src/compiler/crystal/semantic/type_merge.cr b/src/compiler/crystal/semantic/type_merge.cr index 027af4fcfa23..ae3e0f1d2af1 100644 --- a/src/compiler/crystal/semantic/type_merge.cr +++ b/src/compiler/crystal/semantic/type_merge.cr @@ -5,69 +5,77 @@ module Crystal def type_merge(types : Indexable(Type?)) : Type? case types.size when 0 - nil + return nil when 1 - types.first + return types.first when 2 # Merging two types is the most common case, so we optimize it first, second = types - type_merge(first, second) + did_merge, merged_type = type_merge_two(first, second) + return merged_type if did_merge else - combined_union_of compact_types(types) + # combined_union_of end + + combined_union_of compact_types(types) end def type_merge(nodes : Indexable(ASTNode)) : Type? case nodes.size when 0 - nil + return nil when 1 - nodes.first.type? + return nodes.first.type? when 2 # Merging two types is the most common case, so we optimize it first, second = nodes - type_merge(first.type?, second.type?) + did_merge, merged_type = type_merge_two(first.type?, second.type?) + return merged_type if did_merge else - combined_union_of compact_types(nodes, &.type?) + # combined_union_of end - end - - def type_merge(first : Type?, second : Type?) : Type? - # Same, so return any of them - return first if first == second - - # First is nil, so return second - return second unless first - # Second is nil, so return first - return first unless second + combined_union_of compact_types(nodes, &.type?) + end - # NoReturn is removed from unions - return second if first.no_return? - return first if second.no_return? + def type_merge_two(first, second) + if first == second + # Same, so return any of them + {true, first} + elsif first + if second + # first and second not nil and different + if first.opaque_id > second.opaque_id + first, second = second, first + end - # Check if a non-union type is part of a union type - if !first.is_a?(UnionType) && second.is_a?(UnionType) && second.union_types.includes?(first) - return second - end + if first.nil_type? + if second.is_a?(UnionType) && second.union_types.includes?(first) + return true, second + end + end - if !second.is_a?(UnionType) && first.is_a?(UnionType) && first.union_types.includes?(second) - return first + # puts "#{first} vs. #{second}" + {false, nil} + else + # Second is nil, so return first + {true, first} + end + else + # First is nil, so return second + {true, second} end - - # General case - combined_union_of compact_types({first, second}) end - def type_merge_union_of(types : Array(Type)) : Type? + def type_merge_union_of(types : Array(Type)) union_of compact_types(types) end - def compact_types(types) : Array(Type) + def compact_types(types) compact_types(types) { |type| type } end - def compact_types(objects) : Array(Type) + def compact_types(objects) all_types = Array(Type).new(objects.size) objects.each { |obj| add_type all_types, yield(obj) } all_types.reject! &.no_return? if all_types.size > 1 @@ -174,12 +182,12 @@ module Crystal end end - def self.merge!(types_or_nodes) : Type + def self.merge!(types_or_nodes) merge(types_or_nodes).not_nil! end - def self.merge!(type1 : Type, type2 : Type) : Type - type1.program.type_merge(type1, type2).not_nil! + def self.merge!(type1 : Type, type2 : Type) + merge!([type1, type2]) end # Given two non-union types T and U, returns their least common ancestor From b0d0c8aadddcffaf88494428b4533182e7122f4c Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Tue, 1 Nov 2022 23:30:49 +0800 Subject: [PATCH 2/3] Revert "Revert "Compiler: refactor and slightly optimize merging two types (#12436)"" This reverts commit 442daae235baaa3c821c0449844bf9069734a78b. --- .../semantic/type_declaration_processor.cr | 6 +- .../crystal/semantic/type_guess_visitor.cr | 2 +- src/compiler/crystal/semantic/type_merge.cr | 80 +++++++++---------- 3 files changed, 40 insertions(+), 48 deletions(-) diff --git a/src/compiler/crystal/semantic/type_declaration_processor.cr b/src/compiler/crystal/semantic/type_declaration_processor.cr index 3384ff82b00c..65451741fac3 100644 --- a/src/compiler/crystal/semantic/type_declaration_processor.cr +++ b/src/compiler/crystal/semantic/type_declaration_processor.cr @@ -406,7 +406,7 @@ struct Crystal::TypeDeclarationProcessor when NonGenericModuleType type = type_info.type if nilable_instance_var?(owner, name) - type = Type.merge!([type, @program.nil]) + type = Type.merge!(type, @program.nil) end # Same as above, only Nil makes no sense @@ -423,7 +423,7 @@ struct Crystal::TypeDeclarationProcessor when GenericClassType type = type_info.type if nilable_instance_var?(owner, name) - type = Type.merge!([type, @program.nil]) + type = Type.merge!(type, @program.nil) end # Same as above, only Nil makes no sense @@ -442,7 +442,7 @@ struct Crystal::TypeDeclarationProcessor when GenericModuleType type = type_info.type if nilable_instance_var?(owner, name) - type = Type.merge!([type, @program.nil]) + type = Type.merge!(type, @program.nil) end declare_meta_type_var(owner.instance_vars, owner, name, type, type_info.location, instance_var: true, annotations: type_info.annotations) diff --git a/src/compiler/crystal/semantic/type_guess_visitor.cr b/src/compiler/crystal/semantic/type_guess_visitor.cr index b802558c7bdd..64c84ebcb888 100644 --- a/src/compiler/crystal/semantic/type_guess_visitor.cr +++ b/src/compiler/crystal/semantic/type_guess_visitor.cr @@ -419,7 +419,7 @@ module Crystal info.add_annotations(annotations) if annotations vars[name] = info else - info.type = Type.merge!([info.type, type]) + info.type = Type.merge!(info.type, type) info.outside_def = true if @outside_def info.add_annotations(annotations) if annotations vars[name] = info diff --git a/src/compiler/crystal/semantic/type_merge.cr b/src/compiler/crystal/semantic/type_merge.cr index ae3e0f1d2af1..027af4fcfa23 100644 --- a/src/compiler/crystal/semantic/type_merge.cr +++ b/src/compiler/crystal/semantic/type_merge.cr @@ -5,77 +5,69 @@ module Crystal def type_merge(types : Indexable(Type?)) : Type? case types.size when 0 - return nil + nil when 1 - return types.first + types.first when 2 # Merging two types is the most common case, so we optimize it first, second = types - did_merge, merged_type = type_merge_two(first, second) - return merged_type if did_merge + type_merge(first, second) else - # combined_union_of + combined_union_of compact_types(types) end - - combined_union_of compact_types(types) end def type_merge(nodes : Indexable(ASTNode)) : Type? case nodes.size when 0 - return nil + nil when 1 - return nodes.first.type? + nodes.first.type? when 2 # Merging two types is the most common case, so we optimize it first, second = nodes - did_merge, merged_type = type_merge_two(first.type?, second.type?) - return merged_type if did_merge + type_merge(first.type?, second.type?) else - # combined_union_of + combined_union_of compact_types(nodes, &.type?) end - - combined_union_of compact_types(nodes, &.type?) end - def type_merge_two(first, second) - if first == second - # Same, so return any of them - {true, first} - elsif first - if second - # first and second not nil and different - if first.opaque_id > second.opaque_id - first, second = second, first - end + def type_merge(first : Type?, second : Type?) : Type? + # Same, so return any of them + return first if first == second - if first.nil_type? - if second.is_a?(UnionType) && second.union_types.includes?(first) - return true, second - end - end + # First is nil, so return second + return second unless first - # puts "#{first} vs. #{second}" - {false, nil} - else - # Second is nil, so return first - {true, first} - end - else - # First is nil, so return second - {true, second} + # Second is nil, so return first + return first unless second + + # NoReturn is removed from unions + return second if first.no_return? + return first if second.no_return? + + # Check if a non-union type is part of a union type + if !first.is_a?(UnionType) && second.is_a?(UnionType) && second.union_types.includes?(first) + return second end + + if !second.is_a?(UnionType) && first.is_a?(UnionType) && first.union_types.includes?(second) + return first + end + + # General case + combined_union_of compact_types({first, second}) end - def type_merge_union_of(types : Array(Type)) + def type_merge_union_of(types : Array(Type)) : Type? union_of compact_types(types) end - def compact_types(types) + def compact_types(types) : Array(Type) compact_types(types) { |type| type } end - def compact_types(objects) + def compact_types(objects) : Array(Type) all_types = Array(Type).new(objects.size) objects.each { |obj| add_type all_types, yield(obj) } all_types.reject! &.no_return? if all_types.size > 1 @@ -182,12 +174,12 @@ module Crystal end end - def self.merge!(types_or_nodes) + def self.merge!(types_or_nodes) : Type merge(types_or_nodes).not_nil! end - def self.merge!(type1 : Type, type2 : Type) - merge!([type1, type2]) + def self.merge!(type1 : Type, type2 : Type) : Type + type1.program.type_merge(type1, type2).not_nil! end # Given two non-union types T and U, returns their least common ancestor From 4ed648713d0350267e3a738b08c02de7e29a7dd0 Mon Sep 17 00:00:00 2001 From: Caspian Baska Date: Tue, 1 Nov 2022 23:33:36 +0800 Subject: [PATCH 3/3] Revert broken generalisation of non-union type merged with union type --- src/compiler/crystal/semantic/type_merge.cr | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/compiler/crystal/semantic/type_merge.cr b/src/compiler/crystal/semantic/type_merge.cr index 027af4fcfa23..9b2fe4e236f5 100644 --- a/src/compiler/crystal/semantic/type_merge.cr +++ b/src/compiler/crystal/semantic/type_merge.cr @@ -46,12 +46,11 @@ module Crystal return second if first.no_return? return first if second.no_return? - # Check if a non-union type is part of a union type - if !first.is_a?(UnionType) && second.is_a?(UnionType) && second.union_types.includes?(first) + if first.nil_type? && second.is_a?(UnionType) && second.union_types.includes?(first) return second end - if !second.is_a?(UnionType) && first.is_a?(UnionType) && first.union_types.includes?(second) + if second.nil_type? && first.is_a?(UnionType) && first.union_types.includes?(second) return first end