Repository: domluna/JuliaFormatter.jl (julia)

Issue:
Strange line breaks for list comprehensions?
I noticed that currently (v0.10.2) the formatter changes list comprehensions that go beyond the line limit in a suprising (to me) way

here's an example, where the input code has manually broken the list comprehension to fit within the 92 char line limit
```julia
julia> str = """
       function f()
           for group_serv in group_services
               contributing_services = Vector{Service}([
                   serv for serv in get_components(Service, system)
                   if serv.ext["reserve_zone"] == group_serv.ext["reserve_zone"]
               ])
           end
       end
       """;
```
which the formatter then changes to:
```julia
julia> println(format_text(str))
function f()
    for group_serv in group_services
        contributing_services = Vector{Service}([
            serv
            for
            serv in get_components(Service, system) if
            serv.ext["reserve_zone"] == group_serv.ext["reserve_zone"]
        ])
    end
end
```

I noticed this in the context of running BlueStyle, i.e. i get this same result using `format_text(str, BlueStyle())`, but since it affects `DefaultStyle()` too i thought i'd open a general issue.

If the current behaviour is intended and fine for DefaultStyle, then we'd just want to special-case BlueStyle to not do this :)

Pull request:
fixes #314 

This significantly improves line breaks for comprehension expressions by adapting the nesting/unnesting flow used for conditional expressions. That is it's initially nest from right-to-left and then an unnesting pass is used to determine whether lines can be joined.

As a side line breaks for conditionals were also slightly improved.

Examples from linked issue

**Before this PR**

```julia
julia> println(format_text(str))
function f()
    for group_serv in group_services
        contributing_services = Vector{Service}([
            serv
            for
            serv in get_components(Service, system) if
            serv.ext["reserve_zone"] == group_serv.ext["reserve_zone"]
        ])
    end
end

moi_index_to_consecutive_index = Dict(
    moi_index => consecutive_index 
    for 
    (consecutive_index, moi_index) in 
    enumerate(MOI.get(d.m, MOI.ListOfVariableIndices()))
)

```

**This PR**

```julia

julia> println(format_text(str))
function f()
    for group_serv in group_services
        contributing_services = Vector{Service}([
            serv for serv in get_components(Service, system) if
            serv.ext["reserve_zone"] == group_serv.ext["reserve_zone"]
        ])
    end
end


julia> format_text(
           "moi_index_to_consecutive_index = Dict(moi_index => consecutive_index for (consecutive_index, moi_index) in enumerate(MOI.get(d.m, MOI.ListOfVariableIndices())))";
           margin = 80,
       ) |> println
moi_index_to_consecutive_index = Dict(
    moi_index => consecutive_index for (consecutive_index, moi_index) in
    enumerate(MOI.get(d.m, MOI.ListOfVariableIndices()))
)

julia>

```

Patch:
diff --git a/src/styles/default/nest.jl b/src/styles/default/nest.jl
index c4f3287dd..95cd74ac8 100644
--- a/src/styles/default/nest.jl
+++ b/src/styles/default/nest.jl
@@ -325,7 +325,9 @@ n_typedvcat!(style::S, fst::FST, s::State) where {S<:AbstractStyle} =
 
 function n_comprehension!(ds::DefaultStyle, fst::FST, s::State; indent = -1)
     style = getstyle(ds)
+    line_offset = s.line_offset
     line_margin = s.line_offset + length(fst) + fst.extra_margin
+
     closer = is_closer(fst[end])
     if closer && (line_margin > s.opts.margin || must_nest(fst))
         idx = findfirst(n -> n.typ === PLACEHOLDER, fst.nodes)
