Add support for comprehensions with conditionals

[digital-carver]

Attempt to fix #129

Comprehension support works by parsing the underlying generators, and generators with if conditions have filter as their Expr form's second argument. This previously wasn't supported, as the generator parsing code expected the for iteration's = to be the second arg instead.

This change adds support for the case of generators with filters, and adds relevant tests.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.82%. Comparing base (fe89ed1) to head (e079107).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   78.98%   79.82%   +0.83%     
==========================================
  Files           6        6              
  Lines         671      674       +3     
==========================================
+ Hits          530      538       +8     
+ Misses        141      136       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Krastanov]

Oh, this is great, thank you!

With these recent contributions, please let me know if you would be amenable to being added to the maintainer team -- more volunteers would be much appreciated.

Could you bump the version number in Project.toml? That way I can make an immediate relase

Could you add this edge case to the tests as well, to make sure variables stored in the finite state machine are properly used?

@resumable function f()
  c = 1
  @yield [i for i in 1:10 if i<c]
  c += 1
  @yield [i for i in 1:10 if i<c]
end

[digital-carver]

Just an update, that test case exposed an issue which seems to do with Box-ing (collect(f()) itself fails with a failed convert attempt to Core.Box), looks like I can't get away with just a vague understanding of the scoping implementation anymore 😅 I'm looking into it deeper to figure this one out.

About being in the maintanence team: as the above paragraph points out, I'm still working out a lot of the details of how the package works, but I'm open to joining in and helping out in whatever ways I can. I've also had a brief look into #99 , so if/when I dive into that, I'll probably get a much better understanding of the package as a whole too.

[digital-carver]

Ah, it's looking like this was an existing problem before this PR, there just wasn't any pre-existing test for this. Trying a similar test on current master for normal (without conditionals) comprehension:

julia> @resumable function test_comprehension_state()
         c = 1
         @yield [i*c for i in 1:10]
         c += 1
         @yield [i*c for i in 1:10]
       end
test_comprehension_state (generic function with 1 method)

julia> for n in test_comprehension_state()
               @show n
       end
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Core.Box
The function `convert` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  Core.Box(::Any)
   @ Core boot.jl:434
  convert(::Type{T}, ::T) where T
   @ Base Base.jl:126

Stacktrace:
 [1] setproperty!(x::var"##test_comprehension_state_FSMI#231"{var"#1#3", Core.Box, var"#2#4"}, f::Symbol, v::Int64)
   @ Base ./Base.jl:52
 [2] ##test_comprehension_state_FSMI#231
   @ ~/repos/ResumableFunctions.jl/src/macro.jl:2 [inlined]
 [3] ##test_comprehension_state_FSMI#231
   @ ~/repos/ResumableFunctions.jl/src/macro.jl:213 [inlined]
 [4] iterate (repeats 2 times)
   @ ~/repos/ResumableFunctions.jl/src/types.jl:24 [inlined]
 [5] top-level scope
   @ ./REPL[4]:1

which is the same error.

The existing tests for comprehensions all have @yield towards the end of the test functions, and the variables used in the comprehensions don't get modified after the line containing the comprehension. That's the condition that causes the infamous closure-boxing problem that's the root of the issue here: c is used in the comprehension and also modified after the comprehension. That wasn't done in any of the current tests, so this wasn't caught previously.

So for now, I think the best course of action is to just bump the version in the Project.toml, and track the Box problem in a new issue to be solved separately, since it's a pre-existing issue. Let me know your thoughts on this, I can make the version change if that sounds fine.

[Krastanov]

Thank you, that sounds great! Do you mind making the issue and putting @test_broken for that issue inside of the tests as part of this PR?

[Krastanov]

You should have merge privileges now. I have reviewed this PR and I think it is a valuable improvement. Feel free to merge and release with @JuliaRegistrator without further review, even if you do some more minor polish and tests.