Binders: be more selective about semicolon placement

Only place the semicolon onto a new line if the last expression
of the body is an if statement or an operator chain.

src/Nixfmt/Pretty.hs

@@ -126,7 +126,7 @@ instance Pretty Binder where
         = base $ group $ hcat selectors
                  <> nest 2 (hardspace <> pretty assign <> inner) <> pretty semicolon
         where
-          inner =
+        inner =
             case expr of
               -- Absorbable term. Always start on the same line, keep semicolon attatched
               (Term t) | isAbsorbable t -> hardspace <> prettyTermWide t
@@ -140,36 +140,36 @@ instance Pretty Binder where
               (Term _) -> group' False (line <> pretty expr)
               -- Function call
               -- Absorb if all arguments except the last fit into the line, start on new line otherwise
-              (Application f a) -> prettyApp hardline line line' mempty f a
+              (Application f a) -> prettyApp hardline line mempty mempty f a
               -- Absorb function declarations but only those with simple parameter(s)
               (Abstraction _ _ _) | isAbstractionWithAbsorbableTerm expr -> hardspace <> group expr
               -- With expression with absorbable body: Try to absorb and keep the semicolon attached, spread otherwise
-              (With _ _ _ (Term t)) | isAbsorbable t -> softline <> group' False (pretty expr <> softline')
+              (With _ _ _ (Term t)) | isAbsorbable t -> softline <> group expr
               -- Special case `//` operations to be more compact in some cases
               -- Case 1: two arguments, LHS is absorbable term, RHS fits onto the last line
               (Operation (Term t) (Ann [] TUpdate Nothing) b) | isAbsorbable t ->
-                group' False $ line <> group' True (prettyTermWide t) <> line <> pretty TUpdate <> hardspace <> pretty b <> line'
+                group' False $ line <> group' True (prettyTermWide t) <> line <> pretty TUpdate <> hardspace <> pretty b
               -- Case 2a: LHS fits onto first line, RHS is an absorbable term
               (Operation l (Ann [] TUpdate Nothing) (Term t)) | isAbsorbable t ->
-                group' False $ line <> pretty l <> line <> group' True (pretty TUpdate <> hardspace <> prettyTermWide t) <> line'
+                group' False $ line <> pretty l <> line <> group' True (pretty TUpdate <> hardspace <> prettyTermWide t)
               -- Case 2b: LHS fits onto first line, RHS is a function application
               (Operation l (Ann [] TUpdate Nothing) (Application f a)) ->
-                line <> (group $ pretty l) <> line <> prettyApp hardline (pretty TUpdate <> hardspace) line' hardline f a
+                line <> (group l) <> line <> prettyApp hardline (pretty TUpdate <> hardspace) mempty hardline f a
               -- Special case `++` operations to be more compact in some cases
               -- Case 1: two arguments, LHS is absorbable term, RHS fits onto the last line
               (Operation (Term t) (Ann [] TConcat Nothing) b) | isAbsorbable t ->
-                group' False $ line <> group' True (prettyTermWide t) <> line <> pretty TConcat <> hardspace <> pretty b <> line'
+                group' False $ line <> group' True (prettyTermWide t) <> line <> pretty TConcat <> hardspace <> pretty b
               -- Case 2a: LHS fits onto first line, RHS is an absorbable term
               (Operation l (Ann [] TConcat Nothing) (Term t)) | isAbsorbable t ->
-                group' False $ line <> pretty l <> line <> group' True (pretty TConcat <> hardspace <> prettyTermWide t) <> line'
+                group' False $ line <> pretty l <> line <> group' True (pretty TConcat <> hardspace <> prettyTermWide t)
               -- Case 2b: LHS fits onto first line, RHS is a function application
               (Operation l (Ann [] TConcat Nothing) (Application f a)) ->
-                line <> (group $ pretty l) <> line <> prettyApp hardline (pretty TConcat <> hardspace) line' hardline f a
+                line <> (group l) <> line <> prettyApp hardline (pretty TConcat <> hardspace) mempty hardline f a
               -- Everything else:
               -- If it fits on one line, it fits
               -- If it fits on one line but with a newline after the `=`, it fits (including semicolon)
               -- Otherwise, start on new line, expand fully (including the semicolon)
-              _ -> line <> group' False (pretty expr <> line')
+              _ -> line <> group expr
 
 -- Pretty a set
 -- while we already pretty eagerly expand sets with more than one element,
