asterite
commented
9 years ago As a workaround for now you can move the alias below the SmtpSession class.
Open russolsen opened 9 years ago
asterite
commented
9 years ago As a workaround for now you can move the alias below the SmtpSession class.
russolsen
commented
9 years ago Thanks! That's very helpful.
jhass
commented
9 years ago class A
end
class B < A
end
module Foo
alias Callback = A ->
@@callbacks = Hash(String, Callback).new
def self.add(name, &block : Callback)
@@callbacks[name] = block
end
def self.call
@@callbacks.each_value(&.call(B.new))
end
end
Foo.add("foo") do |a|
puts "hi"
end
Foo.callI don't think that's helping in all cases.
asterite
commented
9 years ago Hmmm... yes. We can workaround that too, but it's better if we solve this issue "the right away". We'll take a look at it with @waj.
asterite
commented
9 years ago I fixed #1347. I'll still leave this open because order of declaration shouldn't matter, it should always work. This will need a bit of rethinking, but at least we have more specs and cases to test against.
rmosolgo
commented
9 years ago I have a really similar case: https://gist.github.com/rmosolgo/1f96e96d73a60b37bfcb
In my case the problem class was abstract, and adding abstract class InputObject "fixed" the issue.
Should I open a new issue?
asterite
commented
6 years ago This still happens if the alias is used in the top-level. This makes the compiler solve it for possible macro resolution (which also causes #6344 to behave differently depending on whether it's used or not at the top-level). Reproducible code:
class Session; end
alias CmdHandler = Proc(Session, String, Nil)
CmdHandler.class
class Session
def initialize(&h : CmdHandler)
@h = h
end
def handle(cmd : String)
@h.call(self, cmd)
end
end
class SmtpSession < Session
end
session = SmtpSession.new do |session, cmd|
puts "handling cmd for #{session}: #{cmd}"
end
session.handle("hello")
vlazar
commented
5 years ago Seem like this can be closed?
The original code on top seem to work https://github.com/crystal-lang/crystal/issues/1346#issue-104253186 in Crystal 0.31.1
with these modifications:
class Session;end
alias CmdHandler = Proc(Session, String, Nil)
class Session
- def initialize(&h: CmdHandler)
+ def initialize(&h : CmdHandler)
@h = h
end
- def handle(cmd: String)
+ def handle(cmd : String)
@h.call(self, cmd)
end
class SmtpSession < Session
end
session = SmtpSession.new do |session, cmd|
puts "handling cmd for #{session}: #{cmd}"
end
session.handle("hello")
endProduces
$ crystal e.cr
handling cmd for #<SmtpSession:0x104abce80>: helloEnv
$ crystal --version
Crystal 0.31.1 (2019-10-02)
LLVM: 8.0.1
Default target: x86_64-apple-macosxHowever Ary's code in https://github.com/crystal-lang/crystal/issues/1346#issuecomment-406468683 gives this:
$ crystal e.cr
Showing last frame. Use --error-trace for full trace.
In e.cr:8:3
8 | def initialize(&h : CmdHandler)
^---------
Error: expected block argument's argument #1 to be Session, not Session+
konovod
commented
4 years ago Error is actually true, it works if you make it Session+
Actually, Proc(Session+, String, Nil) fails to parse, but Proc((Session | Session1), String, Nil) works https://carc.in/#/r/7wpe
asterite
commented
2 years ago Regarding https://github.com/crystal-lang/crystal/issues/1346#issuecomment-406468683
Maybe one way to fix this is by not caching the resolved alias inside the alias type when trying to do macro expansion the first time. The way it works now is like this:
Session class is definedCmdHandler is defined but not yet resolvedCmdHandler.class is found, we resolve the alias. We find that it's Proc(Session, String, Nil).Because Session doesn't have subclasses, it's not a virtual type. We store this resolved alias inside the alias type itself. Next time the alias' type is checked, regardless of whether Session had a sublcass later on, the alias' resolved type won't be virtual (it won't be Proc(Session+, ...))
So instead of remembering that the alias resolves to that, in this first pass, we could try not caching that information. Later on when the alias is checked again, after Session had a subclass, it will resolve to the correct value.
An alternative is to stop having this distinction between virtual and non virtual types and just consider all types as potentially virtual.
Either of those changes are pretty tricky to implement.
beta-ziliani
commented
2 years ago Following the Crystal approach of making virtual types if necessary, I would expect the same here: if you only use (by some definition of use) the parent class, then there's no need to virtualize it. I guess this is compatible to the the two-passes you're referring to, right? The alternative you're suggesting is to do the opposite, always consider a type as virtual, only to tighten it to not-virtual later as an optimization?
asterite
commented
2 years ago if you only use (by some definition of use) the parent class, then there's no need to virtualize it. I guess this is compatible to the the two-passes you're referring to, right?
The problem is that macro expansion reaches Proc(Session, ...) before it knows Session has subclasses. That's why it's wrong for Session to not be virtual there. And why I also suggest not eagerly making that distinction.
The alternative you're suggesting is to do the opposite, always consider a type as virtual, only to tighten it to not-virtual later as an optimization?
Exactly. Though I think it's probably a breaking change. For example:
class Foo
def foo
1
end
end
class Bar < Foo
def foo
'a'
end
end
foo = Foo.new
foo.foo + 1 The above works fine because the type of Foo.new returns Foo, not "Foo or any of its subclasses". If we change that, the code will break, because a call to foo.foo will do a dispatch over Foo | Bar, even though in practice it's never the case that foo will be Bar.
The following code crashes the compiler:
Details: