fix: scoping #100
fix: scoping #100
Conversation
Wow, indeed! You are delving in a hacky mess that has not been touched probably by anyone except for Ben, the original author. If this is merged, it will certainly be a breaking release, but for the better. @gerlero , as one of the main current users, a check from you will be very helpful. The original bounty is for a hopefully much cleaner fix that can be made possible in the future, but you are right that the infrastructure library on which it would depend is probably not anywhere close to ready. But I should be able to figure out some form of award for this more restricted set of fixes. I can not commit anything explicit right now, please give me a week to go through some logistical questions on my end. FYI, I will not be very responsive for the next week as I am running a summer workshop (qnumerics.org). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #100 +/- ##
=========================================
Coverage ? 76.94%
=========================================
Files ? 6
Lines ? 681
Branches ? 0
=========================================
Hits ? 524
Misses ? 157
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
@thofma , I added you to the repository with triage access so that CI is not auto-blocked for you. Apologies for the spam. |
|
Thanks. I got the upstream tests working (will add some of the examples as tests here). But I cannot wrap my hand around the error for Fronts.jl. Even with ResumabelFunctions 0.6.9 I get: So the error happens even without the changes here? |
|
Fronts.jl is by @gerlero (pinging in case you have time to look at this). Their last CI is from two months ago, but it passes on julia 1.10 (and it fails on nightly but with an unrelated error) |
|
The only errors in the "breakage" CI run are the ones that are already present without this PR, see #101. |
|
|
Reporting back: I made a hotfix for the latest and LTS failures. |
thofma
force-pushed
the
th/scopeandmore
branch
from
0133327
to
f0bc063
Compare
|
Tests are looking good, thanks for the quick fix. |
|
@Krastanov Modulo cleaning up and documenting the approach, I am happy with the functionality. I linked the issues that are resolved with this in OP. But there are many other things that got fixed along the way (see the added tests). Note that in my experience, just using JuliaLowering to rename the variables is only part of the solution. One then still has to resolve the challenges arising from "let", "for" and "named tuples". |
|
@Krastanov Any update on this? |
|
Apologies for the long wait on this. The start of the semester was a bit hectic, but it was unprofessional of me to not warn you about the delay. It will take me a bit to go over the complete review, but this looks very promising. As we had already discussed, the original bounty had slightly different scope. The main difference that is left compared to the original scope is that JuliaLowering can provide more resilient and clean way to create the state machine (in particular, an implementation that is much more probable to NOT deviate from Julia semantics in some weird edge cases). But in comparison to all the heavy lifting you have already done, that is a small component. I would like to reserve 400$ of the original bounty for that final push to JuliaLowering (when JuliaLowering is ready), but after review I will send the rest to you, either together with the bounties already being processed for you, or as a separate transfer (depending on how quickly we process the first round of bounties). This is slightly informal and potentially uncomfortable position, as the decision on what the award is, happens after most of the work has been done. I acknowledge that. Please do not hesitate to contest what I have described above. |
|
Sounds good to me, but could you wait with the review till I have cleaned it up a bit? I will let you know once it is ready. |
There was a problem hiding this comment.
This is very impressive work and I am eager to merge it. I left only documentation-related comments (things you might already be thinking of anyway, mostly aimed at future maintainers).
I left a bunch of comments of the form "Could you add an error message?". This is more for my sake than the users, so that I do not get confused if I need to work on this codebase in the future. Something along the lines of While transforming a @resumable function into a finite state machine, the following contract was broken: ... Please report this as a bug in ResumableFunctions.jl
| # :name is :(fA::A) if it is an overloading call function (fA::A)(...) | ||
| # ... | ||
| if func_def[:name] isa Expr | ||
| @assert func_def[:name].head == :(::) |
There was a problem hiding this comment.
Could you add an error message?
| # test for simple for a in b expression | ||
| expr.args[1].head === :(=) && return transform_for_inner(expr, ui8) | ||
| # must be a complicated iteration | ||
| @assert expr.args[1].head === :block |
There was a problem hiding this comment.
Could you add an error message?
| end | ||
| if s.head === :tuple | ||
| # we should never have to treat a (;a) = b here | ||
| @assert !(s.args[1] isa Expr && s.args[1].head === :parameters) |
There was a problem hiding this comment.
Could you add an error message?
| end | ||
|
|
||
| function scope_generator(expr, scope) | ||
| @assert expr.head === :generator |
There was a problem hiding this comment.
Could you add an error message?
| @assert expr.head === :generator | ||
| # first the generator case | ||
| for i in 2:length(expr.args) | ||
| @assert expr.args[i] isa Expr && expr.args[i].head === :(=) |
There was a problem hiding this comment.
Could you add an error message?
| @@ -34,6 +34,7 @@ macro resumable(expr::Expr) | |||
|
|
|||
| # The function that executes a step of the finite state machine | |||
| func_def = splitdef(expr) | |||
| @debug func_def[:body] | |||
There was a problem hiding this comment.
I like these @debug statements, please do not remove them after you are done. The overhead of the logger is fine in these "compilation" steps.
| function transform_remove_local(ex) | ||
| ex isa Expr && ex.head === :local && return Expr(:block) | ||
| return ex | ||
| end | ||
|
|
||
| function transform_macro(ex) | ||
| ex isa Expr || return ex | ||
| ex.head !== :macrocall && return ex | ||
| return Expr(:call, :__secret__, ex.args) | ||
| end | ||
|
|
||
| function transform_macro_undo(ex) | ||
| ex isa Expr || return ex | ||
| (ex.head !== :call || ex.args[1] !== :__secret__) && return ex | ||
| return Expr(:macrocall, ex.args[2]...) | ||
| end |
There was a problem hiding this comment.
Could you please add brief docstrings explaining why these steps are necessary?
| function transform_macro(ex) | ||
| ex isa Expr || return ex | ||
| ex.head !== :macrocall && return ex | ||
| return Expr(:call, :__secret__, ex.args) |
There was a problem hiding this comment.
Is a global const gensym viable here instead of __secret__? I guess gensym is not deterministic so this can be bad. What about just a struct MacroMarker end instance?
I just want to avoid the exceedingly rare possibility that some other weird metaprogramming package is using the same symbol.
| """ | ||
| Function that replaces a variable `_fsmi.x` in an expression by `x` where `x` is a variable declared in a `let` block. | ||
| """ | ||
| function transform_let(expr, symbols::Set{Symbol}) | ||
| function transform_local(expr) |
There was a problem hiding this comment.
Just curious about the naming convention. If this is specifically about let blocks, why do you prefer calling it _local? I am not asking for this to be changed, rather I just have easier time understanding other people's code if I understand the reasoning behind names.
| @@ -208,3 +210,338 @@ end | |||
| function typed_fsmi_fallback(fsmi::Type{T}, fargs...)::T where T | |||
| return T() | |||
| end | |||
|
|
|||
| mutable struct ScopeTracker | |||
There was a problem hiding this comment.
ScopeTracker and all the new functionality below it is the heaviest work done in this PR, right? Could you sprinkle docstrings for these objects, just for future developpers (e.g. someone who in the future decides to rework this using JuliaLowering)? I am more curious about the "why" of this design than anything else.
|
Whoops, just saw your other message. My bad! Anyway, my first round review was pretty superficial (I did not feel like I wasted time) and I probably pointed out things you already were planning to clean up. |
What a ride.
It is still not finished, but all tests are passing for now. Actually, lots of tests themselves had scoping issues and should not have been valid before. Also some examples in the manual were actually wrong.
Todo
let i = i; j = ia = sin; b = a(2)).(;a) = bsyntax (named tuple syntax breaks@resumable#93)UndefVarError: #temp# not defined#14 (need to fix theforpass)UndefVarError: #temp# not defined#14)@Krastanov I poked at the problem using JuliaLowering (after I built the unique julia master version for which some combination of packages works). I don't think it is in a state to be used here. I already spent too much time thinking about scope, so I cooked up this solution here.
Closes #14, #32, #69, #70, #93