search

FluxML / MacroTools.jl

MacroTools provides a library of tools for working with Julia code and expressions.
https://fluxml.ai/MacroTools.jl/stable/
Other
310 stars 79 forks source link

misc fixes to `flatten` #197

Closed Krastanov closed 1 year ago

Krastanov commented 1 year ago

Fixes https://github.com/JuliaLang/julia/issues/50710 Fixes #196 Fixes #194 Closes #195

codecov-commenter commented 1 year ago

Codecov Report

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

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

:exclamation: 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 ```diff @@ 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](https://app.codecov.io/gh/FluxML/MacroTools.jl/pull/197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux) | Coverage Δ | | |---|---|---| | [src/utils.jl](https://app.codecov.io/gh/FluxML/MacroTools.jl/pull/197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux#diff-c3JjL3V0aWxzLmps) | `69.06% <100.00%> (+2.39%)` | :arrow_up: | ... and [6 files with indirect coverage changes](https://app.codecov.io/gh/FluxML/MacroTools.jl/pull/197/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Krastanov commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Krastanov commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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