Crystal: Regression in 0.35: Hash#each with block receiving tuple

[Exilor]

These could be two separate issues but they strike me as related.

EDIT: First issue was separated into #9451 (@straight-shoota)

The second involves not yielding a tuple when using the delegate macro on each:

class HashWrapper(K, V)
  include Enumerable({K, V})

  @hash = {} of K => V

  delegate each, to: @hash
end

HashWrapper(Int32, Int32).new.to_a

outputs:

In /usr/share/crystal/src/enumerable.cr:1544:20

 1544 | each { |e| ary << e }
                       ^-
Error: no overload matches 'Array(Tuple(Int32, Int32))#<<' with type Int32

Both worked in 0.34 and I didn't see anything in the changelog related to that so didn't seem to be changed on purpose.

[jhass]

Probably #9208 /cc @MakeNowJust

[asterite]

Oh, look at that! I wasn't convinced about a change, and it turned out it triggers a regression!

https://github.com/crystal-lang/crystal/pull/8887#pullrequestreview-389918770

See why I'm against changing things that add no benefit and can risk breaking code?

[waj]

I just did a bisect. The first issue started to happen on this commit: https://github.com/crystal-lang/crystal/commit/15e6e28ad9155c12b4be6f03a94aff5c0d85ddb0

I'll bisect the second one now.

[asterite]

@waj the second one is #8887

[straight-shoota]

Since these are completely separate regressions, they should be tracked in different issues. I'll open a new issue for the first part.

[straight-shoota]

@asterite You might be right, but to me this shows a problem in the semantics of unwrapping a yielded tuple.

Between yield 1, 2 and yield({1, 2}) there's no difference when yielding to a block with two block arguments. But when the block has only one argument (or, in case of #delegate, a splat argument), they behave differently. That's already unexpected because usually you can simply omit trailing block arguments if you don't need them.

For this example, the fix would probably be to let the splat argument unwrap the tuple in the same way as when a tuple is unwrapped as multiple block arguments.

[asterite]

I know. All I'm saying is good luck fixing it. I think the type annotation should be reverted if you want to fix that before 1.0 :-)

[asterite]

Note that this has nothing to do with yielding individual arguments or a tuple. The problem is the type restriction: it makes something want to be that type, then it fails. That's the bug. It involves tuples, splatting on the block side and splatting on the yield side. The code is very complex. If you can fix it, then great! But I don't think anyone knows how this works (I knew but now I don't).

[asterite]

Reduced:

def foo(& : Tuple(Int32, Int32) ->)
  yield({1, 2})
end

def bar
  foo do |*x|
    yield *x
  end
end

t = uninitialized {Int32, Int32}
bar do |x|
  t = x
end

Remove the type restriction, then it works again.

^ In case someone wants to tackle this...

[asterite]

Relevant code is in Call#match_block_arg and YieldBlockBinder

[asterite]

What I mean is that it's easier to revert those two PRs then to fix them. They shouldn't block 1.0. Specially when they are mostly reactors or just docs variations without affecting how code is compiled. Well, maybe the first one is easy to fix, or it should be easier.

[straight-shoota]

Yes, we can simply revert that part of the PR. But it's not a real fix. It only treats a symptom of a deeper problem that can cause other issues as well.

[asterite]

Yes, and everyone can still use the code that worked before 0.34.0 in the next release, and many other releases as long as we don't put that restriction back. But when we do, we can make sure we do it after we fix this issue.

[bcardiff]

Can we close this issue and create a follow up to track how to make https://github.com/crystal-lang/crystal/issues/9450#issuecomment-641609520 pass? The regression itself is fixed.

[asterite]

Created #9524