misc fixes to flatten

[Krastanov]

Fixes JuliaLang/julia#50710
Fixes #196
Fixes #194
Closes #195

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +5.85% 🎉

Comparison is base (d1937f9) 73.76% compared to head (b829874) 79.62%.

❗ 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     #197      +/-   ##
==========================================
+ Coverage   73.76%   79.62%   +5.85%     
==========================================
  Files           9       10       +1     
  Lines         404      422      +18     
==========================================
+ Hits          298      336      +38     
+ Misses        106       86      -20     

Files Changed	Coverage Δ
src/utils.jl	69.06% <100.00%> (+2.39%)	⬆️

... and 6 files with indirect coverage changes

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

[cstjean]

👍 Thank you for working on this!

[Krastanov]

Thanks for the quick review, Cédric!

My comment for the error message is a personal preference, but of course I will change it if you confirm that you as a maintainer prefer the shorter style.

I think I have given a complete argument for why simplifying the treatment of the try block would not work. Let me know if I am missing something or if you want me to approach it differently.

There are newly added tests that run on all julia versions and more tests for >=1.8 which are the only versions with support for try else.

[cstjean]

My comment for the error message is a personal preference, but of course I will change it if you confirm that you as a maintainer prefer the shorter style.

I don't have a strong personal preference, but conciseness is a pretty big theme of this package and I'm inclined to follow it, if you don't mind.

In any case, I would at least take out the Please report it, because 1. it's implicit in all error messages and 2. a malformed try block isn't a priori MacroTools' fault.

[Krastanov]

I will try to do a couple more changes, currently I reintroduced an issue with ResumableFunctions. Moving this to a draft.

[Krastanov]

I think now all comments are addressed. The difficulty was that arg[2] had to be a symbol or false, unlike the arg[3] or arg[4] which have to be a block or false. This is what makes it difficult to just use a map(some_kind_of_flatten, args).

Now it is as simple as I can think to make it.

Added clarification comments in code and tests. Added more tests. Fixed the error messages.

Please excuse the confused back and forth, I do not have much experience working with Julia macros.

[Krastanov]

Made the changes such that unnatural block-less try expressions are not forced to have a block inserted around them anymore and simplified the code as you suggested. Added tests for the block-less try expressions.

[cstjean]

💯 It's a really nice PR, thank you for bearing with me on the iterations!

[cstjean]

Should we consider this a breaking change? I think no, I think it's a bugfix, but still, could you please summarize the expressions whose outcome has changed? If you run the test suite on 1.9 without this PR, what fails?

Last I checked MacroTools is used in thousands of packages, so personally it's always a bit unnerving to tag a new release. I think we should sleep on this PR for a week or so in case we think of something wrong.

Don't hesitate to ping on this PR in case I forget about it. Typhoon's about to hit where I live...

[Krastanov]

I think it should be tagged as a bug fix. For the following two bugs.

1. flatten was a bit fragile with try blocks which got parsed slightly differently on 1.10 (extra line number nodes), which triggered wrong behavior. I will make one more commit in which I document that a bit better with more comments.

2. I think it is also correct to say that flatten(quote try f() catch end end) == Expr(:try, Expr(:call, ;f), false) was a bug that is now fixed.

I suggest tagging as bug fix and only if there is a complaint we can yank the release from the registry and tag as breaking. I do not expect complaints though.

I agree about waiting a week.

Be safe in the storm!

[Krastanov]

The last commit documents what issue existed both in 1.9 and 1.10 and what is the issue that existed only in 1.10.

All tests in CI pass.

All my local tests on 1.9 and nightly pass for MacroTools, ResumableFunctions, and ConcurrentSim (the original library in which I detected these issues).

The PR was reviewed and approved.

Should be good for merge and release as non-breaking bug fix after a week of due-diligence wait time.

[Krastanov]

@cstjean , I hope the storm was not too bad! Bumping this in case it can be merged now.