fredrikekre / Runic.jl

A code formatter for Julia with rules set in stone.
MIT License
92 stars 2 forks source link

Formatting changes semantics (removes `;`) #38

Closed lmshk closed 1 month ago

lmshk commented 1 month ago

In situations involving macros, removing a trailing ; sometimes changes semantics. For example, in

using Catalyst
net = @network_component net begin
    r, X --> Y
end
net′ = extend(net, @network_component (@species Z(t);))

removing the ; actually makes this code invalid.

Runic should keep the ; in cases like this.

fredrikekre commented 1 month ago

Thanks for the report. The fault is that we first have a [block] node but without the ; it becomes a [parens] node. This doesn't matter unless in a macro context but probably better to stick to the node type that was in the source consistently.

julia> JuliaSyntax.parseall(JuliaSyntax.GreenNode, "(@a;)")
     1:5      │[toplevel]
     1:5      │  [block]
     1:1      │    (
     2:3      │    [macrocall]
     2:2      │      @
     3:3      │      MacroName          ✔
     4:4      │    ;
     5:5      │    )

julia> JuliaSyntax.parseall(JuliaSyntax.GreenNode, "(@a)")
     1:4      │[toplevel]
     1:4      │  [parens]
     1:1      │    (
     2:3      │    [macrocall]
     2:2      │      @
     3:3      │      MacroName          ✔
     4:4      │    )
pfitzseb commented 1 month ago

JuliaFormatter re-parses the output after formatting to check whether it's equivalent. I'd suggest doing that here as well, just as a basic sanity check (I don't think the overhead should be prohibitive).

fredrikekre commented 1 month ago

Yea, locally I reparse to check that the output is at least parseable, but I don't compare the expressions. It doesn't seem trivial though(?) because some changes can modify the tree without the meaning I think? E.g. if you insert a ; before kwargs in a call site.

julia> JuliaSyntax.parseall(JuliaSyntax.GreenNode, "f(a = 1)")
     1:8      │[toplevel]
     1:8      │  [call]
     1:1      │    Identifier           ✔
     2:2      │    (
     3:7      │    [=]
     3:3      │      Identifier         ✔
     4:4      │      Whitespace
     5:5      │      =
     6:6      │      Whitespace
     7:7      │      Integer            ✔
     8:8      │    )

julia> JuliaSyntax.parseall(JuliaSyntax.GreenNode, "f(; a = 1)")
     1:10     │[toplevel]
     1:10     │  [call]
     1:1      │    Identifier           ✔
     2:2      │    (
     3:9      │    [parameters]
     3:3      │      ;
     4:4      │      Whitespace
     5:9      │      [=]
     5:5      │        Identifier       ✔
     6:6      │        Whitespace
     7:7      │        =
     8:8      │        Whitespace
     9:9      │        Integer          ✔
    10:10     │    )
pfitzseb commented 1 month ago

Oh, yeah, good point. I guess there needs to be a "semantic normalization" pass that doesn't act on macro inputs. Not actually sure how JuliaFormatter handles that...

fredrikekre commented 1 month ago

It also doesn't seem feasible to skip all formatting except whitespace inside macros even if a macro can observe it. Then you couldn't format a function with an @inline annotation or a struct with @kwdef etc...

fredrikekre commented 1 month ago

Thanks for the report, this is fixed in the patch linked above.

This is comparable to (1) vs (1,) where the trailing comma is also important (this case was already handled correctly though).