MacroTools.flatten does not work on 1.10 beta

[Krastanov]

Edit: a simpler MWE is shown in the next comment. Skip this earlier description of the problem.

ResumableFunctions has a rather complicated macro @resumable. That macro used to work fine in 1.9, but now it produces code with syntax errors:

julia> using ResumableFunctions

julia> @resumable function f()
           try @yield g() catch end
       end

ERROR: syntax: cannot goto label "_STATE_1" inside try/catch block
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

julia> versioninfo()
Julia Version 1.10.0-beta1
Commit 6616549950e (2023-07-25 17:43 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 32 × AMD Ryzen 9 7950X 16-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, znver3)
  Threads: 1 on 32 virtual cores

julia> using Pkg; Pkg.status()
Status `/tmp/Project.toml`
  [c5292f4c] ResumableFunctions v0.6.3

The creator of that macro is not active anymore. I will try to investigate and prepare a smaller example.

[Krastanov]

Edit: see the next comment for an even simpler MWE

The problem is with MacroTools.flatten deleting catch blocks.

On 1.10 beta, the catch block is lost

julia> using MacroTools

julia> MacroTools.flatten(quote try f() catch end end)
quote
    #= REPL[2]:1 =#
    try
        #= REPL[2]:1 =#
        f()
    end
end

julia> versioninfo()
Julia Version 1.10.0-beta1
Commit 6616549950e (2023-07-25 17:43 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 32 × AMD Ryzen 9 7950X 16-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, znver3)
  Threads: 47 on 32 virtual cores

julia> using Pkg; Pkg.status()
Status `/tmp/Project.toml`
  [1914dd2f] MacroTools v0.5.10

On 1.9 the catch block is preserved

julia> using MacroTools

julia> MacroTools.flatten(quote try f() catch end end)
quote
    #= REPL[2]:1 =#
    try
        #= REPL[2]:1 =#
        f()
    catch
    end
end

julia> versioninfo()
Julia Version 1.9.2
Commit e4ee485e909 (2023-07-05 09:39 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 32 × AMD Ryzen 9 7950X 16-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, znver3)
  Threads: 32 on 32 virtual cores

julia> using Pkg; Pkg.status()
Status `/tmp/Project.toml`
  [1914dd2f] MacroTools v0.5.10

[Krastanov]

Yet more simplified:

On 1.9:

julia> :(try f() catch end).args[3].args |> length
0

On 1.10 beta

julia> :(try f() catch end).args[3].args |> length
1

@c42f , any chance this is due to the new JuliaSyntax parser?

[KristofferC]

This just seems to be due to the new parser adding an extra line info node?

1.9:

julia> :(try f() catch end)
:(try
      #= REPL[1]:1 =#
      f()
  catch
  end)

1.10:

julia> :(try f() catch end)
:(try
      #= REPL[1]:1 =#
      f()
  catch
      #= REPL[1]:1 =#
  end)

Seems like this should be fixed in MacroTools.

[Krastanov]

It can be fixed in MacroTools and I would be happy to even submit the PR myself later today, but is this not a "breaking change" that might lead to incompatibilities with user code running in production "somewhere".

[KristofferC]

I don't think more accurate line info in expressions is considered breaking no.

[cstjean]

Twist: eval(MacroTools.flatten(quote try f() catch end end)) yields the correct result (nothing) in both Julia 1.9 and Julia 1.10. I think there's a bug in the pretty-printer (not new AFAICT): while the compiler treats a third-argument LineNumberNode as an empty catch block, the pretty-printer treats it as an absent catch.

And so I'd say that #50710 (comment) is an unrelated bug with respect to this issue.

[cstjean]

@Krastanov Did you test that FluxML/MacroTools.jl#195 really fixes the ResumableFunctions issue?

[Krastanov]

Actually, no. I will go head and to that shortly.

[KristofferC]

@brenhinkeller, what's the regression here?

[brenhinkeller]

I mean, it's a regression in that something used to work in a past version and now doesn't, but I agree that this is MacroTools responsibility to fix

[Krastanov]

Maybe a team member can update the label description for "regression" - I have seen this come up a few times where some people assume it is either "any regression" or "only performance regressions or breakage covered by Julia's semver guarantees". Another source of confusion is that the naive interpretation of "semver guarantee" is different from the battle-born version used by Julia devs (I think reasonably so, but still, it does cause confusion in this type of conversation every couple of weeks)

