Simplify and fix comments between items

Previously, newlines after the last list/attrs/let-in item would not be
preserved. So e.g.

    [
      "foo"

    ]

would wrongly turn into

    [
      "foo"
    ]

Subsequently it would turn into

    [ "foo" ]

This commit fixes it, such that the original is preserved in this case.

Whereas previously all the newlines were stripped already during
parsing, in this new commit all newlines are preserved, which both fixes
the above problem and simplifies the code.

This does introduce one odd edge case due to interaction with another
feature. That feature is that comments before a let-in's `in` part, get
moved below it. The following:

    let
      x = 10;
    # X
    in
    x

Gets reformatted into

    let
      x = 10;
    in
    # X
    x

Due to how the new code preserves newlines, those newly also get moved
further down. So whereas previously the following:

    let
      x = 10;

    in
    x

Would wrongly remove the newline, turning it into:

    let
      x = 10;
    in
    x

With this change it won't anymore, but it will move the newline down:

    let
      x = 10;
    in

    x

While this could be handled with a special case, I'm not sure if that's
worth it. It might be better to rethink this moving of comments further
down idea, I'm not sure if that's necessary anymore.

src/Nixfmt/Parser.hs

@@ -341,26 +341,17 @@ term = label "term" $ do
     _ -> Selection t sel def
 
 items :: Parser a -> Parser (Items a)
-items p = Items <$> many (item p) <> (toList <$> optional lastItem)
+items p = Items <$> many (item p) <> (toList <$> optional itemComment)
 
 item :: Parser a -> Parser (Item a)
-item p = detachedComment <|> CommentedItem <$> takeTrivia <*> p
+item p = itemComment <|> Item <$> p
 
-lastItem :: Parser (Item a)
-lastItem = do
+itemComment :: Parser (Item a)
+itemComment = do
   trivia <- takeTrivia
   case trivia of
     [] -> empty