@@ -457,19 +457,24 @@ instance Pretty Expression where
           inPart = groupWithStart (Ann [] in_ Nothing) $ hardline
               -- Take our trailing and inject it between `in` and body
               <> pretty (concat binderComments ++ leading ++ convertTrailing trailing')
-              <> pretty expr <> hardline
+              <> pretty expr
 
     pretty (Assert assert cond semicolon expr)
         = base (pretty assert <> hardspace
           <> nest 2 (group cond) <> pretty semicolon)
           <> absorbSet expr
 
     pretty (If if_ cond then_ expr0 else_ expr1)
-        = base $ group $
+        = base $ group' False $
             -- `if cond then` if it fits on one line, otherwise `if\n  cond\nthen` (with cond indented)
             groupWithStart if_ (line <> nest 2 (pretty cond) <> line <> pretty then_)
             <> (surroundWith line $ nest 2 $ group expr0)
             <> pretty else_ <> absorbElse expr1
+            -- This trailing line' is a bit of a hack. It makes sure that the semicolon in binders gets placed onto
+            -- a new line if the items ends with a (multiline) if.
+            -- Normally this should only be the case when in binders as this might interfere with other syntax constructs,
+            -- but because our style always puts a new line after multiline Ifs it turns out to work just fine ^^
+            <> line'
 
     pretty (Abstraction (IDParameter param) colon body)
         = pretty param <> pretty colon <> absorbAbs 1 body
@@ -519,8 +524,8 @@ instance Pretty Expression where
             -- Extract comment before the first operand and move it out, to prevent force-expanding the expression
             (operationWithoutComment, comment') = mapFirstToken' (\(Ann leading token trailing') -> (Ann [] token trailing', leading)) operation
           in
-            pretty comment' <> (group $
-                (concat . map prettyOperation . (flatten Nothing)) operationWithoutComment)
+            pretty comment' <> (group' False $
+                ((concat . map prettyOperation . (flatten Nothing)) operationWithoutComment) <> line')
 
     pretty (MemberCheck expr qmark sel)
         = pretty expr <> softline

test/diff/apply/out.nix

@@ -133,8 +133,7 @@
           asdf = 1;
           # multiline
         }
-        argument
-    ;
+        argument;
 
     name3 =
       function arg
@@ -143,8 +142,7 @@
           # multiline
         }
         { qwer = 12345; }
-        argument
-    ;
+        argument;
   }
   {
     name4 =
@@ -153,8 +151,7 @@
           qwer = 12345;
           qwer2 = 54321;
         }
-        argument
-    ;
+        argument;
   }
   {
     option1 =
@@ -163,34 +160,30 @@
           qwer = 12345;
           qwer2 = 54321;
         }
-        lastArg
-    ;
+        lastArg;
 
     option2 =
       function arg { asdf = 1; }
         {
           qwer = 12345;
           qwer2 = 54321;
         }
-        lastArg
-    ;
+        lastArg;
 
     option3 =
       function arg { asdf = 1; }
         {
           qwer = 12345;
           qwer2 = 54321;
         }
-        lastArg
-    ;
+        lastArg;
   }
   # https://github.com/kamadorueda/alejandra/issues/372#issuecomment-1435083516
   {
     outputs =
       { utils }:
       # For each supported platform,
-      utils.lib.eachDefaultSystem (system: { })
-    ;
+      utils.lib.eachDefaultSystem (system: { });
   }
   {
     escapeSingleline = libStr.escape [
@@ -207,8 +200,7 @@
         [
           "''\${"
           "'''"
-        ]
-    ;
+        ];
     test =
       foo
         [
@@ -224,8 +216,7 @@
           1
           2
           3 # multiline
-        ]
-    ;
+        ];
     looooooooong =
       (toINI
         {
@@ -248,8 +239,7 @@
             aaaaaaaa
           ;
         }
-        sections
-    ;
+        sections;
   }
 
   # Test breakup behavior at different line lengths
@@ -347,7 +337,6 @@
         (lib.take 3)
         # Quote all entries
         (map (x: ''"'' + x + ''"''))
-      ]
-    ;
+      ];
   }
 ]

test/diff/attr_set/out.nix

@@ -117,8 +117,7 @@
         [
           # several
           items
-        ]
-    ;
+        ];
     a =
       [
         some
@@ -180,8 +179,7 @@
         secret-config.ssh-hosts
       // {
         foo = "bar";
-      }
-    ;
+      };
     programs.ssh.knownHosts2 =
       someStuff
       //
