Repository: domluna/JuliaFormatter.jl
Pull request #224 (2020-04-17 01:43:48)

Indent standalone lazy and/or expressions after first arg
I'm defining this as a lazy and/or expression that's NOT a component of a larger expression.

ref #222 

For demonstration purposes the maximum margin is set to 1.

Examples:

```julia
a && b

->

a &&
    b
```

```julia
a && b && c

->

a &&
    b &&
    c
```

the testset has more examples.

Something I'm still pondering is whether to treat `||` and `&&` as the same operation in this case.

So instead of `a || b && c` being

```
a ||
    b &&
        c
```

it would be

```
a ||
    b &&
    c
```

but this may be too pessimistic, since with a larger margin, which is what it would be in practice,
the former would be

 ```
a ||
    b && c
```


StefanKarpinski: > Something I'm still pondering is whether to treat `||` and `&&` as the same operation in this case.

Maybe distinguish by precedence level? Which would argue against treating them the same.
domluna: it already 

> Maybe distinguish by precedence level? Which would argue against treating them the same.

That's currently what it does. It seems to only perhaps cause a worse format in really odd cases and there is another way to make it cleaner rather than mess with the precedence.

Patch:
diff --git a/docs/src/style.md b/docs/src/style.md
index 71eac00f..cae886bf 100644
--- a/docs/src/style.md
+++ b/docs/src/style.md
@@ -199,7 +199,7 @@ e1 :
 e2
 ```
 
-If nesting is required for a `=` binary operation, the RHS is placed on the following line.
+If nesting is required for a `=` binary operation, the RHS is placed on the following line and indented.
 
 ```julia
 foo() = body
