Crystal: Proc with Nil return type causes other procs to always return nil

[Blacksmoke16]

Started happening after #7527.

Code:

abstract struct Action; end

record RouteAction(A) < Action, action : A

class Handler
  getter actions : Array(Action) = [] of Action

  def initialize
    action1_proc = ->(val : Int32) do
      nil
    end
    action1 = RouteAction(Proc(Int32, Nil)).new action1_proc

    actions << action1

    action2_proc = ->(val : Int32) do
      val
    end
    action2 = RouteAction(Proc(Int32, Int32)).new action2_proc

    actions << action2
  end
end

handler = Handler.new

pp handler.actions[0].action.call 123
pp handler.actions[1].action.call 123

https://play.crystal-lang.org/#/r/6ram

Result before (0.27.2):

nil
123

Result after (0.28.0):

nil
nil

Currently this has totally broken Athena for anyone who defined a route action with a return type of Nil.

[asterite]

Yeah, my solution was probably a mistake and should be reverted.

[Blacksmoke16]

Or at least a fix that doesn't cause _ALL_ procs to return nil if one has a Nil return type restriction would be greatly appreciated. However I can't speak for if this is even doable...

I can can fix Athena for now, but currently this makes it impossible to have a Nil return type, or even a union that includes Nil; which, if one was defined, would break all other routes.

EDIT: Managed a fix that allows for Nil return types (mapping Nil to another private type). So this is less of a big deal to me now. Still seems like its unintended behavior however.

[bcardiff]

Reduced

proc1 = ->(val : Int32) { nil }
proc2 = ->(val : Int32) { val }
procs = [proc1, proc2]
pp! procs[0].call(42), typeof(procs[0].call(42))
pp! procs[1].call(42), typeof(procs[1].call(42))

# 0.27.2
procs[0].call(42)         # => nil
typeof(procs[0].call(42)) # => (Int32 | Nil)
procs[1].call(42)         # => 42
typeof(procs[1].call(42)) # => (Int32 | Nil)

# 0.28.0
procs[0].call(42)         # => nil
typeof(procs[0].call(42)) # => Nil
procs[1].call(42)         # => nil
typeof(procs[1].call(42)) # => Nil

The PR was maybe to strict. It is sound to think that since _one_ proc returns Nil, unless you know better than the compiler, it is safe to force you know nothing of any call. The alternative is to build the union of the result.

The other goal of the PR was to allow Proc(T, R) to be used as a Proc(T, Nil).

The following code is invalid in 0.27.2 but valid in 0.28.0 and we want to keep it that way.

class ProcNil
  getter proc : Proc(Int32, Nil)

  def initialize
    @proc = ->(val : Int32) { val }
  end
end

pp! ProcNil.new.proc.call(42), typeof(ProcNil.new.proc.call(42))

# 0.27.2
in foo.ignoreme.cr:11: instance variable '@proc' of ProcNil must be Proc(Int32, Nil), not Proc(Int32, Int32)

# 0.28.0
ProcNil.new.proc.call(42)         # => nil
typeof(ProcNil.new.proc.call(42)) # => Nil

[asterite]

In my opinion only the last snippet should have been fixed but it broke other things. The last snippet is not very important, you can just add nil at the of it to make it work. It's more important to retain unions of procs untouched. So I'd revert my PR.

[bcardiff]

The last snippet is not very important, you can just add nil at the of it to make it work

Not when the Proc is a value passed around. The workaround up until now was to make an explicit wrapper, though.

[asterite]

I think it's better to have a way to workaround this problem. Right now with my PR merged whenever you have two procs, one returning T and one returning Nil, the resulting type is Nil and there's no workaround you can do to fix that. That's why I think reverting my PR is better.

To fix the other problem there are some things that need to change in the compiler but they are much harder to do.

[asterite]

Or maybe I can find a way to make both scenarios work. Just me a day or two to try something.

[asterite]

Hmm... it's a bit hard. I feel like I'm constantly monkey patching the type system to make things work. For now I'll stop touching the compiler.

[RX14]

I'm fine reverting #7527. We could make all procs explicitly castable to procs with the same arguments but nil return type which would ease some pain. I'm not sure @bcardiff's second example should work. Introducing all these special rules into the type system makes things more complex for everybody.

[paulcsmith]

Not sure what needs to be done for this since I am by no means a pro at Crystal internals 🤣, but I'd like to just note that I really love and rely on the Proc(Nil) feature added in #7527. I definitely agree the bug should be fixed, but would be pretty 😔 if Proc(Nil) from #7527 were reverted with no replacement.

If it helps, I can put a bountysource up for this. I have not run into this bug myself, but would be happy to help in that way if interested