Check abstract def implementations with splats, default values and ke…

…yword arguments (#9585)

spec/compiler/semantic/abstract_def_spec.cr

@@ -674,4 +674,207 @@ describe "Semantic: abstract def" do
       end
     ))
   end
+
+  it "doesn't error if implementation have default value" do
+    semantic %(
+      abstract class Foo
+        abstract def foo(x)
+      end
+
+      class Bar < Foo
+        def foo(x = 1)
+        end
+      end
+      )
+  end
+
+  it "errors if implementation doesn't have default value" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(x = 1)
+      end
+
+      class Bar < Foo
+        def foo(x)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(x = 1)` must be implemented by Bar"
+  end
+
+  it "errors if implementation doesn't have the same default value" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(x = 1)
+      end
+
+      class Bar < Foo
+        def foo(x = 2)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(x = 1)` must be implemented by Bar"
+  end
+
+  it "errors if implementation doesn't have keyword arguments" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(*, x)
+      end
+
+      class Bar < Foo
+        def foo(a = 0, b = 0)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(*, x)` must be implemented by Bar"
+  end
+
+  it "errors if implementation doesn't have a keyword argument" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(*, x)
+      end
+
+      class Bar < Foo
+        def foo(*, y)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(*, x)` must be implemented by Bar"
+  end
+
+  it "doesn't error if implementation matches keyword argument" do
+    semantic %(
+      abstract class Foo
+        abstract def foo(*, x)
+      end
+
+      class Bar < Foo
+        def foo(*, x)
+        end
+      end
+      )
+  end
+
+  it "errors if implementation doesn't match keyword argument type" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(*, x : Int32)
+      end
+
+      class Bar < Foo
+        def foo(*, x : String)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(*, x : Int32)` must be implemented by Bar"
+  end
+
+  it "doesn't error if implementation have keyword arguments in different order" do
+    semantic %(
+      abstract class Foo
+        abstract def foo(*, x : Int32, y : String)
+      end
+
+      class Bar < Foo
+        def foo(*, y : String, x : Int32)
+        end
+      end
+      )
+  end
+
+  it "errors if implementation has more keyword arguments" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(*, x)
+      end
+
+      class Bar < Foo
+        def foo(*, x, y)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(*, x)` must be implemented by Bar"
+  end
+
+  it "doesn't error if implementation has more keyword arguments with default values" do
+    semantic %(
+      abstract class Foo
+        abstract def foo(*, x)
+      end
+
+      class Bar < Foo
+        def foo(*, x, y = 1)
+        end
+      end
+      )
+  end
+
+  it "errors if implementation doesn't have a splat" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(*args)
+      end
+
+      class Bar < Foo
+        def foo(x = 1)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(*args)` must be implemented by Bar"
+  end
+
+  it "errors if implementation doesn't match splat type" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo(*args : Int32)
+      end
+
+      class Bar < Foo
+        def foo(*args : String)
+        end
+      end
+      ),
+      "abstract `def Foo#foo(*args : Int32)` must be implemented by Bar"
+  end
+
+  it "doesn't error with splat" do
+    semantic %(
+      abstract class Foo
+        abstract def foo(*args)
+      end
+
+      class Bar < Foo
+        def foo(*args)
+        end
+      end
+    )
+  end
+
+  it "doesn't error with splat and args with default value" do
+    semantic %(
+      abstract class Foo
+        abstract def foo(*args)
+      end
+
+      class Bar < Foo
+        def foo(a = 1, *args)
+        end
+      end
+    )
+  end
+
+  it "allows arguments to be collapsed into splat" do
+    semantic %(
+      abstract class Foo
+        abstract def foo(a : Int32, b : String)
+      end
+
+      class Bar < Foo
+        def foo(*args : Int32 | String)
+        end
+      end
+    )
+  end
 end

src/compiler/crystal/semantic/abstract_def_checker.cr

@@ -22,8 +22,6 @@
 #   def foo(x); end
 # end
 # ```
-#
-# TODO: the check currently ignores methods that involve splats.
 class Crystal::AbstractDefChecker
   def initialize(@program : Program)
     @all_checked = Set(Type).new
@@ -48,9 +46,6 @@ class Crystal::AbstractDefChecker
         defs_with_metadata.each do |def_with_metadata|
           a_def = def_with_metadata.def
           if a_def.abstract?
-            # TODO: for now we skip methods with splats and default arguments
-            next if a_def.splat_index || a_def.args.any? &.default_value
-
             check_implemented_in_subtypes(type, a_def)
           end
         end
@@ -120,10 +115,8 @@ class Crystal::AbstractDefChecker
     return false unless m1.name == m2.name
     return false unless m1.yields == m2.yields
 
