Repository: domluna/JuliaFormatter.jl
Pull request #111 (2019-11-04 03:36:24)

Formatting julia repo fixes
I ran format over all .jl files in the julia repo and found a handful of bugs.


Patch:
diff --git a/src/JuliaFormatter.jl b/src/JuliaFormatter.jl
index 40ed2487..8aa1e920 100644
--- a/src/JuliaFormatter.jl
+++ b/src/JuliaFormatter.jl
@@ -96,7 +96,8 @@ function Document(text::AbstractString)
                     if c == '\n'
                         s = length(ranges) > 0 ? last(ranges[end]) + 1 : 1
                         push!(ranges, s:offset+1)
-                        idx = min(findfirst(c -> !isspace(c), cs), ws + 1)
+                        fc = findfirst(c -> !isspace(c), cs)
+                        idx = fc === nothing ? 1 : min(fc, ws + 1)
                         comments[line] = (ws, cs[idx:end])
                         line += 1
                         cs = ""
@@ -147,7 +148,7 @@ function Document(text::AbstractString)
         str = str[idx1:end]
         push!(format_skips, (stack[1], -1, str))
     end
-    # @info "" comments format_skips
+    # @info "" lit_strings
     Document(text, ranges, line_to_range, lit_strings, comments, semicolons, format_skips)
 end
 
@@ -241,7 +242,7 @@ function format_text(
     text = normalize_line_ending(text)
 
     _, ps = CSTParser.parse(CSTParser.ParseState(text), true)
-    ps.errored && error("Parsing error for formatted $text")
+    ps.errored && error("Parsing error for formatted text:\n\n $text")
     return text
 end
 
diff --git a/src/nest.jl b/src/nest.jl
index 8730ac10..6b8f2921 100644
--- a/src/nest.jl
+++ b/src/nest.jl
@@ -245,7 +245,12 @@ function n_stringh!(x, s; extra_width = 0)
 end
 
 function n_for!(x, s; extra_width = 0)
-    ph_idx = findfirst(n -> n.typ === PLACEHOLDER, x.nodes[3].nodes)
+    block_idx = findfirst(n -> !is_leaf(n), x.nodes)
+    if block_idx === nothing
+        nest!(x.nodes, s, x.indent, extra_width = extra_width)
+        return
+    end
+    ph_idx = findfirst(n -> n.typ === PLACEHOLDER, x.nodes[block_idx].nodes)
     nest!(x.nodes, s, x.indent, extra_width = extra_width)
 
     # return if the argument block was nested
diff --git a/src/pretty.jl b/src/pretty.jl
index 3f2d4bf8..a0af34d4 100644
--- a/src/pretty.jl
+++ b/src/pretty.jl
@@ -98,8 +98,18 @@ function add_node!(t::PTree, n::PTree, s::State; join_lines = false, max_padding
             push!(t.nodes, n)
             return
         end
-    elseif n.typ === TRAILINGCOMMA && is_comma(t.nodes[end])
-        t.nodes[end] = n
+    elseif n.typ === TRAILINGCOMMA
+        en = t.nodes[end]
+        if en.typ === CSTParser.Generator || en.typ === CSTParser.Filter
+            # don't insert trailing comma in these cases
+        elseif is_comma(en)
+            t.nodes[end] = n
+        else
+            t.len += length(n)
+            n.startline = t.startline
+            n.endline = t.endline
+            push!(t.nodes, n)
+        end
         return
     elseif n.typ === NOTCODE || n.typ === INLINECOMMENT
         push!(t.nodes, n)
@@ -460,7 +470,8 @@ function p_literal(x, s)
     # Tokenize treats the `ix` part of r"^(=?[^=]+)=(.*)$"ix as an
     # IDENTIFIER where as CSTParser parses it as a LITERAL.
     # An IDENTIFIER won't show up in the string literal lookup table.
-    if str_info === nothing && x.parent.typ === CSTParser.x_Str
+    if str_info === nothing &&
+       (x.parent.typ === CSTParser.x_Str || x.parent.typ === CSTParser.x_Cmd)
         s.offset += x.fullspan
         return PTree(x, loc[1], loc[1], x.val)
     end
@@ -567,7 +578,6 @@ function p_macrocall(x, s)
             add_node!(t, n, s, join_lines = true)
             add_node!(t, Placeholder(0), s)
         elseif is_closer(n)
-            add_node!(t, TrailingComma(), s)
             add_node!(t, Placeholder(0), s)
             add_node!(t, n, s, join_lines = true)
         elseif CSTParser.is_comma(a) && i < length(x) && !is_punc(x.args[i+1])
diff --git a/src/print.jl b/src/print.jl
index e42edf76..c04f6060 100644
--- a/src/print.jl
+++ b/src/print.jl
@@ -60,7 +60,7 @@ function print_tree(io::IOBuffer, x::PTree, s::State)
         return
     end
 
-    ws = repeat(" ", x.indent)
+    ws = repeat(" ", max(x.indent, 0))
     for (i, n) in enumerate(x.nodes)
         if is_leaf(n)
             print_leaf(io, n, s)


Test patch:
diff --git a/test/runtests.jl b/test/runtests.jl
index 63446d06..fc14a469 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -1339,6 +1339,9 @@ end
               end\"""
         """
         @test fmt(str) == str
+
+        str = raw"""@test :(x`s`flag) == :(@x_cmd "s" "flag")"""
+        @test fmt(str) == str
     end
 
     @testset "comments" begin
@@ -2567,7 +2570,7 @@ end
                 body2
                 @Expr :break loop_exit2
                 body3
-            end,
+            end
         )"""
         @test fmt(str_, 4, 20) == str
 
@@ -2609,7 +2612,7 @@ end
           a,
           @g(
              b,
-             c,
+             c
           ),
           d,
         )"""
@@ -2972,6 +2975,15 @@ end
         end"""
         @test fmt(str, 4, 1) == str_
         @test fmt(str_) == str
+
+        str = """
+        let
+            # comment
+            list = [1, 2, 3]
+
+            body
+        end"""
+        @test fmt(str) == str
     end
 
     @testset "single newline at end of file" begin
@@ -2990,4 +3002,40 @@ end
         rm(f1)
     end
 
+    @testset "trailing comma - breaking cases" begin
+        # A trailing comma here is ambiguous
+        # It'll cause a parsing error.
+        str = """
+        gen2 = Iterators.filter(
+            x -> x[1] % 2 == 0 && x[2] % 2 == 0,
+            (x, y) for x = 1:10, y = 1:10
+        )"""
+        str_ = "gen2 = Iterators.filter(x -> x[1] % 2 == 0 && x[2] % 2 == 0, (x, y) for x = 1:10, y = 1:10)"
+
+        @test fmt(str_, 4, 80) == str
+
+        # With macro calls, a trailing comma can
+        # change the semantics of the macro.
+        #
+        # Keeping this in mind it should not be
+        # automatically added.
+        str = """
+        @func(
+            a,
+            b,
+            c
+        )"""
+        @test fmt("@func(a, b, c)", 4, 1) == str
+
+        str = """
+        @func(
+            a,
+            b,
+            c,
+        )"""
+        @test fmt("@func(a, b, c,)", 4, 1) == str
+
+    end
+
+
 end