Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Formatting changes semantics (removes ;) #38

Closed
lmshk opened this issue Aug 7, 2024 · 6 comments
Closed

Formatting changes semantics (removes ;) #38

lmshk opened this issue Aug 7, 2024 · 6 comments

Comments

@lmshk
Copy link

lmshk commented Aug 7, 2024

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
Copy link
Owner

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
Copy link

pfitzseb commented Aug 8, 2024

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
Copy link
Owner

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
Copy link

pfitzseb commented Aug 8, 2024

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
Copy link
Owner

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
Copy link
Owner

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants