diff --git a/spec/compiler/semantic/macro_overload_spec.cr b/spec/compiler/semantic/macro_overload_spec.cr new file mode 100644 index 000000000000..5ee59dcdbe68 --- /dev/null +++ b/spec/compiler/semantic/macro_overload_spec.cr @@ -0,0 +1,29 @@ +require "../../spec_helper" + +describe "Semantic: macro overload" do + it "doesn't overwrite last macro definition if named args differs" do + assert_type(%( + macro foo(*, arg1) + 1 + end + + macro foo(*, arg2) + "foo" + end + + foo(arg1: true) + )) { int32 } + + assert_type(%( + macro foo(*, arg1) + 1 + end + + macro foo(*, arg2) + "foo" + end + + foo(arg2: true) + )) { string } + end +end diff --git a/src/compiler/crystal/semantic/restrictions.cr b/src/compiler/crystal/semantic/restrictions.cr index 2ff06d1bc5db..74c9d9b924d3 100644 --- a/src/compiler/crystal/semantic/restrictions.cr +++ b/src/compiler/crystal/semantic/restrictions.cr @@ -66,6 +66,8 @@ module Crystal struct DefWithMetadata def restriction_of?(other : DefWithMetadata, owner) + # This is how multiple defs are sorted by 'restrictions' (?) + # If one yields and the other doesn't, none is stricter than the other return false unless yields == other.yields @@ -193,12 +195,38 @@ module Crystal class Macro def overrides?(other : Macro) - # For now we consider that a macro overrides another macro - # if it has the same number of arguments, splat index and - # named arguments. - args.size == other.args.size && - splat_index == other.splat_index && - !!double_splat == !!other.double_splat + # If they have different number of arguments, splat index or presence of + # double splat, no override. + if args.size != other.args.size || + splat_index != other.splat_index || + !!double_splat != !!other.double_splat + return false + end + + self_named_args = self.required_named_arguments + other_named_args = other.required_named_arguments + + # If both don't have named arguments, override. + return true if !self_named_args && !other_named_args + + # If one has required named args and the other doesn't, no override. + return false unless self_named_args && other_named_args + + self_names = self_named_args.map(&.external_name) + other_names = other_named_args.map(&.external_name) + + # If different named arguments names, no override. + return false unless self_names == other_names + + true + end + + def required_named_arguments + if (splat_index = self.splat_index) && splat_index != args.size - 1 + args[splat_index + 1..-1].select { |arg| !arg.default_value }.sort_by &.external_name + else + nil + end end end diff --git a/src/compiler/crystal/types.cr b/src/compiler/crystal/types.cr index 5f6e35cc57a9..87768ebafdc4 100644 --- a/src/compiler/crystal/types.cr +++ b/src/compiler/crystal/types.cr @@ -746,51 +746,57 @@ module Crystal list.each_with_index do |ex_item, i| if item.restriction_of?(ex_item, self) if ex_item.restriction_of?(item, self) + # The two defs have the same signature so item overrides ex_item. list[i] = item a_def.previous = ex_item a_def.doc ||= ex_item.def.doc ex_item.def.next = a_def return ex_item.def else + # item has a new signature, stricter than ex_item. list.insert(i, item) return nil end end end + + # item has a new signature, less strict than the existing defs with same name. list << item nil end - def add_macro(a_def) - case a_def.name + def add_macro(a_macro) + case a_macro.name when "inherited" - return add_hook :inherited, a_def + return add_hook :inherited, a_macro when "included" - return add_hook :included, a_def + return add_hook :included, a_macro when "extended" - return add_hook :extended, a_def + return add_hook :extended, a_macro when "method_added" - return add_hook :method_added, a_def, args_size: 1 + return add_hook :method_added, a_macro, args_size: 1 when "method_missing" - if a_def.args.size != 1 + if a_macro.args.size != 1 raise TypeException.new "macro 'method_missing' expects 1 argument (call)" end end macros = (@macros ||= {} of String => Array(Macro)) - array = (macros[a_def.name] ||= [] of Macro) - index = array.index { |existing_macro| a_def.overrides?(existing_macro) } + array = (macros[a_macro.name] ||= [] of Macro) + index = array.index { |existing_macro| a_macro.overrides?(existing_macro) } if index - a_def.doc ||= array[index].doc - array[index] = a_def + # a_macro has the same signature of an existing macro, we override it. + a_macro.doc ||= array[index].doc + array[index] = a_macro else - array.push a_def + # a_macro has a new signature, add it with the others. + array << a_macro end end - def add_hook(kind, a_def, args_size = 0) - if a_def.args.size != args_size + def add_hook(kind, a_macro, args_size = 0) + if a_macro.args.size != args_size case args_size when 0 raise TypeException.new "macro '#{kind}' must not have arguments" @@ -802,7 +808,7 @@ module Crystal end hooks = @hooks ||= [] of Hook - hooks << Hook.new(kind, a_def) + hooks << Hook.new(kind, a_macro) end def filter_by_responds_to(name)