Binary comprehension

[NelsonVides]

Trying to solve #199 by checking that binary is a subtype of the expected result type of the binary comprehension.

[codecov-io]

Codecov Report

Merging #201 into master will decrease coverage by 0.26%.
The diff coverage is 65.51%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   83.91%   83.64%   -0.27%     
==========================================
  Files          15       15              
  Lines        2381     2403      +22     
==========================================
+ Hits         1998     2010      +12     
- Misses        383      393      +10

Impacted Files	Coverage Δ
src/typechecker.erl	87.91% <65.51%> (-0.45%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b4c6b0...00351ef. Read the comment docs.

[zuiderkwast]

Awesome! Although not covering the general case, this is a clear improvement. Adding a TODO comment for covering the general case (unions containing all bitstring types) would be good IMO.

[zuiderkwast]

This covers ResTy iodata(), but what if ResTy is a union containing bitstring() or another bit type such as <<_:7, _:_*3>>? I think the general solution would be do like in the list comprehension case and create a function expect_bitstring_type.

Another solution: I think we can get rid of all the expect_ functions though if we split any union (e.g. iodata() after normalizing it) before this function is called. I'm not sure if there is a performance concern for doing this, @josefs? Personally I'm more concerned about the code complexity than the performance for these things.

[NelsonVides]

Ok, thinking about that, I came up with this other testcase, which fails for my current solution:

-spec bitstring_fun() -> <<_:7, _:_*3>> | string().
bitstring_fun() -> << <<1:N>> || N <- lists:seq(3, 12)>>.

I think I'm coming for a solution for that, with the idea of splitting the unions, but the code is getting a bit uglier at least from my perspective. I'll push it here when I have it, but maybe I could put it on a different PR so that you can choose the best solution, maybe?

[NelsonVides]

An even crazier example

-type nested() :: string() | bitstring() | binary().
-spec nested_bitstrings() -> nested() | boolean() | bitstring().
nested_bitstrings() ->
    << <<1:N>> || N <- lists:seq(3, 12)>>.

😃

[zuiderkwast]

Good test case.

[NelsonVides]

@zuiderkwast I moved that test to test/known_problems/should_fail/, I tried having a look at fixing it, but I honestly didn't know how for now.

I also changed the matching on the throws, let me know what do you think about it by now and what else can I do to improve this PR.

[zuiderkwast]

OK, so the idea is good in general, but the code needs improvements. :-)

[josefs]

It's nice to see binary comprehensions getting some love because they're almost completely forgotten right now. In order to type check them properly we need a new function expect_binary_type similar to expect_list_type, expect_tuple_type etc. It's a great little project if you want to wade in a little deeper into the code base.

[NelsonVides]

@josefs

It's nice to see binary comprehensions getting some love because they're almost completely forgotten right now. In order to type check them properly we need a new function expect_binary_type similar to expect_list_type, expect_tuple_type etc. It's a great little project if you want to wade in a little deeper into the code base.

As we've been talking with @zuiderkwast, the fact that there are all these expect_ functions seems a bit reduntant. I'm trying to implement it now that way and it feels like duplicated code everywhere, doing the union handling for each language construct. Unions should be folded earlier in the call stack, so these points don't need to handle all sorts of unions separately 😞

[josefs]

Yes, I agree that the is room for improvement in the code. Patches are most welcome. I'm afraid it's not very high on my priority list at the moment.

[zuiderkwast]

As we've been talking with @zuiderkwast, the fact that there are all these expect_ functions seems a bit reduntant. I'm trying to implement it now that way and it feels like duplicated code everywhere, doing the union handling for each language construct. Unions should be folded earlier in the call stack, so these points don't need to handle all sorts of unions separately disappointed

I agree with you @NelsonVides! I didn't mean to tell you to rewrite the code into the style of the old stuff. I just wanted you to be aware of it, so that we can refactor it as whole. Folding higher up on the call stack is exactly what I've been tying to suggest, but then we need to take the error messages into account, perhaps not simply re-throwing the one for one element in the union.

[NelsonVides]

@zuiderkwast It's all rewritten now, following the style of expect_ functions. I was aware of those from the very beginning, but I had the feeling they were a bit of an overengineering for something so simple. In any case I also accept the idea of keeping things consistent, even if the style chosen is not the best, it's better to have just one style rather than a bunch of incoherent mixes. We can fix that when we move the union folding higher up the call stack :)

[zuiderkwast]

I think this works, at least most of it. I find these expect functions a bit hard to read. Perhaps @josefs (who wrote the other expect functions) can have a look?

There are some other functions for taking types apart, such as list_view/1 (and some similar ones), that I find more elegant. It seems to serve the same purpose as expect_list_type, at least partially, but it doesn't handle unions, type variables and a bunch of other things. (Maybe one day, these can be unified...)

Also, we should really try to add specs and some description for new functions. It helps.

[Zalastax]

This pull request has unfortunately lost momentum. What are the current blocking items? How can we help get this merged?

[codecov-commenter]

Codecov Report

Patch coverage: 65.51% and project coverage change: -3.82 ⚠️

Comparison is base (e226803) 87.46% compared to head (00351ef) 83.64%.

❗ Current head 00351ef differs from pull request most recent head e44a85b. Consider uploading reports for the commit e44a85b to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   87.46%   83.64%   -3.82%     
==========================================
  Files          18       15       -3     
  Lines        2760     2403     -357     
==========================================
- Hits         2414     2010     -404     
- Misses        346      393      +47     

Impacted Files	Coverage Δ
src/typechecker.erl	87.91% <65.51%> (-2.38%)	⬇️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[erszcz]

I've rebased this on top of the current master and made sure all the tests pass - they do, together with the self-check. I'll merge it now, as otherwise it might linger forever.