Improve group handling for bindings and inherit

There is now the choice whether to pull whitespace just before/at the start of a group
in or out. This allows to fix an outstanding issue with inherit statements, and is also
used in the bindings format

src/Nixfmt/Predoc.hs

@@ -15,6 +15,7 @@ module Nixfmt.Predoc
     , hcat
     , base
     , group
+    , group'
     , nest
     , softline'
     , line'
@@ -60,8 +61,14 @@ data Spacing
 
 data DocAnn
     -- | Node Group docs indicates either all or none of the Spaces and Breaks
-    -- in docs should be converted to line breaks.
-    = Group
+    -- in docs should be converted to line breaks. This does not affect softlines,
+    -- those will be expanded only as necessary and with a lower priority.
+    --
+    -- The boolean argument determines how to handle whitespace directly before the
+    -- group or at the start of the group. By default (False), it gets pulled out
+    -- in front of the group, which is what you want in most cases. If set to True,
+    -- whitespace before the group will be pulled in instead.
+    = Group Bool
     -- | Node (Nest n) doc indicates all line starts in doc should be indented
     -- by n more spaces than the surrounding Base.
     | Nest Int
@@ -100,8 +107,15 @@ text :: Text -> Doc
 text "" = []
 text t  = [Text t]
 
+-- | Group document elements together (see Node Group documentation)
+-- Any whitespace at the start of the group will get pulled out in front of it.
 group :: Pretty a => a -> Doc
-group = pure . Node Group . pretty
+group = pure . Node (Group False) . pretty
+
+-- | Group document elements together (see Node Group documentation)
+-- Any whitespace directly before the group will be pulled into it.
+group' :: Pretty a => a -> Doc
+group' = pure . Node (Group True) . pretty
 
 -- | @nest n doc@ sets the indentation for lines in @doc@ to @n@ more than the
 -- indentation of the part before it. This is based on the actual indentation of
@@ -200,8 +214,12 @@ mergeLines (x : xs)                     = x : mergeLines xs
 
 moveLinesIn :: Doc -> Doc
 moveLinesIn [] = []
+-- Move space before Nest in
 moveLinesIn (Spacing l : Node (Nest level) xs : ys) =
-    Node (Nest level) (Spacing l : moveLinesIn xs) : moveLinesIn ys
+    Node (Nest level) (moveLinesIn (Spacing l : xs)) : moveLinesIn ys
+-- Move space before (Group True) in
+moveLinesIn (Spacing l : Node (Group True) xs : ys) =
+    Node (Group False) (moveLinesIn (Spacing l : xs)) : moveLinesIn ys
 
 moveLinesIn (Node ann xs : ys) =
     Node ann (moveLinesIn xs) : moveLinesIn ys
@@ -255,16 +273,17 @@ firstLineFits targetWidth maxWidth docs = go maxWidth docs
           go c (Text t : xs)            = go (c - textWidth t) xs
           go c (Spacing Hardspace : xs) = go (c - 1) xs
           go c (Spacing _ : _)          = maxWidth - c <= targetWidth
-          go c (Node Group ys : xs)     =
+          go c (Node (Group _) ys : xs)     =
               case fits (c - firstLineWidth xs) ys of
                    Nothing -> go c (ys ++ xs)
                    Just t  -> go (c - textWidth t) xs
 
           go c (Node _ ys : xs)         = go c (ys ++ xs)
 
--- |
+-- | A document element with target indentation
 data Chunk = Chunk Int DocE
 
+-- | Create `n` newlines and `i` spaces
 indent :: Int -> Int -> Text
 indent n i = Text.replicate n "\n" <> Text.replicate i " "
 
@@ -275,12 +294,17 @@ unChunk (Chunk _ doc) = doc
 -- cc   Current Column
 -- ci   Current Indentation
 -- ti   Target Indentation
+--        an indent only changes the target indentation at first.
+--        Only for the tokens starting on the next line the current
+--        indentation will match the target indentation.
 layoutGreedy :: Int -> Doc -> Text
-layoutGreedy tw doc = Text.concat $ go 0 0 [Chunk 0 $ Node Group doc]
-    where go _ _ [] = []
+layoutGreedy tw doc = Text.concat $ go 0 0 [Chunk 0 $ Node (Group False) doc]
+    where go :: Int -> Int -> [Chunk] -> [Text]
+          go _ _ [] = []
           go cc ci (Chunk ti x : xs) = case x of
             Text t               -> t : go (cc + textWidth t) ci xs
 
+            -- This code treats whitespace as "expanded"
             Spacing Break        -> indent 1 ti : go ti ti xs
             Spacing Space        -> indent 1 ti : go ti ti xs
             Spacing Hardspace    -> " "         : go (cc + 1) ci xs
