crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.42k stars 1.62k forks source link

Warning with delegate and abstract method #8250

Open kostya opened 5 years ago

kostya commented 5 years ago

is this ok?

abstract class A
  abstract def bla : Int32
end

class Bla
  def bla : Int32
    1
  end
end

class B < A
  def initialize
    @bla = Bla.new
  end

  delegate :bla, to: @bla
end

p B.new.bla
In 1.cr:16:3

 16 | delegate :bla, to: @bla
      ^
Warning: expanding macro

There was a problem expanding macro 'delegate'

Called macro defined in /usr/local/Cellar/crystal/0.31.0_1/src/object.cr:1208:3

 1208 | macro delegate(*methods, to object)

Which expanded to:

    1 |
    2 |
 >  3 |         def bla(*args, **options)
    4 |           @bla.bla(*args, **options)
    5 |         end
    6 |
    7 |
    8 |           def bla(*args, **options)
    9 |             @bla.bla(*args, **options) do |*yield_args|
   10 |               yield *yield_args
   11 |             end
   12 |           end
   13 |
   14 |
   15 |
   16 |
Warning: this method overrides A#bla() which has an explicit return type of Int32.

Please add an explicit return type (Int32 or a subtype of it) to this method as well.

The above warning will become an error in a future Crystal version.

A total of 1 warnings were found.
asterite commented 5 years ago

Another case that hints that abstract methods in this language should maybe disappear

bcardiff commented 5 years ago

The same will happen for any macro that expands a method. They might need a way to inject the return type in order to comply with the current abstract method check.

A way to unlock the delegate macro is to allow passing the returning type.

delegate :bla, to: @bla, returning: Int32
delegate bla : Int32, to: @bla

# NOTE: proposal / not working example

But it takes some of the fun.

The property macro does allow return types already.

As I mentioned at https://github.com/crystal-lang/crystal/issues/8232#issuecomment-535115398 if the check is reframed as an after check to enforce that the contract of the abstract method is hold then this issue could be resolved without changes to the delegate macro.

asterite commented 5 years ago

Maybe an after check could work. The problem is that when I tried to do that the compiler would already complain of the type mismatch in the regular type inference phase, or complain about the missing method. In summary, the compiler will complain, it's just that the error message will be slightly different.

bcardiff commented 5 years ago

I understand that the compiler will probably complain before a delayed abstract check. If the offending override is used in a way it bothers. The explicit return type helped to catch this earlier.

Maybe the relaxation is that if there is an explicit return type it needs to match the abstract def. That way the generated method that will probably work by construction will not need a workaround to annotate a return type.

A compiler tool could be in charge of suggesting the explicit return types.

(throwing ideas to find the sweet spot of freedom and contracts here)