lfe / lfe

Lisp Flavoured Erlang (LFE)
Apache License 2.0
2.33k stars 142 forks source link

Multi-variable binary generator not working with list/binary comprehensions #442

Closed dendrown closed 3 months ago

dendrown commented 2 years ago

Hello. I am having trouble using the (<= ...) construct with a list or binary comprehension when pattern matching to multiple variables. The problem occurs, for example, when recreating this bit of Erlang code from Learn You Some Erlang for Great Good in LFE:

Pixels = <<213,45,132,64,76,32,76,0,0,234,32,15>>.
<<213,45,132,64,76,32,76,0,0,234,32,15>>

RGB = [ {R,G,B} || <<R:8,G:8,B:8>> <= Pixels ].
[{213,45,132},{64,76,32},{76,0,0},{234,32,15}] 

If I just pull a series of red bytes, things work fine (though it's not particularly useful):

(set pixels #B(213 45 132 64 76 32 76 0 0 234 32 15))
#B(213 45 132 64 76 32 76 0 0 234 32 15)

(lc ((<= (r (size 8)) pixels))
    (tuple r))
(#(213) #(45) #(132) #(64) #(76) #(32) #(76) #(0) #(0) #(234) #(32) #(15))

But I can't figure out the correct syntax which allows me to add the green and blue to the list comprehension. I would expect something like this:

(lc ((<= (binary (r (size 8)) (g (size 8)) (b (size 8))) pixels))
   (tuple r g b))
** exception error: #(undefined_bittype (r (size 8)))
  in lfe_eval_bits:eval_error/1 (src/lfe_eval_bits.erl, line 299)
  in lfe_eval_bits:get_bitseg/2 (src/lfe_eval_bits.erl, line 65)
  in lfe_eval_bits:match_bitsegs/4 (src/lfe_eval_bits.erl, line 153)

or given that the one-variable version doesn't require the binary keyword, this:

(lc ((<= ((r (size 8)) (g (size 8)) (b (size 8))) pixels))
   (tuple r g b))       
** exception error: #(undefined_bittype (g (size 8)))
  in lfe_eval_bits:eval_error/1 (src/lfe_eval_bits.erl, line 299)
  in lfe_eval_bits:get_bitseg/2 (src/lfe_eval_bits.erl, line 65)
  in lfe_eval_bits:match_bitsegs/4 (src/lfe_eval_bits.erl, line 153)

I have tried many variants on this syntax, but none work. I have also looked high and low on the web for an example of a <= generator that pattern matches to more than one variable, but I have not been able to find one.

Please tell me what I am missing.

Thanks, den

oubiwann commented 2 years ago

So, we have an LFE copy of part of that tutorial somewhere

(digs it up ...)

I think this is it? Here's the colour example used for maps:

(that's not what I was thinking of)

I'll keep digging around for resources for you -- I've done stuff like this in the past, I just forget ;-)

oubiwann commented 2 years ago

I think I was wrong :-( After lots of poking around, this isn't something we covered in a tutorial. In fact, I'm feeling like full binary expressions are not supported in the qual position, only segments ... and maybe only one segment :-(

Let's see what @rvirding says ...

dendrown commented 2 years ago

Well, I was afraid you guys might say that. Thanks for checking, @oubiwann. Let's hope @rvirding surprises us.

rvirding commented 2 years ago

I will check it out and see what is going.

rvirding commented 2 years ago

@dendrown @oubiwann OK, the cause of the problem is sort of a syntax issue. So when I defined the syntax for binary generators and comprehensions I tried to optimise the syntax by only having one binary segments. This allowed me to not have the (binary ...) wrapper around the segment which made the syntax a bit more compact. But this of course restricted it to only one segment which is why @dendrown was getting the syntax errors he got, it was an incorrect segment.

What we need now is to define a way of allowing multiple segments but keep the old syntax as well. A simple and straight forward way would be to allow (binary ...) wrapper around the segments to like @dendrown experimented with. This would make it a proper bianry expression. We could keep the special case for one segment and require (binary ...) wrapper multiple segments but also allow it around segment.

So the following cases would be legal:

(list-comp ((<= (binary (r (size 8)) (g (size 8)) (b (size 8))) pixels))
   (tuple r g b))
(list-comp ((<= (r (size 8)) pixels))
    (tuple r))
(list-comp ((<= (binary (r (size 8))) pixels))
    (tuple r))

The same for expressions.

NOTE: lc and bc are exactly the same thing as list-comp and binary-comp.

oubiwann commented 2 years ago

This sounds great, @rvirding!

dendrown commented 2 years ago

Agreed. I like that it matches the generalized syntax for pattern matching on binaries, and the single-segment short form is available as a more concise (and backwards-compatible) alternative.

Now I'm really looking forward to LFE v2.1. :)

rvirding commented 2 years ago

This is now fixed in the develop branch

dendrown commented 2 years ago

Hi again. Sorry, I was trying to confirm this, but I keep getting "exception error: function lc/2 undefined" (or "list-comp/2 undefined") in the REPL. List comprehensions are working in a program, but not from the REPL.

I had this problem with the base LFE REPL back when I opened this ticket. I could get around it by using rebar3 lfe repl instead of just lfe. Now I get the undefined error using both methods (but list comprehensions still compile in my code).

rvirding commented 2 years ago

Yes, sorry for that. I now know why it doesn't work in the REPL. Originally lc/list-comp/bc/binary-comp were macro expanded to mutally recursive functions which could then either be compiled or run in the evaluator. The optimisation I did was to remove this macro expansion and translate it at compiletime to calls into the exising Erlang forms which then the Erlang compiler handled, and does a much better job. HOWEVER that means that means that these now have to be explicitly handled in the LFE evaluator, which they are not. Hance the problem with the REPL.

I will fix this.

dendrown commented 2 years ago

Thanks, @rvirding. It's interesting to understand the dynamics of why it worked in an actual program, but not in the REPL.

oubiwann commented 1 year ago

Quick update: @rvirding's working on an implementation of lc/list-comp that will add it back to the REPL for us :-) Should be in develop soon.

dendrown commented 1 year ago

That is excellent news. Thanks to you both.

oubiwann commented 11 months ago

They're back! (in the develop branch); hoping to have them in a release very soon!

rvirding commented 10 months ago

@dendrown @oubiwann Sorry for not answering this much much much earlier.

Yes, so it is now fixed and you have both list and binary comprehensions in the LFE REPL. Also the syntax for a <= binary generator is now fixed: you have to wrap the pattern in a (binary ...) irrespective of how many fields you are extracting. No way around it I'm afraid. Consistency wins!

So you now write:

(list-comp ((<= (binary (r (size 8)) (g (size 8)) (b (size 8))) pixels))
   (tuple r g b))

(list-comp ((<= (binary (r (size 8))) pixels))
    (tuple r))
dendrown commented 3 months ago

Thank you, @rvirding and @oubiwann