Crystal: Iterable of tuple, methods select/reject/map do not unpack tuple

[yourpalal]

Very similar to #3419, which was specific to the sort_by method. There are many other methods which also cannot unpack tuples automatically. Eg.

[{1,2}].each.select { |x,y| y > x }

will cause a compile error.

Note for users looking at this bug you can work around this limitation by manually unpacking the args like so:

[{1,2}].each.select { |(x, y)| y > x }

I don't think this necessarily has to be fixed right this instant, but this is IMO a big usability bug. There's no clear way to inspect the type signature of a method to tell if it can unpack tuples for you. There's no clear pattern to why some methods will unpack tuples and why some won't. This is not included in the documentation. Finally, the error Error in line 1: wrong number of block arguments (given 2, expected 1) is kind of confusing because the block would be called with 1 argument, while it expects 2.

Perhaps this would be a better error message: Error in line 1: wrong number of block arguments (given block accepting 2 arguments, expected block accepting 1).

Edited: Originally the code examples above used [{1, 2}].select without the intermediate each, meaning they were calling the select method defined by Enumerable, which is not affected by this issue.

[asterite]

It seems to be working fine: https://play.crystal-lang.org/#/r/6ruj

Also in 0.2.8.0

Maybe you meant this?

# Note the `each`, which will make it an Iterator
[{1,2}].each.select { |x,y| y > x }

If that's the case then yes, this bug/feature probably makes sense though it's a bit hard to implement.

[asterite]

There's no clear pattern to why some methods will unpack tuples and why some won't

There is. Right now if a method captures a block then you can't also unpack tuples. If the method doesn't capture the block then you can unpack them.

I'm not saying this is good, I'm just pointing out when the bug happens.

[yourpalal]

Sorry yes, I meant .each.select .

I understand that there's a technical reason for this difference, but, as I think you understand, this doesn't really make any sense from the perspective of the user. Whether the block is captured or not is an implementation detail that leaks out of the method and into the calling context. The way a user writes their calling code has no effect on how the block is used, which means you cannot tell what's happening just by reading the calling code.

Overall, crystal is super nice and it's very usable and readable (good job and thank you!), complexity seems to be generally easy to keep low. I think this is one spot where this falters, and there's some hidden complexity that bubbles up into user code in a confusing way.

I believe the chasm between captured/non-captured blocks will also increase maintenance burden over time, as changing the internal implementation of a method changes what users can do with it. I'm still not sure if this difference is captured in the type system. Consider that you cannot return from a captured block. If a Array#each is changed to start capturing a block, the following code will almost be valid, but will no longer work:

some_array.each do |x|
  return x if x > 3
end

Anyway, for now I think it would be worth switching the methods of Iterable to use yield but at the moment this means they can never be switched back to capturing the given block.

[asterite]

I'm sorry if I wasn't clear before:

• I agree with you that this is something that should be fixed
• The fix is something hard to do and low priority right now (you can always use |(x, y)| to accomplish what you want)
• Making the methods of Iterator use yield is impossible
• Array#each will never be changed to capture the block

[yourpalal]

@asterite sorry we got our wires crossed a bit! I just wanted to elaborate for you and anyone else looking into the issue the reasons why I think this is a very important issue.

Good point that Iterator can't use yield, as the block has to be captured in order to enable lazy evaluation.

I'm not expecting Array#each to change, but I do think that it would be very easy for someone to change a method in this way without recognizing the resulting change in semantics. I think this issue is something that could lead to accidental breakage in the standard library or user code. Thankfully, in the event it does happen, the issue would be caught by the compiler.

@asterite thanks for your time!

[konovod]

@yourpalal non-capturing blocks have different semantic and it is not an issue - it is a feature 😄
Non-capturing blocks are better in all cases where they are possible (better performance, you can use return etc) so it is unlikely that some method would change from non-capturing to capturing.

It could happen (not with Array#each, but with some hypothetical method) only if using non-capturing block become impossible with it. I don't think someone could accidentally change yield to block.call.

[yourpalal]

@konovod Non-capturing blocks are definitely nicer, especially for the caller. To me the fact that they look exactly the same in the calling code is an issue, but it seems I'm in the minority here and that's fine! :smile:

I guess if I could get just one small improvement in this area it would be the compiler errors.

Error in line 1: wrong number of block arguments (given 2, expected 1)

could be replaced with

Error in line 1: wrong number of block arguments (given block accepting 2 arguments, expected block accepting 1)

or even, when it's relevant (when a Tuple is being passed to the block)

Error in line 1: wrong number of block arguments (given block accepting 2 arguments, expected block accepting 1). Tuples are not automatically unpacked when given to captured blocks, consider using |(a, b)| to manually unpack them.

or something like that.

[asterite]

To me the fact that they look exactly the same in the calling code is an issue

Yes, if this used a different semantic then the whole language also becomes much easier to implement because you can know which variables are closured, lexically. I think there's still time to change this but it can probably be confusing to have two syntaxes to specify block or proc, plus there's the &. notation, etc.