-    _ -> pure $ DetachedComments trivia
-
-detachedComment :: Parser (Item a)
-detachedComment = do
-  trivia <- takeTrivia
-  case break (== EmptyLine) trivia of
-    -- Return a set of comments that don't annotate the next item
-    (detached, EmptyLine : trivia') -> pushTrivia trivia' >> pure (DetachedComments detached)
-    -- The remaining trivia annotate the next item
-    _ -> pushTrivia trivia >> empty
+    _ -> pure $ Comments trivia
 
 -- ABSTRACTIONS
 

src/Nixfmt/Pretty.hs

@@ -91,23 +91,12 @@ instance Pretty Trivium where
         | otherwise = comment l <> hardline
 
 instance (Pretty a) => Pretty (Item a) where
-  pretty (DetachedComments trivia) = pretty trivia
-  pretty (CommentedItem trivia x) = pretty trivia <> group x
+  pretty (Comments trivia) = pretty trivia
+  pretty (Item x) = group x
 
 -- For lists, attribute sets and let bindings
 prettyItems :: (Pretty a) => Items a -> Doc
--- Special case: Preserve an empty line with no items
--- usually, trailing newlines after the last element are not preserved
-prettyItems (Items [DetachedComments []]) = emptyline
-prettyItems items = prettyItems' $ unItems items
-  where
-    prettyItems' :: (Pretty a) => [Item a] -> Doc
-    prettyItems' [] = mempty
-    prettyItems' [item] = pretty item
-    prettyItems' (item : xs) =
-      pretty item
-        <> case item of CommentedItem _ _ -> hardline; DetachedComments _ -> emptyline
-        <> prettyItems' xs
+prettyItems (Items items) = sepBy hardline items
 
 instance Pretty [Trivium] where
   pretty [] = mempty
@@ -561,7 +550,7 @@ instance Pretty Expression where
       (binderComments, bindersWithoutComments) =
         foldr
           ( \item (start, rest) -> case item of
-              (DetachedComments inner) | null rest -> (inner : start, rest)
+              (Comments inner) | null rest -> (inner : start, rest)
               _ -> (start, item : rest)
           )
           ([], [])

src/Nixfmt/Types.hs

@@ -58,12 +58,10 @@ instance (Eq a) => Eq (Ann a) where
 --    show (Ann _ a _) = show a
 
 data Item a
-  = -- | An item with a list of line comments that apply to it. There is no
-    -- empty line between the comments and the stuff it applies to.
-    CommentedItem Trivia a
-  | -- | A list of line comments not associated with any item. Followed by an
-    -- empty line unless they're the last comments in a set or list.
-    DetachedComments Trivia
+  = -- | An item
+    Item a
+  | -- | Trivia interleaved in items
+    Comments Trivia
   deriving (Foldable, Show)
 
 newtype Items a = Items {unItems :: [Item a]}
@@ -279,33 +277,33 @@ instance LanguageElement Term where
   walkSubprograms = \case
     -- Map each item to a singleton list, then handle that
     (List _ items _) | Prelude.length (unItems items) == 1 -> case Prelude.head (unItems items) of
-      (CommentedItem c item) -> [emptySet c, Term item]
-      (DetachedComments _) -> []
+      (Item item) -> [Term item]
+      (Comments _) -> []
     (List _ items _) ->
       unItems items >>= \case
-        CommentedItem comment item ->
-          [Term (List (ann TBrackOpen) (Items [CommentedItem comment item]) (ann TBrackClose))]
-        DetachedComments c ->
-          [Term (List (ann TBrackOpen) (Items [DetachedComments c]) (ann TBrackClose))]
+        Item item ->
+          [Term (List (ann TBrackOpen) (Items [Item item]) (ann TBrackClose))]
+        Comments c ->
+          [Term (List (ann TBrackOpen) (Items [Comments c]) (ann TBrackClose))]
     (Set _ _ items _) | Prelude.length (unItems items) == 1 -> case Prelude.head (unItems items) of
-      (CommentedItem c (Inherit _ from sels _)) ->
-        (Term <$> maybeToList from) ++ concatMap walkSubprograms sels ++ [emptySet c]
-      (CommentedItem c (Assignment sels _ expr _)) ->
-        expr : concatMap walkSubprograms sels ++ [emptySet c]
-      (DetachedComments _) -> []
+      (Item (Inherit _ from sels _)) ->
+        (Term <$> maybeToList from) ++ concatMap walkSubprograms sels
+      (Item (Assignment sels _ expr _)) ->
+        expr : concatMap walkSubprograms sels
+      (Comments _) -> []
     (Set _ _ items _) ->
       unItems items >>= \case
         -- Map each binding to a singleton set
-        (CommentedItem comment item) ->
-          [Term (Set Nothing (ann TBraceOpen) (Items [CommentedItem comment item]) (ann TBraceClose))]
-        (DetachedComments c) -> [emptySet c]
+        (Item item) ->
+          [Term (Set Nothing (ann TBraceOpen) (Items [Item item]) (ann TBraceClose))]
+        (Comments c) -> [emptySet c]
     (Selection term sels Nothing) -> Term term : (sels >>= walkSubprograms)
     (Selection term sels (Just (_, def))) -> Term term : (sels >>= walkSubprograms) ++ [Term def]
     (Parenthesized _ expr _) -> [expr]
     -- The others are already minimal
     _ -> []
     where
-      emptySet c = Term (Set Nothing (ann TBraceOpen) (Items [DetachedComments c]) (ann TBraceClose))
+      emptySet c = Term (Set Nothing (ann TBraceOpen) (Items [Comments c]) (ann TBraceClose))
 
 instance LanguageElement Expression where
   mapFirstToken' f = \case
@@ -342,8 +340,8 @@ instance LanguageElement Expression where
       body
         : ( unItems items >>= \case
               -- Map each binding to a singleton set
-              (CommentedItem _ item) -> [Term (Set Nothing (ann TBraceOpen) (Items [CommentedItem [] item]) (ann TBraceClose))]
-              (DetachedComments _) -> []
+              (Item item) -> [Term (Set Nothing (ann TBraceOpen) (Items [Item item]) (ann TBraceClose))]
+              (Comments _) -> []
           )
     (Assert _ cond _ body) -> [cond, body]
     (If _ expr0 _ expr1 _ expr2) -> [expr0, expr1, expr2]

test/diff/attr_set/out.nix

@@ -87,6 +87,7 @@
     c = 1;
 
     e = 1;
+
   }
 
   rec
@@ -102,6 +103,7 @@
     e = 1;
 
     # f