[Krastanov]

I will submit a different approach to the MacroTools fix tonight (there is one fix submitted already, but there is discussion about whether it is the appropriate way to do it)

[cstjean]

Perhaps I missed something, but I still don't understand what the bug is. As I mentioned in #50710 (comment), the bug in this issue and in FluxML/MacroTools.jl#194 is with the pretty printer, there's no difference in outcome. FluxML/MacroTools.jl#196 is a true bug, but it's unrelated AFAICT? So if the latest MacroTools PR really fixes @resumable, I'd love to understand why.

[Krastanov]

I will attempt to answer:

Julia 1.9.2, MacroTools 0.5.10, ResumableFunctions 0.6.4

julia> quote
           try
               begin f() end
           catch
           end
       end |> flatten |> striplines |> dump
Expr
  head: Symbol block
  args: Array{Any}((1,))
    1: Expr
      head: Symbol try
      args: Array{Any}((3,))
        1: Expr
          head: Symbol block
          args: Array{Any}((1,))
            1: Expr
              head: Symbol call
              args: Array{Any}((1,))
                1: Symbol f
        2: Bool false
        3: Expr
          head: Symbol block
          args: Array{Any}((0,))

Julia 1.11-dev, MacroTools 0.5.10, ResumableFunctions 0.6.4

This is wrong, it gives me a try block with 2 arguments when the minimum valid one is a try block with 3 arguments. Then ResumableFunctions garbles it even more because it was expecting 3 arguments (but did not verify that it actually got 3 arguments).

julia> quote
           try
               begin f() end
           catch
           end
       end |> flatten |> striplines |> dump
Expr
  head: Symbol block
  args: Array{Any}((1,))
    1: Expr
      head: Symbol try
      args: Array{Any}((2,))
        1: Expr
          head: Symbol block
          args: Array{Any}((1,))
            1: Expr
              head: Symbol call
              args: Array{Any}((1,))
                1: Symbol f
        2: Bool false

Julia 1.11-dev, MacroTools 0.5.10 with the fix, ResumableFunctions 0.6.4

julia> quote
           try
               begin f() end
           catch
           end
       end |> flatten |> striplines |> dump
Expr
  head: Symbol block
  args: Array{Any}((1,))
    1: Expr
      head: Symbol try
      args: Array{Any}((3,))
        1: Expr
          head: Symbol block
          args: Array{Any}((1,))
            1: Expr
              head: Symbol call
              args: Array{Any}((1,))
                1: Symbol f
        2: Bool false
        3: Expr
          head: Symbol block
          args: Array{Any}((0,))

the fix also includes tests for this

[Krastanov]

In other words, it is not just a pretty printing issue: the catch block was actually being deleted, try was left with 2 arguments, while it was supposed to have 3. What triggered this was that in 1.10 extra line number nodes were being emitted, and the original MacroTools.flatten has some edge-case-inconsistent logic about how to treat line number nodes.

[vchuravy]

I am going to close this here, since this is a MacroTools bug and not a Julia base one.

@Krastanov thanks for digging into this!

[cstjean]

Thank you for the details! Just one piece that I didn't understand:

julia> quote
           try
               begin f() end
           catch
           end
       end |> flatten |> dump
Expr
  head: Symbol block
  args: Array{Any}((2,))
    1: LineNumberNode
      line: Int64 2
      file: Symbol REPL[8]
    2: Expr
      head: Symbol try
      args: Array{Any}((3,))
        1: Expr
          head: Symbol block
          args: Array{Any}((3,))
            1: LineNumberNode
              line: Int64 3
              file: Symbol REPL[8]
            2: LineNumberNode
              line: Int64 3
              file: Symbol REPL[8]
            3: Expr
              head: Symbol call
              args: Array{Any}((1,))
                1: Symbol f
        2: Bool false
        3: LineNumberNode
          line: Int64 4
          file: Symbol REPL[8]

Third element is a LineNumberNode, which gets stripped afterwards by striplines. So then indeed, preserving blocks is a necessity. I wonder if there are other problematic expressions like this.

[Krastanov]

My understanding is that this is also an example of MacroTools.flatten doing the wrong thing - the third argument should be a block wrapping a line number. But I was not able to find a documentation for what is actually permitted -- I am just going of what I see julia produces when parsing.

[cstjean]

Right - but that's what your PR fixes, no?

[Krastanov]

yes