-    # TODO: for now we consider that if there's a splat, the method is implemented
-    return true if m1.splat_index
-
-    return false if m1.args.size < m2.args.size
+    m1_args, m1_kargs = def_arg_ranges(m1)
+    m2_args, m2_kargs = def_arg_ranges(m2)
 
     # If the base type is a generic type, we find the generic instantiation of
     # t1 for it. This will have a mapping of type vars to types, for example
@@ -137,25 +130,108 @@ class Crystal::AbstractDefChecker
       m2 = replace_method_arg_paths_with_type_vars(t2, m2, generic_base)
     end
 
-    m2.args.zip(m1.args) do |a2, a1|
-      r1 = a1.restriction
-      r2 = a2.restriction
-      if r2 && r1 && r1 != r2
-        # Check if a1.restriction is contravariant with a2.restriction
-        begin
-          rt1 = t1.lookup_type(r1)
-          rt2 = t2.lookup_type(r2)
-          return false unless rt2.covariant?(rt1)
-        rescue Crystal::TypeException
-          # Ignore if we can't find a type (assume the method is implemented)
-          next
-        end
+    # First check positional arguments
+    # The following algorithm walk through the arguments in the abstract
+    # method and the implementation at the same time, until a splat argument is found
+    # or the end of the positional argument list is reached in both lists.
+    # The table below resumes the allowed cases (OK) and rejected (x) for each combination
+    # of the argument in the implementation (a1) and the abstract def (a2).
+    # `an = Dn` represents an argument with a default value. `-` represents that
+    # no more arguments are available to compare.
+    # Allowed cases are then verified that they have compatible default value
+    # and type restrictions.
+    #
+    #         |  a2  | a2 = D2 | *a2 |  -  |
+    # a1      |  OK  |   x     | x   |  x  |
+    # a1 = D1 |  OK  |   OK    | OK  |  OK |
+    # *a1     |  OK  |   x     | OK  |  OK |
+    # -       |  x   |   x     | x   |  OK |
+    i1 = i2 = 0
+    loop do
+      a1 = i1 <= m1_args ? m1.args[i1] : nil
+      a2 = i2 <= m2_args ? m2.args[i2] : nil
+
+      case
+      when !a1
+        # No more arguments in the implementation
+        return false unless !a2
+      when i1 == m1.splat_index
+        # The argument in the implementation is a splat
+        return false if a2 && a2.default_value
+      when !a1.default_value
+        # The argument in the implementation doesn't have a default value
+        return false if !a2 || a2.default_value || i2 == m2.splat_index
+      end
+
+      if a1 && a2
+        return false unless check_arg(t1, a1, t2, a2)
+      end
+
+      # Move next, unless we're on the splat already or at the end of the arguments
+      done = true
+      unless i1 == m1.splat_index || a1 == nil
+        i1 += 1
+        done = false
+      end
+      unless i2 == m2.splat_index || a2 == nil
+        i2 += 1
+        done = false
+      end
+      break if done
+    end
+
+    # Index keyword arguments
+    kargs =
+      m1_kargs.to_h do |i|
+        a1 = m1.args[i]
+        {a1.name, a1}
+      end
+
+    # Check keyword arguments
+    m2_kargs.each do |i|
+      a2 = m2.args[i]
+      a1 = kargs.delete(a2.name)
+      return false unless a1
+      return false unless check_arg(t1, a1, t2, a2)
+    end
+
+    # Remaining keyword arguments must have a default value
+    kargs.each_value do |a1|
+      return false unless a1.default_value
+    end
+
+    true
+  end
+
+  private def def_arg_ranges(method : Def)
+    if splat = method.splat_index
+      if method.args[splat].name.size == 0
+        {splat - 1, (splat + 1...method.args.size)}
+      else
+        {splat, (splat + 1...method.args.size)}
       end
+    else
+      {method.args.size - 1, (0...0)}
+    end
+  end
+
+  def check_arg(t1 : Type, a1 : Arg, t2 : Type, a2 : Arg)
+    if a2.default_value
+      return false unless a1.default_value == a2.default_value
     end
 
-    # If the method has more arguments, but default values for them, it implements it
-    if m1.args.size > m2.args.size
-      return false unless m1.args[m2.args.size].default_value
+    r1 = a1.restriction
+    r2 = a2.restriction
+    if r2 && r1 && r1 != r2
+      # Check if a1.restriction is contravariant with a2.restriction
+      begin
+        rt1 = t1.lookup_type(r1)
+        rt2 = t2.lookup_type(r2)
+        return false unless rt2.covariant?(rt1)
+      rescue Crystal::TypeException
+        # Ignore if we can't find a type (assume the method is implemented)
+        return true
+      end
     end
 
     true