Passing a block to a call on a union that uses typeof

[jbomanson]

Building the following code fails with an error.

class A
  def explain(&block : A -> _)
    typeof(block)
  end
end

class B
  def explain(&block : B -> _)
    :symbol
  end
end

a = true ? A.new : B.new
p a.explain { |x| x.to_s }
#                 ^
# BUG: already had enclosing call

This happens both with Crystal 0.23.1 [e2a1389] (2017-07-13) LLVM 3.8.1 and Crystal 0.23.0+313 [ea4187c57] (2017-10-27) LLVM: 3.9.1 on Ubuntu 16.04 LTS.

The following code does compile but the types will be wrong. There are too many As and not enough Bs. Also, the class types should have unions in them in some of the cases.

class A
  def explain(&block : A -> _)
    typeof(block)
  end
end

class B
  def explain(&block : B -> _)
    typeof(block)
  end
end

a = true ? A.new : B.new
p a.explain { |x| x }      # Proc(A, A)      : Proc(A, A):Class
p a.explain { |x| x.to_s } # Proc(A, String) : Proc(A, String):Class

a = A.new
p a.explain { |x| x }      # Proc(A, A)      : Proc(A, A):Class
p a.explain { |x| x.to_s } # Proc(A, String) : Proc(A, String):Class

b = false ? A.new : B.new
p b.explain { |x| x }      # Proc(A, A)      : Proc(A, A):Class
p b.explain { |x| x.to_s } # Proc(A, String) : Proc(A, String):Class

b = B.new
p b.explain { |x| x }      # Proc(B, B)      : Proc(B, B):Class
p b.explain { |x| x.to_s } # Proc(A, String) : Proc(A, String):Class

[z64]

Bump; We're working on an abstract caching API in discordcr/discordcr#136 that allows users to define a custom caching backend, and ran into this issue when implementing #each(&block) for Enumerable class functions.

The current workaround has been to use .as to tell the compiler the type to treat it as. I've boiled down our use case a little to this: https://carc.in/#/r/4gsy

# BUG: already has enclosing call
client.cache.each do |value|
  p value
end

# OK:
client.cache.as(MemoryCache(Int32, String)).each do |value|
  p value
end

Crystal 0.25.1 (2018-06-29)

LLVM: 6.0.0
Default target: x86_64-pc-linux-gnu

Let me know if there's any other info I can provide.

[asterite]

I checked out this is done and it seems the language can't handle multiple dispatch when captured blocks are involved (either when two methods that capture blocks are involved, or when one captures and the other one yields). Changing this is pretty complex, so for now I would advice not doing that: either yield all the time, or avoid the multiple dispatch (especially when the block type changes, as in the example above with A and B, which makes very little sense).

Also, typeof has nothing to do here, it's just that when block is mentioned, the block becomes captured.

@z64 What's your use case? Why is this a problem? Or do you run into the bug when you try to inspect the block's type using typeof?

[z64]

@asterite If you'll reference the boiled down carc.in I linked (https://carc.in/#/r/4gsy), I ran into this naturally while developing it: (not a lot of this applies to the issue at hand probably, but I'll paint the full picture as best I can)

abstract class Cache(K, V) exists so that users can have a specific (user configurable) cache backend for multiple kinds of types in our application. It is for chat software, Discord - so we cache things like chat channels and user accounts, for example.

A user might chose to store something like channels in memory, while choosing to store users in a database backend on disk.

1. abstract class Cache(K, V) describes the API contract we want to present to users. This is storing, fetching, removing, and lastly, users need a way to iterate over that data to do work across the entire cached set. We essentially describe a Hash, but abstract in implementation detail

2. By default, our client does no caching. We represent this as a NullCache instead of defaulting the ivar to nil. Each part of the cache API, it simply returns nil. Switching between cache types (or disabling it by assigning NullCache) this way requires 0 change in code.

3. MemoryCache is an implementation of Cache that wraps a Hash(K, V). So, we implement Cache's abstract def each(&block : V ->), delegating basically to @data : Hash(K, V):

def each(&block : Tuple(K, V) ->)
  @data.each do |key, value|
    yield({key, value})
  end
end

In another implementation for example, like an SQLCache, this would be going over some ResultSet of course instead of something in memory.

4. As mentioned, our default cache type is NullCache before it is configured by users to something else, i.e., MemoryCache or their own SQLCache, or RedisCache, etc, that implements our contract. This is what will create the union of NullCache | MemoryCache, which raises the BUG unless I use .as(MemoryCache).some_enumerable_method

I hope that is clear enough..

I realize this is painted with generics, which I know is very much a weak point right now, but sans for this bug/behavior, it results in a very powerful way for our users to control how our client stores data per-type, which is particularly important for large scale deployments of our client.

[asterite]

@z64 Just replace:

@cache.each_value(&block)

with:

@cache.each_value do |value|
  yield value
end

and it will work. That way you don't capture the block, the block gets inlined, it's more efficient, and it actually compiles :-)

[z64]

@asterite Thank you so much! That works. ❤️

I guess I didn't realize that explicitly writing def foo(&block) wasn't the same as "implicitly" just having yield in the method body. I had just written out the method like that to match the abstract def requirement. Noted! 😄

[asterite]

It's more or less explained here: https://crystal-lang.org/docs/syntax_and_semantics/block_forwarding.html

It's of course no excuse for the bug :-)

[RX14]

If we're going to disallow this, we should have a proper error message.

[asterite]

No, it's a bug but fixing it is hard.

[jwoertink]

I think I'm getting this same bug. I found a way around it for now, but I get BUG: already had enclosing call in my app. I tried to come up with a small reproducible sample, but it's just too many moving parts. I'll do my best to explain the setup though.

Using crystal 0.26.1 with Lucky, and using clear.

This is the code that throws the error:

movie.actors.each do |actor|
  link(actor.name, to: path_for_actor(actor))
end

Where movie is either a Clear::Model or something that "acts" like one called NullMovie, and actors is either a Clear::Model::Collection or a [] of Actor.

To fix this, I changed the actors method

class NullMovie
def actors
- [] of Actor
+ Actor::Collection.new
end
end

This actually makes more sense anyway since this makes actors the same type in both cases. I just assumed it would work the first time since both methods respond to #each. I know it's not much to go on, and it's been stated this is a tough bug, but in case anyone else runs in to this hopefully it will help point to a solution.

[asterite]

I think the correct way to fix this would be to have one block instance per def instantiation. That means that for the multi-dispatch case one block would be typed with A as an argument, another block would be typed with B as an argument... though that complicates what happens with local variables outside of the block. Then the resulting type is the type of all blocks.

Alternatively we could just forbid passing a block if it's going to have multiple types, because the block is essentially transformed into a Proc and you generally want to represent it as a single Proc, not as a union of possibly multiple procs.