Semantic: Allow macro overload on named args (#5808)

* Add comments in add_def & add_macro * Fix var name: a_def => a_macro * Allow macro overloads based on named arguments names * Override when both don't have named args * Use obvious types for tests & Apply morgan's on unless vs if * fixup! Use obvious types for tests & Apply morgan's on unless vs if * Fix format
crystal-lang · Apr 14, 2018 · a327907 · a327907
1 parent 27f8b5d
commit a327907
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 21 deletions.
diff --git a/spec/compiler/semantic/macro_overload_spec.cr 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
@@ -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
@@ -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)