@@ -372,18 +374,75 @@ n_comprehension!(style::S, fst::FST, s::State; indent = -1) where {S<:AbstractSt
 n_typedcomprehension!(style::S, fst::FST, s::State) where {S<:AbstractStyle} =
     n_typedcomprehension!(DefaultStyle(style), fst, s)
 
-@inline n_generator!(ds::DefaultStyle, fst::FST, s::State) =
-    n_comprehension!(ds, fst, s, indent = fst.indent)
+function n_generator!(ds::DefaultStyle, fst::FST, s::State; indent = -1)
+    style = getstyle(ds)
+    line_margin = s.line_offset + length(fst) + fst.extra_margin
+
+    if line_margin > s.opts.margin || must_nest(fst)
+        line_offset = s.line_offset
+        fst.indent = s.line_offset
+        phs = reverse(findall(n -> n.typ === PLACEHOLDER, fst.nodes))
+        for (i, idx) in enumerate(phs)
+            if i == 1
+                fst[idx] = Newline(length = fst[idx].len)
+            else
+                nidx = phs[i-1]
+                l1 = sum(length.(fst[1:idx-1]))
+                l2 = sum(length.(fst[idx:nidx-1]))
+                width = line_offset + l1 + l2
+                if must_nest(fst) || width > s.opts.margin
+                    fst[idx] = Newline(length = fst[idx].len)
+                end
+            end
+        end
+
+        for (i, n) in enumerate(fst.nodes)
+            if n.typ === NEWLINE
+                s.line_offset = fst.indent
+            elseif i == length(fst.nodes)
+                nest!(style, n, s)
+            else
+                n.extra_margin = 1
+                nest!(style, n, s)
+            end
+        end
+
+        s.line_offset = line_offset
+        for (i, n) in enumerate(fst.nodes)
+            if n.typ === NEWLINE && !is_comment(fst[i+1]) && !is_comment(fst[i-1])
+                # +1 for newline to whitespace conversion
+                width = s.line_offset + 1
+                w, _ = length_to(fst, (NEWLINE,), start = i + 1)
+                width += w
+                # @info "" s.line_offset w width fst[i-1].typ fst[i-1].val width <= s.opts.margin
+                if width <= s.opts.margin
+                    fst[i] = Whitespace(1)
+                    s.line_offset += length(fst[i])
+                else
+                    s.line_offset = fst.indent
+                end
+            else
+                walk(increment_line_offset!, fst[i], s)
+            end
+        end
+
+        s.line_offset = line_offset
+        walk(increment_line_offset!, fst, s)
+    else
+        nest!(style, fst.nodes, s, fst.indent, extra_margin = fst.extra_margin)
+    end
+end
+
 n_generator!(style::S, fst::FST, s::State) where {S<:AbstractStyle} =
     n_generator!(DefaultStyle(style), fst, s)
 
 @inline n_filter!(ds::DefaultStyle, fst::FST, s::State) =
-    n_comprehension!(ds, fst, s, indent = fst.indent)
+    n_generator!(ds, fst, s, indent = fst.indent)
 n_filter!(style::S, fst::FST, s::State) where {S<:AbstractStyle} =
     n_filter!(DefaultStyle(style), fst, s)
 
 @inline n_flatten!(ds::DefaultStyle, fst::FST, s::State) =
-    n_comprehension!(ds, fst, s, indent = fst.indent)
+    n_generator!(ds, fst, s, indent = fst.indent)
 n_flatten!(style::S, fst::FST, s::State) where {S<:AbstractStyle} =
     n_flatten!(DefaultStyle(style), fst, s)
 
@@ -491,7 +550,7 @@ function n_conditionalopcall!(ds::DefaultStyle, fst::FST, s::State)
                     s.line_offset = fst.indent
                 end
             else
-                s.line_offset += length(fst[i])
+                walk(increment_line_offset!, fst[i], s)
             end
         end
 
diff --git a/src/styles/default/pretty.jl b/src/styles/default/pretty.jl
index 409baa521..110a11de2 100644
--- a/src/styles/default/pretty.jl
+++ b/src/styles/default/pretty.jl
@@ -1960,7 +1960,7 @@ end
 p_row(style::S, cst::CSTParser.EXPR, s::State) where {S<:AbstractStyle} =
     p_row(DefaultStyle(style), cst, s)
 
-# Generator/Filter
+# Generator/Filter/Flatten
 function p_generator(ds::DefaultStyle, cst::CSTParser.EXPR, s::State)
     style = getstyle(ds)
     t = FST(cst, nspaces(s))
@@ -1968,10 +1968,12 @@ function p_generator(ds::DefaultStyle, cst::CSTParser.EXPR, s::State)
     for (i, a) in enumerate(cst)
         n = pretty(style, a, s)
         if a.typ === CSTParser.KEYWORD
-            if !has_for_kw && a.kind === Tokens.FOR
+            if a.kind === Tokens.FOR
                 has_for_kw = true
             end
 
+            # for keyword can only be on the following line
+            # if this expression is within an iterable expression
             if a.kind === Tokens.FOR && parent_is(
                 a,
                 is_iterable,


Test patch:
diff --git a/test/default_style.jl b/test/default_style.jl
index 26ab389d7..d967f4ef2 100644
--- a/test/default_style.jl
+++ b/test/default_style.jl
@@ -3179,6 +3179,18 @@
         var5"""
         @test fmt(str_, 4, 14) == str
 
+        str = """
+        (
+            var1,
+            var2,
+        ) ?
+        (
+            var3,
+            var4,
+        ) : var5"""
+        @test fmt(str_, 4, 13) == str
+        @test fmt(str_, 4, 8) == str
+
         str = """
         (
             var1,
@@ -3189,7 +3201,7 @@
             var4,
         ) :
         var5"""
-        @test fmt(str_, 4, 13) == str
+        @test fmt(str_, 4, 7) == str
 
         str = """
         (var1, var2) ? (var3, var4) :
@@ -3634,10 +3646,16 @@
 
     @testset "comphrehensions types" begin
         str_ = "var = (x, y) for x = 1:10, y = 1:10"
+        str = """
+        var = (x, y) for x = 1:10,
+        y = 1:10"""
+        @test fmt(str_, 4, length(str_) - 1) == str
+        @test fmt(str_, 4, 26) == str
+
         str = """
         var = (x, y) for
         x = 1:10, y = 1:10"""
-        @test fmt(str_, 4, length(str_) - 1) == str
+        @test fmt(str_, 4, 25) == str
         @test fmt(str_, 4, 18) == str
 
         str = """
@@ -3656,8 +3674,8 @@
         str = """
         begin
             weights = Dict(
-                (file, i) => w for (file, subject) in subjects
-                for (i, w) in enumerate(weightfn.(eachrow(subject.events)))
+                (file, i) => w for (file, subject) in subjects for
+                (i, w) in enumerate(weightfn.(eachrow(subject.events)))
             )
         end"""
         @test fmt(str_, 4, 90) == str
@@ -3666,8 +3684,7 @@
         begin
             weights = Dict(
                 (file, i) => w for (file, subject) in subjects
-                for
-                (i, w) in
+                for (i, w) in
                 enumerate(weightfn.(eachrow(subject.events)))
             )
         end"""
@@ -3676,9 +3693,8 @@
         str = """
         begin
             weights = Dict(
-                (file, i) => w
-                for (file, subject) in subjects
-                for
+                (file, i) => w for
+                (file, subject) in subjects for
                 (i, w) in enumerate(
                     weightfn.(eachrow(subject.events)),
                 )
diff --git a/test/files/reverse.jl b/test/files/reverse.jl
index 94804ef56..c9a530b57 100644
--- a/test/files/reverse.jl
+++ b/test/files/reverse.jl
@@ -60,8 +60,7 @@ function instrument_getproperty!(ir, v, ex)
     (
         ir[v] =
             xcall(Zygote, :literal_getproperty, ex.args[2], Val(unwrapquote(ex.args[3])))
-    ) :
-    ex
+    ) : ex
 end
 
 is_literal_getindex(ex) =
@@ -90,8 +89,7 @@ function instrument_iterate!(ir, v, ex)
             Val(unwrapquote(ex.args[3])),
             ex.args[4:end]...,
         )
-    ) :
-    ex
+    ) : ex
 end
 
 function instrument_literals!(ir, v, ex)