@@ -210,6 +210,8 @@ foo() =
     body
 ```
 
+Lazy `&&` and `||` operations are nested according to [`is_standalone_shortcircuit`](@ref) rules.
+
 All arguments of a function call (applies to any opening/closing punctuation type) are nested
 if the expression exceeds the margin. The arguments are indented one level. 
 
diff --git a/src/fst.jl b/src/fst.jl
index 479c9e69..c3da858e 100644
--- a/src/fst.jl
+++ b/src/fst.jl
@@ -102,10 +102,6 @@ end
     (cst.typ === CSTParser.BinaryOpCall && cst[2].kind === Tokens.COLON) ||
     cst.typ === CSTParser.ColonOpCall
 
-@inline is_lazy_op(cst::CSTParser.EXPR) =
-    cst.typ === CSTParser.BinaryOpCall &&
-    (cst[2].kind === Tokens.LAZY_OR || cst[2].kind === Tokens.LAZY_AND)
-
 function is_multiline(fst::FST)
     fst.typ === CSTParser.StringH && return true
     if fst.typ === CSTParser.x_Str && fst[2].typ === CSTParser.StringH
@@ -128,13 +124,13 @@ function is_importer_exporter(fst::FST)
 end
 
 # f a function which returns a bool
-function parent_is(cst::CSTParser.EXPR, f; ignore_typs = [])
+function parent_is(cst::CSTParser.EXPR, valid; ignore = (n) -> false)
     p = cst.parent
     p === nothing && return false
-    while p !== nothing && p.typ in ignore_typs
+    while p !== nothing && ignore(p)
         p = p.parent
     end
-    f(p)
+    valid(p)
 end
 
 function contains_comment(fst::FST)
@@ -462,6 +458,7 @@ function is_gen(x::Union{CSTParser.EXPR,FST})
     x.typ === CSTParser.Generator && return true
     x.typ === CSTParser.Filter && return true
     x.typ === CSTParser.Flatten && return true
+    # x.typ === CSTParser.InvisBrackets && return true
     return false
 end
 
@@ -510,18 +507,10 @@ function nest_rhs(cst::CSTParser.EXPR)::Bool
     false
 end
 
-function flattenable(kind::Tokens.Kind)
-    kind === Tokens.AND && return true
-    kind === Tokens.OR && return true
-    kind === Tokens.LAZY_AND && return true
-    kind === Tokens.LAZY_OR && return true
-    kind === Tokens.RPIPE && return true
-    return false
-end
-flattenable(::Nothing) = false
-
 function op_kind(cst::CSTParser.EXPR)::Union{Nothing,Tokens.Kind}
-    if cst.typ === CSTParser.BinaryOpCall
+    if cst.typ === CSTParser.BinaryOpCall ||
+       cst.typ === CSTParser.Comparison ||
+       cst.typ === CSTParser.ChainOpCall
         return cst[2].kind
     elseif cst.typ === CSTParser.UnaryOpCall
         return cst[1].typ === CSTParser.OPERATOR ? cst[1].kind : cst[2].kind
@@ -530,3 +519,61 @@ function op_kind(cst::CSTParser.EXPR)::Union{Nothing,Tokens.Kind}
 end
 op_kind(::Nothing) = nothing
 op_kind(fst::FST) = fst.ref === nothing ? nothing : op_kind(fst.ref[])
+
+is_lazy_op(kind) = kind === Tokens.LAZY_AND || kind === Tokens.LAZY_OR
+
+"""
+    is_standalone_shortcircuit(cst::CSTParser.EXPR)
+
+Returns `true` if the `cst` is a short-circuit expression (uses `&&`, `||`)
+and is _standalone_, meaning it's not directly associated with another statement or
+expression.
+
+### Examples
+
+```julia
+# this IS a standalone short-circuit
+a && b
+
+# this IS NOT a standalone short-circuit
+if a && b
+end
+
+# this IS NOT a standalone short-circuit
+var = a && b
+
+# this IS NOT a standalone short-circuit
+@macro a && b
+
+# operation inside parenthesis IS NOT a standalone short-circuit
+# operation outside parenthesis IS a standalone short-circuit
+(a && b) && c
+```
+"""
+function is_standalone_shortcircuit(cst::CSTParser.EXPR)
+    kind = op_kind(cst)
+    is_lazy_op(kind) || return false
+
+    function valid(n)
+        n === nothing && return true
+        n.typ === CSTParser.InvisBrackets && return false
+        n.typ === CSTParser.MacroCall && return false
+        n.typ === CSTParser.Return && return false
+        n.typ === CSTParser.If && return false
+        n.typ === CSTParser.Block && nest_assignment(n.parent) && return false
+        n.typ === CSTParser.BinaryOpCall && nest_assignment(n) && return false
+        return true
+    end
+
+    function ignore(n::CSTParser.EXPR)
+        n.typ === CSTParser.InvisBrackets && return false
+        n.typ === CSTParser.If && return false
+        n.typ === CSTParser.Block && return false
+        n.typ === CSTParser.MacroCall && return false
+        n.typ === CSTParser.Return && return false
+        n.typ === CSTParser.BinaryOpCall && nest_assignment(n) && return false
+        return true
+    end
+
+    return parent_is(cst, valid, ignore = ignore)
+end
diff --git a/src/nest.jl b/src/nest.jl
index fcc89b6e..7d5e35f5 100644
--- a/src/nest.jl
+++ b/src/nest.jl
@@ -664,9 +664,12 @@ function n_binaryopcall!(ds::DefaultStyle, fst::FST, s::State)
         fst[i1] = Newline(length = fst[i1].len)
         cst = fst.ref[]
 
-        has_eq = CSTParser.defines_function(cst) || nest_assignment(cst)
-        has_arrow = cst[2].kind === Tokens.PAIR_ARROW || cst[2].kind === Tokens.ANON_FUNC
-        indent_nest = has_eq || has_arrow
+        indent_nest =
+            CSTParser.defines_function(cst) ||
+            nest_assignment(cst) ||
+            cst[2].kind === Tokens.PAIR_ARROW ||
+            cst[2].kind === Tokens.ANON_FUNC ||
+            is_standalone_shortcircuit(cst)
 
         if indent_nest
             s.line_offset = fst.indent + s.indent_size
@@ -783,6 +786,10 @@ function n_block!(ds::DefaultStyle, fst::FST, s::State; indent = -1)
         line_offset = s.line_offset
         indent >= 0 && (fst.indent = indent)
 
+        if fst.typ === CSTParser.ChainOpCall && is_standalone_shortcircuit(fst.ref[])
+            fst.indent += s.indent_size
+        end
+
         for (i, n) in enumerate(fst.nodes)
             if n.typ === NEWLINE
                 s.line_offset = fst.indent
diff --git a/src/passes.jl b/src/passes.jl
index 7ff9054f..40a79db8 100644
--- a/src/passes.jl
+++ b/src/passes.jl
@@ -1,17 +1,31 @@
 # FST passes/transforms
 
+function flattenable(kind::Tokens.Kind)
+    kind === Tokens.AND && return true
+    kind === Tokens.OR && return true
+    kind === Tokens.LAZY_AND && return true
+    kind === Tokens.LAZY_OR && return true
+    kind === Tokens.RPIPE && return true
+    return false
+end
+flattenable(::Nothing) = false
+
 """
 Flattens a binary operation call tree if the operation repeats 2 or more times.
 "a && b && c" will be transformed while "a && b" will not.
 """
 function flatten_binaryopcall(fst::FST; top = true)
     nodes = FST[]
+    kind = op_kind(fst)
+
+    # @info "a" top kind
 
     lhs = fst[1]
-    kind = op_kind(fst)
     rhs = fst[end]
-    lhs_same_op = op_kind(lhs) === kind
-    rhs_same_op = op_kind(rhs) === kind
+    lhs_kind = op_kind(lhs)
+    rhs_kind = op_kind(rhs)
+    lhs_same_op = lhs_kind === kind
+    rhs_same_op = rhs_kind === kind
 
     if top && !lhs_same_op && !rhs_same_op
         return nodes
@@ -45,8 +59,10 @@ function flatten_fst!(fst::FST)
             # possibly convert BinaryOpCall to ChainOpCall
             nnodes = flatten_binaryopcall(n)
             if length(nnodes) > 0
-                n.nodes = nnodes
                 n.typ = CSTParser.ChainOpCall
+                n.nodes = nnodes
+            else
+                flatten_fst!(n)
             end
         else
             flatten_fst!(n)
@@ -54,9 +70,6 @@ function flatten_fst!(fst::FST)
     end
 end
 
-is_pipe(n) = op_kind(n) === Tokens.RPIPE
-
-
 """
     pipe_to_function_call_pass!(fst::FST)
 
@@ -65,7 +78,7 @@ Rewrites `x |> f` to `f(x)`.
 function pipe_to_function_call_pass!(fst::FST)
     is_leaf(fst) && return
 
-    if is_pipe(fst)
+    if op_kind(fst) === Tokens.RPIPE
         fst.nodes = pipe_to_function_call(fst)
         fst.typ = CSTParser.Call
         return
@@ -74,7 +87,7 @@ function pipe_to_function_call_pass!(fst::FST)
     for n in fst.nodes
         if is_leaf(n)
             continue
-        elseif is_pipe(n)
+        elseif op_kind(n) === Tokens.RPIPE
             n.nodes = pipe_to_function_call(n)
             n.typ = CSTParser.Call
         else
diff --git a/src/pretty.jl b/src/pretty.jl
index 220cdd36..99559327 100644
--- a/src/pretty.jl
+++ b/src/pretty.jl
@@ -1718,12 +1718,7 @@ function p_generator(ds::DefaultStyle, cst::CSTParser.EXPR, s::State)
             if a.kind === Tokens.FOR && parent_is(
                 a,
                 is_iterable,
-                ignore_typs = (
-                    CSTParser.InvisBrackets,
-                    CSTParser.Generator,
-                    CSTParser.Flatten,
-                    CSTParser.Filter,
-                ),
+                ignore = n -> is_gen(n) || n.typ === CSTParser.InvisBrackets,
             )
                 add_node!(t, Placeholder(1), s)
             else
diff --git a/src/styles/yas.jl b/src/styles/yas.jl
index b2f26768..35554cc7 100644
--- a/src/styles/yas.jl
+++ b/src/styles/yas.jl
@@ -269,6 +269,7 @@ end
 
 function p_generator(ys::YASStyle, cst::CSTParser.EXPR, s::State)
     t = FST(cst, nspaces(s))
+
     for (i, a) in enumerate(cst)
         # n = a.typ === CSTParser.BinaryOpCall ? pretty(ys, a, s, nonest = true) :
         #     pretty(ys, a, s)
@@ -277,12 +278,7 @@ function p_generator(ys::YASStyle, cst::CSTParser.EXPR, s::State)
             incomp = parent_is(
                 a,
                 is_iterable,
-                ignore_typs = (
-                    CSTParser.InvisBrackets,
-                    CSTParser.Generator,
-                    CSTParser.Flatten,
-                    CSTParser.Filter,
-                ),
+                ignore = n -> is_gen(n) || n.typ === CSTParser.InvisBrackets,
             )
 
             if is_block(cst[i-1])


Test patch:
diff --git a/test/default_style.jl b/test/default_style.jl
index b7112671..0d4445ba 100644
--- a/test/default_style.jl
+++ b/test/default_style.jl
@@ -875,7 +875,7 @@
         str_ = "__module__ == Main || @warn \"Replacing docs for `\$b :: \$sig` in module `\$(__module__)`\""
         str = """
         __module__ == Main ||
-        @warn \"Replacing docs for `\$b :: \$sig` in module `\$(__module__)`\""""
+            @warn \"Replacing docs for `\$b :: \$sig` in module `\$(__module__)`\""""
         @test fmt(str_, 4, length(str_) - 1) == str
     end
 
@@ -2779,23 +2779,6 @@
                :mesh_dim => Cint(3),)"""
         @test fmt(str_, 4, 80) == str
 
-        str = """
-        begin
-            a &&
-            b
-            a ||
-            b
-        end"""
-        @test fmt(str, 4, 1) == str
-
-        str = """
-        begin
-            a &&
-            b ||
-            c &&
-            d
-        end"""
-        @test fmt("begin\n a && b || c && d\nend", 4, 1) == str
 
         str = """
         func(
@@ -3233,7 +3216,7 @@
         str_ = "(var1,var2) && var3"
         str = """
         (var1, var2) &&
-        var3"""
+            var3"""
         @test fmt(str_, 4, 19) == str
         @test fmt(str_, 4, 15) == str
 
@@ -3249,7 +3232,7 @@
             var1,
             var2,
         ) &&
-        var3"""
+            var3"""
         @test fmt(str_, 4, 1) == str
 
         str_ = "(var1,var2) ? (var3,var4) : var5"
@@ -4653,4 +4636,220 @@ some_function(
         @test fmt(str_) == str
     end
 
+    @testset "standalone lazy expr indent" begin
+        str = """
+        begin
+          a &&
+            b
+          a ||
+            b
+        end"""
+        @test fmt(str, 2, 1) == str
+
+        str_ = """
+        begin
+         a && b || c && d
+        end"""
+
+        str = """
+        begin
+            a && b ||
+                c && d
+        end"""
+        @test fmt(str_, 4, 19) == str
+
+        str = """
+        begin
+            a && b ||
+                c &&
+                    d
+        end"""
+        @test fmt(str_, 4, 13) == str
+
+        str = """
+        begin
+            a &&
+                b ||
+                c &&
+                    d
+        end"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        begin
+        a || (b && c && d)
+        end"""
+
+        str = """
+        begin
+            a ||
+                (
+                    b &&
+                    c &&
+                    d
+                )
+        end"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        begin
+        (a && b && c) || d
+        end"""
+
+        str = """
+        begin
+            (
+                a &&
+                b &&
+                c
+            ) ||
+                d
+        end"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        begin
+         a || b && c || d
+        end"""
+
+        str = """
+        begin
+            a ||
+                b && c ||
+                d
+        end"""
+        @test fmt(str_, 4, 19) == str
+
+        str = """
+        begin
+            a ||
+                b &&
+                    c ||
+                d
+        end"""
+        @test fmt(str_, 4, 16) == str
+
+        # Due to parsing but in practice this
+        # case that will never come up and
+        # can be fixed by adding parenthesis.
+        str_ = """
+        begin
+         a && b || c || d || e
+        end"""
+        str = """
+        begin
+            a &&
+                b ||
+                    c ||
+                    d ||
+                    e
+        end"""
+        @test_broken fmt(str_, 4, 1) == str
+
+        str_ = """
+        begin
+         a || b && c && d && e
+        end"""
+        str = """
+        begin
+            a ||
+                b &&
+                    c &&
+                    d &&
+                    e
+        end"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        if aa && bb
+        end
+
+        if (aa && bb)
+        end"""
+
+        str = """
+        if aa &&
+           bb
+        end
+
+        if (
+            aa &&
+            bb
+        )
+        end"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        if aa || bb || cc
+        end
+
+        if (aa || bb || cc)
+        end"""
+
+        str = """
+        if aa ||
+           bb ||
+           cc
+        end
+
+        if (
+            aa ||
+            bb ||
+            cc
+        )
+        end"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """var = a && b"""
+        str = """
+        var =
+            a &&
+            b"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """var() = a && b"""
+        str = """
+        var() =
+            a &&
+            b"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """var = a || b || c"""
+        str = """
+        var =
+            a ||
+            b ||
+            c"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """var() = a || b || c"""
+        str = """
+        var() =
+            a ||
+            b ||
+            c"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        @hello arg1 && arg2 && return arg3"""
+        str = """
+        @hello arg1 &&
+               arg2 &&
+               return arg3"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        @hello arg1 || return arg2"""
+        str = """
+        @hello arg1 ||
+               return arg2"""
+        @test fmt(str_, 4, 1) == str
+
+        str_ = """
+        return arg1 || arg2"""
+        str = """
+        return arg1 ||
+               arg2"""
+        @test fmt(str_, 4, 1) == str
+    end
 end
diff --git a/test/files/Docs.jl b/test/files/Docs.jl
index e9d74d4b..f7b5b5db 100644
--- a/test/files/Docs.jl
+++ b/test/files/Docs.jl
@@ -221,7 +221,7 @@ function doc!(__module__::Module, b::Binding, str::DocStr, @nospecialize sig = U
         # that over-writing a docstring *may* have been accidental.  The warning
         # is suppressed for symbols in Main, for interactive use (#23011).
         __module__ == Main ||
-        @warn "Replacing docs for `$b :: $sig` in module `$(__module__)`"
+            @warn "Replacing docs for `$b :: $sig` in module `$(__module__)`"
     else
         # The ordering of docstrings for each Binding is defined by the order in which they
         # are initially added. Replacing a specific docstring does not change it's ordering.
diff --git a/test/files/reverse.jl b/test/files/reverse.jl
index 46976f23..d5d5a807 100644
--- a/test/files/reverse.jl
+++ b/test/files/reverse.jl
@@ -51,7 +51,8 @@ unwrapquote(x::QuoteNode) = x.value
 is_literal_getproperty(ex) =
     (
         iscall(ex, Base, :getproperty) ||
-        iscall(ex, Core, :getfield) || iscall(ex, Base, :getfield)
+        iscall(ex, Core, :getfield) ||
+        iscall(ex, Base, :getfield)
     ) && ex.args[3] isa Union{QuoteNode,Integer}
 
 function instrument_getproperty!(ir, v, ex)