Fix cases where trailing comma should not be added (#1086)

Closes #1017

Author: penelopeysm

HISTORY_v2.md

@@ -1,3 +1,7 @@
+# v2.6.13
+
+Fixed a bug where JuliaFormatter would insert trailing commas after expressions that had macros or `global` keywords, leading to syntactically invalid Julia code. (#1017, #1086)
+
 # v2.6.12
 
 Fixed a bug where `;;\n` separators in rows of array literals were being converted to `;;`, leading to invalid Julia code. (#1080, #1083)

Project.toml

@@ -1,6 +1,6 @@
 name = "JuliaFormatter"
 uuid = "98e50ef6-434e-11e9-1051-2b60c6c9e899"
-version = "2.6.12"
+version = "2.6.13"
 authors = ["Dominique Luna <dluna132@gmail.com> and contributors"]
 
 [deps]

docs/make.jl

@@ -34,6 +34,7 @@ makedocs(;
         "Contributing" => "contributing.md",
         "Project Status" => "status.md",
     ],
+    checkdocs = :export,
 )
 
 deploydocs(;

src/fst.jl

@@ -1006,6 +1006,45 @@ function next_node_is(f::Function, nn::JuliaSyntax.GreenNode)
     f(nn) || (haschildren(nn) && next_node_is(f, nn[1]))
 end
 
+function ends_with_macro_or_global(fst::FST)
+    # Detects constructs such as
+    #    @macro foo
+    #    arg = @macro foo
+    #    arg => @macro foo
+    #    () -> global x = true
+    # which cause parse errors if a comma is inserted after them.
+    return if fst.typ === MacroCall || fst.typ === MacroBlock || fst.typ === Global
+        true
+    elseif is_leaf(fst)
+        false
+    elseif fst.typ === Kw || fst.typ === Binary
+        length(fst.nodes) > 0 && ends_with_macro_or_global(fst[end])
+    else
+        false
+    end
+end
+function skip_trailing_comma(fst::FST)
+    prev_node = fst[end]
+    return if is_comma(prev_node) && fst.typ === TupleN && n_args(fst) == 1
+        # e.g. `(x,)` -- removing the comma changes the meaning
+        true
+    elseif (
+        prev_node.typ === Generator ||
+        prev_node.typ === Filter ||
+        prev_node.typ === Flatten ||
+        prev_node.typ === SEMICOLON ||
+        prev_node.typ === HASHEQCOMMENT ||
+        # Things that end with macros can't have a trailing comma inserted after them
+        # as that causes Julia to fail to parse.
+        # https://github.com/JuliaEditorSupport/JuliaFormatter.jl/issues/1017
+        ends_with_macro_or_global(prev_node)
+    )
+        true
+    else
+        false
+    end
+end
+
 """
     add_node!(
         t::FST,
@@ -1047,28 +1086,14 @@ function add_node!(
 
     if n.typ === TRAILINGCOMMA
         en = (tnodes::Vector{FST})[end]
-        if en.typ === Generator ||
-           en.typ === Filter ||
-           en.typ === Flatten ||
-           en.typ === MacroCall ||
-           en.typ === MacroBlock ||
-           en.typ === SEMICOLON ||
-           en.typ === HASHEQCOMMENT ||
-           # arg = @macro foo
-           en.typ === Kw && (en[end].typ === MacroCall || en[end].typ === MacroBlock) ||
-           # arg => @macro foo
-           en.typ === Binary && (en[end].typ === MacroCall || en[end].typ === MacroBlock)
-
-            # adding a trailing comma causes a syntax error
-            false
-        elseif is_comma(en) && t.typ === TupleN && n_args(t) == 1
-            # e.g. `(x,)` -- removing the comma changes the meaning
+        if skip_trailing_comma(t)
+            # do not insert trailing comma
             false
         elseif s.opts.trailing_comma === nothing
             # preserve original source code
             false
         elseif !s.opts.trailing_comma::Bool
-            # remove trailing comma
+            # remove preexisting comma in FST
             if is_comma(en)
                 t[end] = Whitespace(0)
             end

src/internal/utils.jl

@@ -117,6 +117,12 @@ function format_to_stage(
     throw(ArgumentError("unknown stage: $stage"))
 end
 
+"""
+    _repro_hint(input, style, options)
+
+Generate a hint for how to reproduce a failure in `test_format`, given the input string,
+style, and options.
+"""
 function _repro_hint(input, style, options)
     opts_str = if isempty(options)
         ""
@@ -131,6 +137,33 @@ function _repro_hint(input, style, options)
     return "s = $(repr(input))\nformat_text(s$(style_str)$(opts_str))"
 end
 
+"""
+    _normalise_ast!(ex)
+
+Normalise ASTs (mostly by removing line number nodes, which are not relevant for semantics).
+"""
+_normalise_ast!(ex) = ex
+function _normalise_ast!(ex::Expr)
+    if ex.head === :block || ex.head === :quote || ex.head === :toplevel
+        filter!(ex.args) do x
+            !(isa(x, Expr) && x.head === :line || isa(x, LineNumberNode))
+        end
+    end
+    # macrocall's apparently _need_ to have a line number node or something in that
+    # position, so we can't remove it altogether -- but we can replace all such nodes with
+    # `nothing` so that they compare equal. See: https://stackoverflow.com/a/53285644
+    if ex.head === :macrocall
+        new_args = map(ex.args) do x
+            x isa LineNumberNode ? nothing : x
+        end
+        ex.args = new_args
+    end
+    for subex in ex.args
+        subex isa Expr && _normalise_ast!(subex)
+    end
+    return ex
+end
+
 """
     JuliaFormatter.Internal.test_format(
         input::AbstractString,
@@ -182,6 +215,8 @@ function test_format(
     if ast
         ast_in = Meta.parse(input)
         ast_out = Meta.parse(out)
+        _normalise_ast!(ast_in)
+        _normalise_ast!(ast_out)
         if ast_in != ast_out
             printstyled("AST of input and output did not match.\n\n"; color = :cyan)
             printstyled("Input AST:\n$ast_in\n\n"; color = :green)

test/issues.jl

@@ -2471,7 +2471,43 @@ end
         test_format(s, s)
     end
 
-    @testset "1018" begin
+    @testset "1017 trailing commas after macros/global" begin
+        # macro
+        s_ = "f(a = u -> @. u0)"
+        s = "f(\n    a = u ->\n        @. u0\n)"
+        for trailing_comma in (true, false, nothing)
+            test_format(s_, s_; trailing_comma = trailing_comma)
+            test_format(s_, s; margin = 10, trailing_comma = trailing_comma)
+            for style in (SciMLStyle(), BlueStyle(), YASStyle(), MinimalStyle())
+                test_format(s_, nothing; margin=10, ast=true)
+            end
+        end
+
+        # global
+        s_ = raw"""
+        let libccalllazyfoo = LazyLibrary(lclf_path; on_load_callback=() -> global lclf_loaded = true),
+            libccalllazybar = LazyLibrary(lclb_path; dependencies=[libccalllazyfoo], on_load_callback=() -> global lclb_loaded = true)
+            eval(:(const libccalllazyfoo = $libccalllazyfoo))
+            eval(:(const libccalllazybar = $libccalllazybar))
+        end"""
+        s = raw"""
+        let libccalllazyfoo =
+                LazyLibrary(lclf_path; on_load_callback = () -> global lclf_loaded = true),
+            libccalllazybar = LazyLibrary(
+                lclb_path;
+                dependencies = [libccalllazyfoo],
+                on_load_callback = () -> global lclb_loaded = true
+            )
+
+            eval(:(const libccalllazyfoo = $libccalllazyfoo))
+            eval(:(const libccalllazybar = $libccalllazybar))
+        end"""
+        for trailing_comma in (true, false, nothing)
+            test_format(s_, s; trailing_comma = trailing_comma)
+        end
+    end
+
+    @testset "1018 macro do-block" begin
         # A macro call using `do` with keyword-style args must be formatted like the
         # function-call form, not left partially unformatted with extra spaces and
         # unindented multiline `do` arguments.