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

Compiler: fix abstract def check regarding generic ancestor lookup #8098

Merged
merged 2 commits into from
Sep 4, 2019

Conversation

asterite
Copy link
Member

Fixes #8096

Thank you @exilor ! Thanks to your bug report I was able to understand why other (rare) cases were failing too.

The code is this:

module ICallable(T)
  abstract def call(foo : Proc(T))
end

module Caller
  extend self
  extend ICallable(Nil)

  def call(foo : Proc(Nil))
  end
end

And the compiler will check that ICallable(T)#call is implemented by all concrete subtype. One such subtype is Caller:Module (where class methods live). Does it provide a call method? No. Then the compiler goes and check whether any ancestor of Caller:Module provides it. Because it extends self it checks whether Caller (where instance methods live) provides it. And it does! But we must check whether that matches the method in the generic type. For that the compiler will try to find the generic instantiation of that module. That is, because we are checking a subtype of ICallable(T) then at some point there must have been some include ICallable(X) in the chain, to know what type X is. The problem was that the compiler would start the search from the current ancestor, in this case Caller, and not from the main type we were checking in the first place (Caller:Module).

It's a bit confusing. That's why I also added a similar spec which is a bit simpler:

module ICallable(T)
  abstract def call(foo : T)
end

module Moo
  def call(foo : Int32)
  end
end

module Caller
  extend ICallable(Int32)
  extend Moo
end

Here the compiler again will check whether Caller:Module implements call. It will find that Moo has it and then it will try to find the include/extend ICallable, but it won't find starting from Moo, it will find it starting from Caller:Module. Well, at least in this case, in other cases it might happen that Moo is the one that is including the other module.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Aug 18, 2019
@asterite
Copy link
Member Author

Also undoes #8035 because this fix is the right solution to the problem, the other one was not.

@RX14 RX14 added this to the 0.31.0 milestone Sep 4, 2019
@RX14 RX14 merged commit c83d498 into crystal-lang:master Sep 4, 2019
@asterite
Copy link
Member Author

asterite commented Sep 4, 2019

@RX14 Thanks! 🙏

dnamsons pushed a commit to dnamsons/crystal that referenced this pull request Jan 10, 2020
…rystal-lang#8098)

* Compiler: fix abstract def check regarding generic ancestor lookup

* Compiler: remove now dead code regarding abstract def check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abstract method implemented via extend self not considered to be implemented in some circumstances
2 participants