@@ -212,8 +210,7 @@
       )
       // {
         foo = "bar";
-      }
-    ;
+      };
     programs.ssh.knownHosts4 =
       someStuff
       // lib.mapAttrs (
@@ -253,8 +250,7 @@
             "${host_name}.lo.m-0.eu"
           ];
         })
-        secret-config.ssh-hosts
-    ;
+        secret-config.ssh-hosts;
     programs.ssh.knownHosts7 =
       someStuff # multiline
       // lib.mapAttrs (
@@ -278,8 +274,7 @@
             "${host_name}.lo.m-0.eu"
           ];
         })
-        secret-config.ssh-hosts
-    ;
+        secret-config.ssh-hosts;
     programs.ssh.knownHosts9 =
       {
         multi = 1;
@@ -294,8 +289,7 @@
             "${host_name}.lo.m-0.eu"
           ];
         }
-      )
-    ;
+      );
     programs.ssh.knownHosts10 =
       {
         multi = 1;
@@ -310,7 +304,6 @@
             "${host_name}.lo.m-0.eu"
           ];
         })
-        secret-config.ssh-hosts
-    ;
+        secret-config.ssh-hosts;
   }
 ]

test/diff/idioms_lib_1/out.nix

@@ -6,6 +6,5 @@
     msg:
     # Value to return
     x:
-    if pred then trace msg x else x
-  ;
+    if pred then trace msg x else x;
 }

test/diff/idioms_lib_2/out.nix

@@ -12,8 +12,7 @@ rec {
   id =
     # The value to return
     x:
-    x
-  ;
+    x;
 
   /* The constant function
 
@@ -30,8 +29,7 @@ rec {
     x:
     # Value to ignore
     y:
-    x
-  ;
+    x;
 
   /* Pipes a value through a list of functions, left to right.
 
@@ -67,8 +65,7 @@ rec {
     let
       reverseApply = x: f: f x;
     in
-    builtins.foldl' reverseApply val functions
-  ;
+    builtins.foldl' reverseApply val functions;
 
   # note please don’t add a function like `compose = flip pipe`.
   # This would confuse users, because the order of the functions
@@ -133,8 +130,7 @@ rec {
     x:
     # Right attribute set (higher precedence for equal keys)
     y:
-    x // y
-  ;
+    x // y;
 
   /* Flip the order of the arguments of a binary function.
 
@@ -146,8 +142,7 @@ rec {
   */
   flip =
     f: a: b:
-    f b a
-  ;
+    f b a;
 
   /* Apply function if the supplied argument is non-null.
 
@@ -162,8 +157,7 @@ rec {
     f:
     # Argument to check for null before passing it to `f`
     a:
-    if a == null then a else f a
-  ;
+    if a == null then a else f a;
 
   # Pull in some builtins not included elsewhere.
   inherit (builtins)
@@ -200,7 +194,10 @@ rec {
     let
       suffixFile = ../.version-suffix;
     in
-    if pathExists suffixFile then lib.strings.fileContents suffixFile else "pre-git"
+    if pathExists suffixFile then
+      lib.strings.fileContents suffixFile
+    else
+      "pre-git"
   ;
 
   /* Attempts to return the the current revision of nixpkgs and
@@ -225,8 +222,7 @@ rec {
 
   nixpkgsVersion =
     builtins.trace "`lib.nixpkgsVersion` is deprecated, use `lib.version` instead!"
-      version
-  ;
+      version;
 
   /* Determine whether the function is being called from inside a Nix
      shell.
@@ -406,8 +402,7 @@ rec {
         builtins.concatStringsSep ", " (builtins.map builtins.toString unexpected)
       } unexpected; valid ones: ${
         builtins.concatStringsSep ", " (builtins.map builtins.toString valid)
-      }"
-  ;
+      }";
 
   info = msg: builtins.trace "INFO: ${msg}";
 
@@ -479,8 +474,7 @@ rec {
           .${toString d}
       ;
     in
-    lib.concatMapStrings toHexDigit (toBaseDigits 16 i)
-  ;
+    lib.concatMapStrings toHexDigit (toBaseDigits 16 i);
 
   /* `toBaseDigits base i` converts the positive integer i to a list of its
      digits in the given base. For example:
@@ -508,6 +502,5 @@ rec {
     in
     assert (base >= 2);
     assert (i >= 0);
-    lib.reverseList (go i)
-  ;
+    lib.reverseList (go i);
 }