Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defining abstract methods with generics fails #6762

Closed
spinscale opened this issue Sep 21, 2018 · 6 comments · Fixed by #7956
Closed

Defining abstract methods with generics fails #6762

spinscale opened this issue Sep 21, 2018 · 6 comments · Fixed by #7956

Comments

@spinscale
Copy link

spinscale commented Sep 21, 2018

I have an issue with abstract methods and generics (note, java developer speaking here, so this might be just an misunderstanding).

When defining the following abstract class

abstract class Foo(T)
  abstract def set(copy : Array(T)) 
end

I expected the implementation to be

class Bar < Foo(String)
  def set(copy : Array(String))
  end
end

This however gives a compile time error, where as using def set(copy : Array) works.

@vladfaust
Copy link
Contributor

Looks like a bug for me, because this code raises no error:

class Foo(T)
  def foo(x : T)
    {% raise NotImplementedError %}
  end
end

class Bar < Foo(String)
  def foo(x : String)
    puts x
  end
end

Bar.new.foo("foo")

@RX14
Copy link
Contributor

RX14 commented Sep 22, 2018

Definitely a bug, workaround is just to loosen the type restriction.

@vladfaust
Copy link
Contributor

vladfaust commented Dec 18, 2018

Got this bug again while creating an abstract rescue HTTP handler (https://carc.in/#/r/5tcy):

abstract class Rescuer(T)
  def call(context)
    call_next(context)
  rescue ex : T
    handle(ex)
  end

  abstract def handle(ex : T)
end

class BasicRescuer < Rescuer(Exception)
  def handle(ex : Exception)
    puts ex.inspect_with_backtrace
  end
end

BasicRescuer.new
Error in line 8: abstract `def Rescuer(T)#handle(ex : T)` must be implemented by BasicRescuer

I post it to prove that there are some real-world use-cases.

@asterite
Copy link
Member

Workaround: don't define the abstract method at all.

@vladfaust
Copy link
Contributor

vladfaust commented Dec 18, 2018

As mentioned above in #6762 (comment), defining a non-abstract method with T and then overriding with Type works (https://carc.in/#/r/5tex). But it doesn' work in this particular case (https://carc.in/#/r/5tey):

The example could be simplified, sorry about that

require "http/server"

abstract class Rescuer(T)
  include HTTP::Handler

  def call(context)
    call_next(context)
  rescue error : T
    handle(error)
  end

  def handle(error : T)
    {% raise "Must be implemented" %}
  end
end

class BasicRescuer < Rescuer(Exception)
  def handle(error : Exception)
    puts error.inspect_with_backtrace
  end
end

server = HTTP::Server.new([BasicRescuer.new]) do |context|
  context.response.print("Hello\n")
end

server.bind_tcp(5000)
puts "Listening"
server.listen
in line 9: Must be implemented

As @asterite mentioned in #7200, there might be a problem with module inclusion.

It also seems to be related to #6701.

@asterite
Copy link
Member

Just to clarify, the abstract method check doesn't work well with generic inheritance.

Please try to avoid inheriting from generic types right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants