Compiler: correctly check abstract methods

spec/compiler/semantic/abstract_def_spec.cr

@@ -394,4 +394,165 @@ describe "Semantic: abstract def" do
       end
       )
   end
+
+  it "errors if missing return type" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo : Int32
+      end
+
+      class Bar < Foo
+        def foo
+          1
+        end
+      end
+      ),
+      "this method overrides Foo#foo() which has an explicit return type of Int32.\n\nPlease add an explicit return type (Int32 or a subtype of it) to this method as well."
+  end
+
+  it "errors if different return type" do
+    assert_error %(
+      abstract class Foo
+        abstract def foo : Int32
+      end
+
+      class Bar < Foo
+        struct Int32
+        end
+
+        def foo : Int32
+          1
+        end
+      end
+      ),
+      "this method must return Int32, which is the return type of the overridden method Foo#foo(), or a subtype of it, not Bar::Int32"
+  end
+
+  it "can return a more specific type" do
+    assert_type(%(
+      class Parent
+      end
+
+      class Child < Parent
+      end
+
+
+      abstract class Foo
+        abstract def foo : Parent
+      end
+
+      class Bar < Foo
+        def foo : Child
+          Child.new
+        end
+      end
+
+      Bar.new.foo
+      )) { types["Child"] }
+  end
+
+  it "matches instantiated generic types" do
+    semantic(%(
+      abstract class Foo(T)
+        abstract def foo(x : T)
+      end
+
+      abstract class Bar(U) < Foo(U)
+      end
+
+      class Baz < Bar(Int32)
+        def foo(x : Int32)
+        end
+      end
+    ))
+  end
+
+  it "matches generic types" do
+    semantic(%(
+      abstract class Foo(T)
+        abstract def foo(x : T)
+      end
+
+      class Bar(U) < Foo(U)
+        def foo(x : U)
+        end
+      end
+    ))
+  end
+
+  it "matches instantiated generic module" do
+    semantic(%(
+      module Foo(T)
+        abstract def foo(x : T)
+      end
+
+      class Bar
+        include Foo(Int32)
+
+        def foo(x : Int32)
+        end
+      end
+      ))
+  end
+
+  it "matches generic module" do
+    semantic(%(
+      module Foo(T)
+        abstract def foo(x : T)
+      end
+
+      class Bar(U)
+        include Foo(U)
+
+        def foo(x : U)
+        end
+      end
+      ))
+  end
+
+  it "matches generic module (a bit more complex)" do
+    semantic(%(
+      class Gen(T)
+      end
+
+      module Foo(T)
+        abstract def foo(x : Gen(T))
+      end
+
+      class Bar
+        include Foo(Int32)
+
+        def foo(x : Gen(Int32))
+        end
+      end
+      ))
+  end
+
+  it "matches generic return type" do
+    semantic(%(
+      abstract class Foo(T)
+        abstract def foo : T
+      end
+
+      class Bar < Foo(Int32)
+        def foo : Int32
+          1
+        end
+      end
+    ))
+  end
+
+  it "is missing a return type in subclass of generic subclass" do
+    assert_error %(
+        abstract class Foo(T)
+          abstract def foo : T
+        end
+
+        class Bar < Foo(Int32)
+          def foo
+          end
+        end
+      ),
+      "this method overrides Foo(T)#foo() which has an explicit return type of T.\n\nPlease add an explicit return type (Int32 or a subtype of it) to this method as well."
+  end
 end

spec/std/http/server/server_spec.cr

@@ -23,7 +23,7 @@ private class ReverseResponseOutput < IO
   def initialize(@output : IO)
   end
 
-  def write(slice : Bytes)
+  def write(slice : Bytes) : Nil
     slice.reverse_each do |byte|
       @output.write_byte(byte)
     end

spec/std/io/delimited_spec.cr

@@ -16,7 +16,7 @@ private class PartialReaderIO < IO
     read_size
   end
 
-  def write(slice : Bytes)
+  def write(slice : Bytes) : NoReturn
     raise "write"
   end
 end

spec/std/io/io_spec.cr

@@ -42,7 +42,7 @@ private class SimpleIOMemory < IO
     count
   end
 
-  def write(slice : Bytes)
+  def write(slice : Bytes) : Nil
     count = slice.size
     new_bytesize = bytesize + count
     if new_bytesize > @capacity

spec/std/io/sized_spec.cr

@@ -5,8 +5,7 @@ private class NoPeekIO < IO
     0
   end
 
-  def write(bytes : Bytes)
-    0
+  def write(bytes : Bytes) : Nil
   end
 
   def peek

src/compiler/crystal/semantic.cr

@@ -69,9 +69,15 @@ class Crystal::Program
     node, processor = @progress_tracker.stage("Semantic (type declarations)") do
       TypeDeclarationProcessor.new(self).process(node)
     end
-    @progress_tracker.stage("Semantic (abstract def check)") do
-      AbstractDefChecker.new(self).run
+
+    # TODO: remove this check a couple of versions after 0.30.0 once
+    # we are sure it's working fine for everyone
+    unless has_flag?("skip_abstract_def_check")
+      @progress_tracker.stage("Semantic (abstract def check)") do
+        AbstractDefChecker.new(self).run
+      end
     end
+
     {node, processor}
   end
 end

src/compiler/crystal/semantic/abstract_def_checker.cr

@@ -65,10 +65,11 @@ class Crystal::AbstractDefChecker
   end
 
   def check_implemented_in_subtypes(base, type, method)