@@ -300,7 +324,11 @@ layoutGreedy tw doc = Text.concat $ go 0 0 [Chunk 0 $ Node Group doc]
 
             Node (Nest l) ys     -> go cc ci $ map (Chunk (ti + l)) ys ++ xs
             Node Base ys         -> go cc ci $ map (Chunk ci) ys ++ xs
-            Node Group ys        ->
+            Node (Group _) ys    ->
+                -- Does the group (plus whatever comes after it on that line) fit in one line?
+                -- This is where treating whitespace as "compact" happens
                 case fits (tw - cc - firstLineWidth (map unChunk xs)) ys of
+                     -- Dissolve the group by mapping its members to the target indentation
+                     -- This also implies that whitespace in there will now be rendered "expanded"
                      Nothing     -> go cc ci $ map (Chunk ti) ys ++ xs
                      Just t      -> t : go (cc + textWidth t) ci xs

src/Nixfmt/Pretty.hs

@@ -17,7 +17,7 @@ import qualified Data.Text as Text
   (dropEnd, empty, init, isInfixOf, last, null, strip, takeWhile)
 
 import Nixfmt.Predoc
-  (Doc, Pretty, base, emptyline, group, hardline, hardspace, hcat, line, line',
+  (Doc, Pretty, base, emptyline, group, group', hardline, hardspace, hcat, line, line',
   nest, newline, pretty, sepBy, softline, softline', text, textWidth)
 import Nixfmt.Types
   (Ann(..), Binder(..), Expression(..), File(..), Leaf, ParamAttr(..),
@@ -78,8 +78,11 @@ instance Pretty Binder where
 
     -- `inherit (foo) bar` statement
     pretty (Inherit inherit source ids semicolon)
-        = base $ group (pretty inherit <> hardspace
-                 <> nest 2 ((pretty source) <> line <> sepBy line ids <> line' <> pretty semicolon))
+        = base $ group (pretty inherit <> hardspace <> nest 2 (
+            (group' (line <> pretty source)) <> line
+                <> sepBy line ids
+                <> line' <> pretty semicolon
+        ))
 
     -- `foo = bar`
     pretty (Assignment selectors assign expr semicolon)
@@ -88,17 +91,22 @@ instance Pretty Binder where
         where
           inner =
             case expr of
-              -- Function declaration / If statement / Let binding
-              -- If it is multi-line, force it into a new line with indentation, semicolon on separate line
-              (Abstraction _ _ _) -> line <> pretty expr <> line' <> pretty semicolon
-              (If _ _ _ _ _ _)    -> line <> pretty expr <> line' <> pretty semicolon
-              (Let _ _ _ _)       -> line <> pretty expr <> line' <> pretty semicolon
-              -- Term
-              -- Absorb and keep the semicolon attached if possible
-              (Term t) -> (if isAbsorbable t then hardspace else softline) <> group expr <> pretty semicolon
-              -- Everything else
+              -- Absorbable term. Always start on the same line, keep semicolon attatched
+              (Term t) | isAbsorbable t -> hardspace <> group expr <> pretty semicolon
+              -- Non-absorbable term
+              -- If it is multi-line, force it to start on a new line with indentation
+              (Term _) -> group' (line <> pretty expr) <> pretty semicolon
+              -- Function calls and with expressions
               -- Try to absorb and keep the semicolon attached, spread otherwise
-              _ -> softline <> group (pretty expr <> softline' <> pretty semicolon)
+              (Application _ _) -> group (softline <> pretty expr <> softline' <> pretty semicolon)
+              (With _ _ _ _) -> group (softline <> pretty expr <> softline' <> pretty semicolon)
+              -- Special case `//` operator to treat like an absorbable term
+              (Operation _ (Ann TUpdate _ _) _) -> group (softline <> pretty expr <> softline' <> pretty semicolon)
+              -- 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)
+              _ -> group (line <> pretty expr <> line' <> pretty semicolon)
 
 
 -- | Pretty print a term without wrapping it in a group.

test/diff/attr_set/in.nix

@@ -57,4 +57,41 @@
 
 
   }
+  {
+    x =
+      {
+        foo = 1;
+        bar = 2;
+        # multiline
+      }
+      .${x}
+      ;
+      y = # more multiline
+      {
+        foo = 1;
+        bar = 2;
+        # multiline
+      }
+      .${x}
+      ;
+    z = functionCall {
+      # multi
+      #line
+    } [
+      # several
+      items
+    ];
+    a = [
+      some flags # multiline
+    ] ++ [ short ] ++ [
+      more stuff # multiline
+    ] ++ (if foo then [ bar ] else [baz ]) ++ [] ++
+    (optionals condition [more items]);
+    b = with pkgs; [
+      a
+      lot
+      of
+      packages
+    ];
+  }
 ]

test/diff/attr_set/out.nix

@@ -78,4 +78,46 @@
       # f
 
   }
+  {
+    x =
+      {
+        foo = 1;
+        bar = 2;
+          # multiline
+      }.${x};
+    y = # more multiline
+      {
+        foo = 1;
+        bar = 2;
+          # multiline
+      }.${x};
+    z = functionCall {
+      # multi
+      #line
+    } [
+      # several
+      items
+    ];
+    a =
+      [
+        some
+        flags # multiline
+      ] ++ [ short ] ++ [
+        more
+        stuff # multiline
+      ] ++ (if foo then
+        [ bar ]
+      else
+        [ baz ]) ++ [ ] ++ (optionals condition [
+          more
+          items
+        ])
+      ;
+    b = with pkgs; [
+      a
+      lot
+      of
+      packages
+    ];
+  }
 ]

test/diff/idioms_lib_2/out.nix

@@ -105,25 +105,28 @@ rec {
     ;
 
     # bitwise “and”
-  bitAnd = builtins.bitAnd or (import ./zip-int-bits.nix (a: b:
-    if a == 1 && b == 1 then
-      1
-    else
-      0));
+  bitAnd =
+    builtins.bitAnd or (import ./zip-int-bits.nix (a: b:
+      if a == 1 && b == 1 then
+        1
+      else
+        0));
 
     # bitwise “or”
-  bitOr = builtins.bitOr or (import ./zip-int-bits.nix (a: b:
-    if a == 1 || b == 1 then
-      1
-    else
-      0));
+  bitOr =
+    builtins.bitOr or (import ./zip-int-bits.nix (a: b:
+      if a == 1 || b == 1 then
+        1
+      else
+        0));
 
     # bitwise “xor”
-  bitXor = builtins.bitXor or (import ./zip-int-bits.nix (a: b:
-    if a != b then
-      1
-    else
-      0));
+  bitXor =
+    builtins.bitXor or (import ./zip-int-bits.nix (a: b:
+      if a != b then
+        1
+      else
+        0));
 
     # bitwise “not”
   bitNot = builtins.sub (-1);
@@ -210,7 +213,9 @@ rec {
     ## nixpkgs version strings
 
     # Returns the current full nixpkgs version number.
-  version = release + versionSuffix;
+  version =
+    release + versionSuffix
+    ;
 
     # Returns the current nixpkgs release number as string.
   release = lib.strings.fileContents ../.version;
@@ -261,7 +266,9 @@ rec {
 
        Type: inNixShell :: bool
     */
-  inNixShell = builtins.getEnv "IN_NIX_SHELL" != "";
+  inNixShell =
+    builtins.getEnv "IN_NIX_SHELL" != ""
+    ;
 
     ## Integer operations
 

test/diff/idioms_lib_3/out.nix

@@ -423,8 +423,10 @@ rec {
               "''\${"
               "'''"
             ];
-            singlelineResult = ''"''
-              + concatStringsSep "\\n" (map escapeSingleline lines) + ''"'';
+            singlelineResult =
+              ''"'' + concatStringsSep "\\n" (map escapeSingleline lines)
+              + ''"''
+              ;
             multilineResult =
               let
                 escapedLines = map escapeMultiline lines;

test/diff/idioms_nixos_1/out.nix

@@ -277,15 +277,19 @@ in
     (mkIf (!config.boot.isContainer) {
       system.build = { inherit kernel; };
 
-      system.modulesTree = [ kernel ] ++ config.boot.extraModulePackages;
+      system.modulesTree =
+        [ kernel ] ++ config.boot.extraModulePackages
+        ;
 
         # Implement consoleLogLevel both in early boot and using sysctl
         # (so you don't need to reboot to have changes take effect).
-      boot.kernelParams = [ "loglevel=${toString config.boot.consoleLogLevel}" ]
+      boot.kernelParams =
+        [ "loglevel=${toString config.boot.consoleLogLevel}" ]
         ++ optionals config.boot.vesa [
           "vga=0x317"
           "nomodeset"
-        ];
+        ]
+        ;
 
       boot.kernel.sysctl."kernel.printk" =
         mkDefault config.boot.consoleLogLevel;

test/diff/idioms_nixos_2/out.nix

@@ -834,9 +834,7 @@ in
           let
             c = cfg.config;
             writePhpArray =
-              a:
-              "[${concatMapStringsSep "," (val: ''"${toString val}"'') a}]"
-              ;
+              a: "[${concatMapStringsSep "," (val: ''"${toString val}"'') a}]";
             requiresReadSecretFunction =
               c.dbpassFile != null || c.objectstore.s3.enable;
             objectstoreConfig =
@@ -887,9 +885,7 @@ in
               ;
 
             nextcloudGreaterOrEqualThan =
-              req:
-              versionAtLeast cfg.package.version req
-              ;
+              req: versionAtLeast cfg.package.version req;
 
             overrideConfig = pkgs.writeText "nextcloud-config.php" ''
               <?php