+
   }
   {
     x =

test/diff/idioms_lib_3/out.nix

@@ -20,6 +20,7 @@ let
 
   inherit (lib) isFunction;
 in
+
 rec {
 
   ## -- HELPER FUNCTIONS & DEFAULTS --
@@ -529,6 +530,7 @@ rec {
           )
         );
     in
+
     ''
       <?xml version="1.0" encoding="UTF-8"?>
       <!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">

test/diff/idioms_lib_4/out.nix

@@ -888,6 +888,7 @@ rec {
             abis.unknown;
       };
     in
+
     mkSystem parsed;
 
   mkSystemFromString =
@@ -927,4 +928,5 @@ rec {
     "${cpu.name}-${vendor.name}-${kernelName kernel}${optExecFormat}${optAbi}";
 
   ################################################################################
+
 }

test/diff/idioms_lib_5/out.nix

@@ -604,8 +604,10 @@ let
           yes = true;
         }
         .${validity.valid};
+
     };
 in
+
 {
   inherit assertValidity commonMeta;
 }

test/diff/idioms_nixos_1/out.nix

@@ -214,6 +214,7 @@ in
         lib.kernelConfig functions to build list elements.
       '';
     };
+
   };
 
   ###### implementation
@@ -262,6 +263,7 @@ in
               "hid_logitech_hidpp"
               "hid_logitech_dj"
               "hid_microsoft"
+
             ]
             ++ optionals pkgs.stdenv.hostPlatform.isx86 [
               # Misc. x86 keyboard stuff.
@@ -381,6 +383,9 @@ in
             assertion = attrs.assertion cfg;
             inherit (attrs) message;
           }) config.system.requiredKernelConfig;
+
     })
+
   ];
+
 }

test/diff/idioms_nixos_2/out.nix

@@ -68,6 +68,7 @@ let
 
   inherit (config.system) stateVersion;
 in
+
 {
 
   imports = [
@@ -383,6 +384,7 @@ in
           is used to also support MariaDB version >= 10.6.
         '';
       };
+
     };
 
     config = {
@@ -763,6 +765,7 @@ in
             `services.nextcloud.package`.
           '';
         in
+
         (optional (cfg.poolConfig != null) ''
           Using config.services.nextcloud.poolConfig is deprecated and will become unsupported in a future release.
           Please migrate your configuration to config.services.nextcloud.poolSettings.
@@ -1017,6 +1020,7 @@ in
               '') ([ cfg.hostName ] ++ cfg.config.extraTrustedDomains)
             );
           in
+
           {
             wantedBy = [ "multi-user.target" ];
             before = [ "phpfpm-nextcloud.service" ];

test/diff/idioms_pkgs_4/out.nix

@@ -189,6 +189,7 @@ in
         # Curl should be in /usr/bin or so.
         curl = null;
       };
+
     }
   )
 
@@ -212,4 +213,5 @@ in
         if localSystem.isLinux then [ prevStage.patchelf ] else [ ];
     };
   })
+
 ]

test/diff/idioms_pkgs_5/out.nix

@@ -91,6 +91,7 @@ let
 
       finalPackage = mkDerivationSimple overrideAttrs args;
     in
+
     finalPackage;
 
   #makeDerivationExtensibleConst = attrs: makeDerivationExtensible (_: attrs);
@@ -786,6 +787,7 @@ let
           passthru
       ) (derivation (derivationArg // optionalAttrs envIsExportable checkedEnv));
 in
+
 fnOrAttrs:
 if builtins.isFunction fnOrAttrs then
   makeDerivationExtensible fnOrAttrs

test/diff/key_value/out.nix

@@ -67,4 +67,5 @@ rec {
   ;
 
   p = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa { } a;
+
 }

test/diff/lists/out.nix

@@ -85,6 +85,7 @@
     b
 
     d
+
   ]
   [
 
@@ -97,6 +98,7 @@
     d
 
     # e
+
   ]
 
   [

test/diff/monsters_5/out.nix

@@ -101,6 +101,7 @@ in
                       which would have separate nixos options.
                       `grep features pkgs/os-specific/linux/kernel/common-config.nix`
                     '';
+
               };
 
         boot.kernelPackages
@@ -132,6 +133,7 @@ in
                           =
 
                             mergeEqualOption;
+
                       };
 
                 apply
@@ -189,8 +191,10 @@ in
                                           super.kernel.features
 
                                           features;
+
                                   }
                                 );
+
                         }
                       );
 
@@ -229,6 +233,7 @@ in
                       then it also needs to contain an attribute
                       <varname>nvidia_x11</varname>.
                     '';
+
               };
 
         boot.kernelPatches
@@ -262,5 +267,6 @@ in
                       "[ pkgs.kernelPatches.ubuntu_fan_4_4 ]";
                 description = "A list of additional patches to apply to the kernel.";
               };
+
       };
 }