-    # TODO: check generic modules
     subtypes = case type
                when NonGenericModuleType
                  type.raw_including_types
+               when GenericModuleType
+                 type.raw_including_types
                else
                  type.subclasses
                end
@@ -99,7 +100,11 @@ class Crystal::AbstractDefChecker
     type.defs.try &.each_value do |defs_with_metadata|
       defs_with_metadata.each do |def_with_metadata|
         a_def = def_with_metadata.def
-        return true if implements?(type, a_def, base, method)
+
+        if implements?(type, a_def, base, method)
+          check_return_type(type, a_def, base, method)
+          return true
+        end
       end
     end
     false
@@ -115,6 +120,16 @@ class Crystal::AbstractDefChecker
 
     return false if m1.args.size < m2.args.size
 
+    # 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
+    # T will be Int32 in something like `class Bar < Foo(Int32)` with `Foo(T)`.
+    # Then we replace all `T` in the base method with `Int32`, and just then
+    # we check if they match.
+    if t2.is_a?(GenericType)
+      generic_base = find_base_generic_instantiation(t1, t2)
+      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
@@ -138,4 +153,79 @@ class Crystal::AbstractDefChecker
 
     true
   end
+
+  def check_return_type(type : Type, method : Def, base_type : Type, base_method : Def)
+    base_return_type_node = base_method.return_type
+    return unless base_return_type_node
+
+    original_base_return_type = base_type.lookup_type(base_return_type_node)
+
+    # 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
+    # T will be Int32 in something like `class Bar < Foo(Int32)` with `Foo(T)`.
+    # Then we replace all `T` in the base method return type with `Int32`,
+    # and just then we check if they match.
+    if base_type.is_a?(GenericType)
+      generic_base = find_base_generic_instantiation(type, base_type)
+
+      replacer = ReplacePathWithTypeVar.new(base_type, generic_base)
+      base_return_type_node = base_return_type_node.clone
+      base_return_type_node.accept(replacer)
+    end
+
+    base_return_type = base_type.lookup_type(base_return_type_node)
+
+    return_type_node = method.return_type
+    unless return_type_node
+      method.raise "this method overrides #{Call.def_full_name(base_type, base_method)} which has an explicit return type of #{original_base_return_type}.\n#{@program.colorize("Please add an explicit return type (#{base_return_type} or a subtype of it) to this method as well.").yellow.bold}"
+    end
+
+    return_type = type.lookup_type(return_type_node)
+
+    unless return_type.implements?(base_return_type)
+      return_type_node.raise "this method must return #{base_return_type}, which is the return type of the overridden method #{Call.def_full_name(base_type, base_method)}, or a subtype of it, not #{return_type}"
+    end
+  end
+
+  def replace_method_arg_paths_with_type_vars(base_type : Type, method : Def, generic_type : GenericInstanceType)
+    replacer = ReplacePathWithTypeVar.new(base_type, generic_type)
+
+    method = method.clone
+    method.args.each do |arg|
+      arg.restriction.try &.accept(replacer)
+    end
+    method
+  end
+
+  def find_base_generic_instantiation(type : Type, base_type : GenericType)
+    type.ancestors.find do |t|
+      t.is_a?(GenericInstanceType) && t.generic_type == base_type
+    end.as(GenericInstanceType)
+  end
+
+  class ReplacePathWithTypeVar < Visitor
+    def initialize(@base_type : GenericType, @generic_type : GenericInstanceType)
+    end
+
+    def visit(node : Path)
+      if !node.global? && node.names.size == 1
+        # Check if it matches any of the generic type vars
+        name = node.names.first
+
+        type_var = @generic_type.type_vars[name]?
+        if type_var.is_a?(Var)
+          # Check that it's actually a type parameter on the base type
+          if @base_type.lookup_type?(node).is_a?(TypeParameter)
+            node.type = type_var.type
+          end
+        end
+      end
+
+      false
+    end
+
+    def visit(node : ASTNode)
+      true
+    end
+  end
 end

src/compiler/crystal/semantic/type_lookup.cr

@@ -80,6 +80,13 @@ class Crystal::Type
     delegate program, to: @root
 
     def lookup(node : Path)
+      # A Path might have a type set.
+      # This is at least done in AbstractDefChecker when we replace type
+      # parameters with concrete types.
+      if type = node.type?
+        return type
+      end
+
       type_var = lookup_type_var?(node)
 
       case type_var

src/deque.cr

@@ -11,7 +11,6 @@
 # [circular buffer](https://en.wikipedia.org/wiki/Circular_buffer).
 class Deque(T)
   include Indexable(T)
-  include Comparable(Deque)
 
   # This Deque is based on a circular buffer. It works like a normal array, but when an item is removed from the left
   # side, instead of shifting all the items, only the start position is shifted. This can lead to configurations like:

src/flate/writer.cr

@@ -43,7 +43,7 @@ class Flate::Writer < IO
   end
 
   # See `IO#write`.
-  def write(slice : Bytes)
+  def write(slice : Bytes) : Nil
     check_open
 
     return if slice.empty?

src/http/content.cr

@@ -78,7 +78,7 @@ module HTTP
       @io.skip(bytes_count)
     end
 
-    def write(slice : Bytes)
+    def write(slice : Bytes) : NoReturn
       raise IO::Error.new "Can't write to UnknownLengthContent"
     end
   end
@@ -213,7 +213,7 @@ module HTTP
       end
     end
 
-    def write(slice : Bytes)
+    def write(slice : Bytes) : NoReturn
       raise IO::Error.new "Can't write to ChunkedContent"
     end