fix for new line num nodes from JuliaSyntax

[Krastanov]

Fixes #194
Fixes JuliaLang/julia#50710

[codecov-commenter]

Codecov Report

Patch coverage: 93.75% and project coverage change: +5.91% 🎉

Comparison is base (d1937f9) 73.76% compared to head (29d342e) 79.67%.

❗ 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     #195      +/-   ##
==========================================
+ Coverage   73.76%   79.67%   +5.91%     
==========================================
  Files           9       10       +1     
  Lines         404      428      +24     
==========================================
+ Hits          298      341      +43     
+ Misses        106       87      -19     

Files Changed	Coverage Δ
src/utils.jl	69.51% <93.75%> (+2.85%)	⬆️

... and 6 files with indirect coverage changes

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

[Krastanov]

This still feels a bit fragile, but no more fragile than it already was. I call it fragile because julia does not have a :catch block, rather just an implicit third argument to the :try block, so flattening nameless quote blocks into empty :() is dangerous. Probably try should be special-cased for a more robust fix, but I do not dare touch that for now.

[cstjean]

I'd say that in general, Expr(:block, x) -> x is a valid transform, including for line numbers, so I'm a bit reluctant.

[Krastanov]

Are you saying that you prefer that try is special cased here?

[cstjean]

Could you special-case try? I'm not super into this code, but it looks to me like the special-casing solution would involve replacing the flatten postwalk with an explicit tree traversal, which I also wouldn't like very much. I don't know! It's not a fun bug to fix. Let's see if it really fixes JuliaLang/julia#50710 (comment). If not, then is there a reason to merge this? It might break someone else's subtle macrology.

[Krastanov]

@cstjean , I think I successfully special-cased try. You are right that without reworking the tree-walk, a complete fix is impossible, but I think now a bunch of bugs are fixed and a bunch more silent bugs were turned into loud errors.

The ResumableFunctions issue is fixed by this as well.

[Krastanov]

To help with the review, here is some context:

Before this PR, the following was being processed incorrectly (the error is not caught):

quote; try; error(); catch; false; finally; println(123); end; end |> striplines |> flatten |> eval

Now flatten raises an error and refuses to proceed.

However, the following used to work, but now it raises an error (with a workaround suggested in the error message):

quote; try; 1+1; finally; println(123); end; end |> striplines |> flatten |> eval

I do not see how to avoid the silent bug in the first snippet without refusing to process the second snippet as well -- well, except for a full rewrite of flatten that does not use postwalk.

[Krastanov]

I think making sure people do not run into silent bugs is a good enough reason to sacrifice a bit of related accidentally working functionality.

[Krastanov]

It is also of note that this is highly improbable to be triggered: someone needs to explicitly use striplines otherwise the line number nodes prevent this ambiguity from happening.

[cstjean]

except for a full rewrite of flatten that does not use postwalk.

I think it'd be worth giving this a try, in a separate PR. Sorry that I said I wouldn't like that approach; I just didn't see a completely satisfying (simple and correct) solution anywhere. Mike Innes' original code is beautifully simple and straight-forward, and I'd like to keep that as much as possible.

In ay case, I would start by looking at pre/postwalk and write the recursive descent flatten that special-cases try , so that the third argument's block is never flattened. I don't want to special-case false, because that's precisely the mistake that Julia's AST did, and that special-case can have ripples downstream.

Thank you for tackling this!

[cstjean]

Something like

flatten(x) = x
function flatten(x::Expr)
  if isexpr(x, :try)
      Expr(:try, flatten(x.args[1]), flatten(x.args[2]), Expr(:block, map(flatten, x.args[3].args)...))
   else
     flatten1(Expr(x.head, map(flatten, x.args)...))
end

(just a sketch, this is full of mistakes for sure)

In retrospect, that's quite simple and we get to solve all problems perfectly. Sorry for not having suggested it sooner.

[Krastanov]

I am missing something. I was under the impression that only the following options exist for try blocks:

• 3 argument blocks where the 3rd argument is a block, corresponding to try catch end
• 4 argument blocks where the 3rd argument is a block, corresponding to try catch finally end
• 4 argument blocks where the 3rd argument is false, corresponding to try finally end
• 5 argument blocks where the 4th argument is a block, corresponding to try catch else finally end
• 5 argument blocks where the 4th argument is false, corresponding to try catch else end

In your example above it seems like you only permit 3 argument blocks?

Or maybe you did not mean to focus on specifically what the special case for try should look like, rather you are telling me to use a "prewalk" style recursion instead of the current "postwalk" style recursion? I think I agree with that, but I will not have the time to attempt to contribute this for a couple of weeks.

[cstjean]

Or maybe you did not mean to focus on specifically what the special case for try should look like, rather you are telling me to use a "prewalk" style recursion instead of the current "postwalk" style recursion?

Yes, that. Of course we need to handle all the cases you mentioned. Sorry that wasn't clear.

It's not a prewalk, in the sense that it doesn't apply the transformation again on the outcome. It's like postwalk, where we simply do not directly apply the transformation to the third argument of the try, but deal with it appropriately.

I think I agree with that, but I will not have the time to attempt to contribute this for a couple of weeks.

👍 Thank you very much for the PR and issue! It helps a lot. I don't think that Julia 1.10 will be ready for a long time, so I don't think it's very pressing. Could even be fixed after release, arguably.