fix comments in array literals (#1115)

Fixes #1113 
Fixes #1116

claude did this, and i will probably merge this after reviewing it
because it's a really, really ugly issue and it should be fixed, but in
reality this is a gigantic hack.

the right fix would be to do one or both of the following

1) restructure the CST so that the comments are placed at the top level
of the vcat rather than inside the row.
2) completely rework the handling of line comments. right now they're
not present in the FST and must be added back later via a notcode
mechanism which means lots of moving parts that all interact with one
another.

Author: penelopeysm

HISTORY_v2.md

@@ -1,3 +1,7 @@
+# v2.8.1
+
+Fixed a bug causing line comments inside array literals to be dropped or otherwise cause non-idempotent formatting. (#1113, #1115, #1116)
+
 # v2.8.0
 
 Fixed a bug causing lack of idempotence in typed comprehension expressions (i.e., things like `T[expr for x in y]`). (#1105, #1106)

Project.toml

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

src/styles/default/pretty.jl

@@ -1015,6 +1015,8 @@ end
 # `#= ... =#` comments are reported as whitespace by JuliaSyntax, so block
 # handlers that skip whitespace would otherwise drop them. Append the comment
 # (keeping it on the current line when it trails code) instead of losing it.
+#
+# Returns true if the comment was added, false otherwise.
 function add_hasheq_comment!(t::FST, n::FST, s::State)
     n.typ === HASHEQCOMMENT || return false
     tnodes = t.nodes::Vector{FST}
@@ -3698,6 +3700,7 @@ function p_hcat(
     # to decide whether to place boundary newlines. In other words, we can safely insert
     # Placeholder nodes at these positions.
 
+    prev_comment_was_dropped = false
     for (i, a) in enumerate(childs)
         n = pretty(style, a, s, ctx, lineage)
         if kind(a) === K"["
@@ -3728,7 +3731,7 @@ function p_hcat(
             add_node!(t, n, s; join_lines = true)
         elseif JuliaSyntax.is_whitespace(a)
             if kind(a) === K"Comment"
-                add_node!(t, n, s; join_lines = true)
+                prev_comment_was_dropped = !add_hasheq_comment!(t, n, s)
             elseif is_newline_after_2semicolons(cst, i)
                 # See above: we cannot convert ';;\n' to ';;'
                 add_node!(t, Newline(; nest_behavior = AlwaysNest), s)
@@ -3738,13 +3741,36 @@ function p_hcat(
                 # later on, `n_tuple!` will insert newlines after the `[` and before the
                 # `].`
                 t.nest_behavior = AlwaysNest
-            elseif !(kind(cst[i+1]) in KSet"; ]") && !(kind(cst[i-1]) in KSet"; [")
+                prev_comment_was_dropped = false
+            elseif !(kind(cst[i+1]) in KSet"; ] Comment") && !(kind(cst[i-1]) in KSet"; [")
                 # Whitespace is generally important to retain, because it can be a separator
                 # -- but we should omit it in a few cases:
                 # 1. If it's followed by / before a ';;' separator, since it's not needed.
                 # 2. Directly after the opening bracket.
                 # 3. Directly before the closing bracket.
-                add_node!(t, Whitespace(1), s)
+                #
+                # There's also some special logic for comments, because the current way that
+                # JuliaFormatter deals with comments is crazily hacky.
+                if !isnothing(first_arg_idx) &&
+                   i < first_arg_idx &&
+                   prev_comment_was_dropped
+                    # Before the first argument, after a dropped regular # comment.
+                    # Suppress whitespace so NOTCODE gap detection can reconstruct
+                    # the comment at the parent level.
+                elseif prev_comment_was_dropped
+                    # After a dropped regular # comment, past the first argument.
+                    # Remove the preceding forced NEWLINE (e.g. from ;;) so that
+                    # add_node!'s gap detection is not short-circuited by
+                    # is_prev_newline, and can insert NOTCODE for the comment.
+                    if is_prev_newline(t)
+                        remove_prev_newline!(t)
+                    end
+                else
+                    add_node!(t, Whitespace(1), s)
+                end
+                prev_comment_was_dropped = false
+            else
+                prev_comment_was_dropped = false
             end
         elseif i == 1 && kind(a) !== K"["
             # Type preceding the `[`, e.g. `T` in `T[1 2 3]`.
@@ -3862,6 +3888,24 @@ function is_semantically_important_newline(
     n = length(children(row_cst))
     # Start of the row - not important
     i == 1 && return false
+    # A newline is not important if it's adjacent to a comment and all the children
+    # between the comment and the boundary of the row (start or end) are whitespace.
+    #
+    # Trailing comment case: all children after this newline are whitespace, and at least
+    # one is a comment. Example: `3 4\n # bar\n` — the newline before `# bar` is not a row
+    # separator. We don't need to include it.
+    if all(j -> JuliaSyntax.is_whitespace(row_cst[j]), (i+1):n) &&
+       any(j -> kind(row_cst[j]) === K"Comment", (i+1):n)
+        return false
+    end
+    # Leading comment case: all children before this newline are whitespace, and at least
+    # one is a comment. Example: `\n # foo\n 1 2` — the newline after `# foo` is not a row
+    # separator. We can drop all newlines after the comment, because the insertion of the
+    # newline after the comment is handled by the NOTCODE mechanism later on.
+    if all(j -> JuliaSyntax.is_whitespace(row_cst[j]), 1:(i-1)) &&
+       any(j -> kind(row_cst[j]) === K"Comment", 1:(i-1))
+        return false
+    end
     # End of the row -- might be important
     i == n && return (!is_last_arg_of_parent && kind(row_cst[i-1]) !== K";")
     # Otherwise it's important

test/multidimensional_array.jl

@@ -239,4 +239,68 @@ end
     end
 end
 
+@testset "#1113 - comments in vcat array literals" begin
+    for s in (
+        "X = [\n    # leading\n    1 2\n    3 4\n]\n",
+        "X = [\n    1 2\n    3 4\n    # trailing\n]\n",
+        "X = [\n    # foo\n    1 2\n    3 4\n    # bar\n]\n",
+        "X = [\n    1 2\n    # between\n    3 4\n]\n",
+        "X = Int[\n    # foo\n    1 2\n    3 4\n    # bar\n]\n",
+    )
+        for style in ALL_STYLES
+            if style isa DefaultStyle
+                # Default should leave it unchanged
+                test_format(s, s; ast = true)
+            else
+                test_format(s, nothing, style; ast = true)
+            end
+        end
+    end
+end
+
+@testset "#1113 - comments in ncat array literals" begin
+    for s in (
+        "[\n    # leading\n    1\n    2;;\n    3\n    4\n]\n",
+        "[\n    1\n    2;;\n    3\n    4\n    # trailing\n]\n",
+        "[\n    1\n    2;;\n    # between\n    3\n    4\n]\n",
+        "[\n    1;\n    2;;\n    # between\n    3;\n    4\n]\n",
+    )
+        for style in ALL_STYLES
+            if style isa DefaultStyle
+                # Default should leave it unchanged
+                test_format(s, s; ast = true)
+            else
+                test_format(s, nothing, style; ast = true)
+            end
+        end
+    end
+
+    # this one is still a bit wonky because the \n after 2;; is collapsed
+    s = "[\n    # leading\n    1\n    2;;\n    3\n    4;;;\n    5\n    6;;\n    7\n    8\n]\n"
+    for style in ALL_STYLES
+        test_format(s, nothing, style; ast = true)
+        @test occursin("# leading", format_text(s))
+    end
+    @test_broken format_text(s) == s
+end
+
+@testset "#1113 - comments in hcat array literals" begin
+    for s in (
+        "X = [\n    # only\n    1 2\n]\n",
+        "X = [\n    1 2\n    # trailing\n]\n",
+        "X = [\n    # foo\n    1 2\n    # bar\n]\n",
+        "X = [\n    1 2;;\n    # hi\n    3 4\n]\n",
+        "X = [#= inline =# 1 2]\n",
+        "X = [1 #= mid =# 2]\n",
+    )
+        for style in ALL_STYLES
+            if style isa DefaultStyle
+                test_format(s, s; ast = true)
+            else
+                test_format(s, nothing, style; ast = true)
+            end
+        end
+    end
+end
+
 end # module