Merge pull request NixOS#233 from piegamesde/compact-lists

Don't expand lists within functions
piegamesde · Sep 7, 2024 · d0c0cda · d0c0cda
2 parents 3e8eef7 + d8caa4c
commit d0c0cda
Show file tree

Hide file tree

Showing 13 changed files with 103 additions and 212 deletions.
diff --git a/src/Nixfmt/Pretty.hs b/src/Nixfmt/Pretty.hs
@@ -375,15 +375,34 @@ instance Pretty Parameter where
 --    then start on a new line instead".
 prettyApp :: Bool -> Doc -> Bool -> Expression -> Expression -> Doc
 prettyApp indentFunction pre hasPost f a =
-  let -- This is very hacky, but selections shouldn't be in a priority group,
+  let -- Walk the function call chain
+      absorbApp :: Expression -> Doc
+      -- This is very hacky, but selections shouldn't be in a priority group,
       -- because if they get expanded before anything else,
       -- only the `.`-and-after part gets to a new line, which looks very odd
-      absorbApp (Application f' a'@(Term Selection{})) = group' Transparent (absorbApp f') <> line <> nest (group' RegularG a')
-      absorbApp (Application f' a') = group' Transparent (absorbApp f') <> line <> nest (group' Priority a')
+      absorbApp (Application f' a'@(Term Selection{})) = group' Transparent (absorbApp f') <> line <> nest (group' RegularG $ absorbInner a')
+      absorbApp (Application f' a') = group' Transparent (absorbApp f') <> line <> nest (group' Priority $ absorbInner a')
+      -- First argument
       absorbApp expr
         | indentFunction && null comment' = nest $ group' RegularG $ line' <> pretty expr
         | otherwise = pretty expr
 
+      -- Render the inner arguments of a function call
+      absorbInner :: Expression -> Doc
+      -- If lists have only simple items, try to render them single-line instead of expanding
+      -- This is just a copy of the list rendering code, but with `sepBy line` instead of `sepBy hardline`
+      absorbInner (Term (List paropen@Ann{trailComment = post'} items parclose))
+        | length (unItems items) <= 4 && all (isSimple . Term) items =
+            pretty (paropen{trailComment = Nothing})
+              <> surroundWith sur (nest $ pretty post' <> sepBy line (unItems items))
+              <> pretty parclose
+        where
+          -- If the brackets are on different lines, keep them like that
+          sur = if sourceLine paropen /= sourceLine parclose then hardline else line
+      absorbInner expr = pretty expr
+
+      -- Render the last argument of a function call
+      absorbLast :: Expression -> Doc
       absorbLast (Term t)
         | isAbsorbable t =
             group' Priority $ nest $ prettyTerm t
@@ -522,7 +541,11 @@ absorbExpr _ expr = pretty expr
 -- Render the RHS value of an assignment or function parameter default value
 absorbRHS :: Expression -> Doc
 absorbRHS expr = case expr of
-  -- Absorbable expression. Always start on the same line
+  -- Exception to the case below: Don't force-expand attrsets if they only contain a single inherit statement
+  (Term (Set _ _ binders _))
+    | case unItems binders of [Item (Inherit{})] -> True; _ -> False ->
+        hardspace <> group (absorbExpr False expr)
+  -- Absorbable expression. Always start on the same line, and force-expand attrsets
   _ | isAbsorbableExpr expr -> hardspace <> group (absorbExpr True expr)
   -- Parenthesized expression. Same thing as the special case for parenthesized last argument in function calls.
   (Term (Parenthesized open expr' close)) -> hardspace <> absorbParen open expr' close

diff --git a/src/Nixfmt/Types.hs b/src/Nixfmt/Types.hs
@@ -122,7 +122,7 @@ data Item a
     Comments Trivia
   deriving (Foldable, Show, Functor)
 
-newtype Items a = Items {unItems :: [Item a]} deriving (Functor)
+newtype Items a = Items {unItems :: [Item a]} deriving (Functor, Foldable)
 
 instance (Eq a) => Eq (Items a) where
   (==) = (==) `on` concatMap Data.Foldable.toList . unItems

diff --git a/test/diff/apply/in.nix b/test/diff/apply/in.nix
@@ -65,7 +65,7 @@
       mapAttrsToStringsSep "\n" mkSection attrsOfAttrs;
   }
   [
-    (mapAttrsToStringsSep [force long] "\n" mkSection attrsOfAttrs)
+    (mapAttrsToStringsSep [force /* meow */ long] "\n" mkSection attrsOfAttrs)
   ]
   (a
   b)

diff --git a/test/diff/apply/out-pure.nix b/test/diff/apply/out-pure.nix
@@ -71,7 +71,7 @@
   }
   [
     (mapAttrsToStringsSep [
-      force
+      force # meow
       long
     ] "\n" mkSection attrsOfAttrs)
   ]
@@ -160,16 +160,10 @@
       ''"''
       "\${"
     ];
-    escapeMultiline =
-      libStr.replaceStrings
-        [
-          "\${"
-          "''"
-        ]
-        [
-          "''\${"
-          "'''"
-        ];
+    escapeMultiline = libStr.replaceStrings [ "\${" "''" ] [
+      "''\${"
+      "'''"
+    ];
     test =
       foo
         [

diff --git a/test/diff/apply/out.nix b/test/diff/apply/out.nix
@@ -71,7 +71,7 @@
   }
   [
     (mapAttrsToStringsSep [
-      force
+      force # meow
       long
     ] "\n" mkSection attrsOfAttrs)
   ]

diff --git a/test/diff/idioms_lib_3/out-pure.nix b/test/diff/idioms_lib_3/out-pure.nix
@@ -130,13 +130,7 @@ rec {
   toINI =
     {
       # apply transformations (e.g. escapes) to section names
-      mkSectionName ? (
-        name:
-        libStr.escape [
-          "["
-          "]"
-        ] name
-      ),
+      mkSectionName ? (name: libStr.escape [ "[" "]" ] name),
       # format a setting line from key and value
       mkKeyValue ? mkKeyValueDefault { } "=",
       # allow lists as values for duplicate keys
@@ -191,13 +185,7 @@ rec {
   toINIWithGlobalSection =
     {
       # apply transformations (e.g. escapes) to section names
-      mkSectionName ? (
-        name:
-        libStr.escape [
-          "["
-          "]"
-        ] name
-      ),
+      mkSectionName ? (name: libStr.escape [ "[" "]" ] name),
       # format a setting line from key and value
       mkKeyValue ? mkKeyValueDefault { } "=",
       # allow lists as values for duplicate keys
@@ -378,16 +366,10 @@ rec {
               ''"''
               "\${"
             ];
-            escapeMultiline =
-              libStr.replaceStrings
-                [
-                  "\${"
-                  "''"
-                ]
-                [
-                  "''\${"
-                  "'''"
-                ];
+            escapeMultiline = libStr.replaceStrings [ "\${" "''" ] [
+              "''\${"
+              "'''"
+            ];
             singlelineResult =
               ''"'' + concatStringsSep "\\n" (map escapeSingleline lines) + ''"'';
             multilineResult =

diff --git a/test/diff/idioms_lib_3/out.nix b/test/diff/idioms_lib_3/out.nix
@@ -133,13 +133,7 @@ rec {
   toINI =
     {
       # apply transformations (e.g. escapes) to section names
-      mkSectionName ? (
-        name:
-        libStr.escape [
-          "["
-          "]"
-        ] name
-      ),
+      mkSectionName ? (name: libStr.escape [ "[" "]" ] name),
       # format a setting line from key and value
       mkKeyValue ? mkKeyValueDefault { } "=",
       # allow lists as values for duplicate keys
@@ -194,13 +188,7 @@ rec {
   toINIWithGlobalSection =
     {
       # apply transformations (e.g. escapes) to section names
-      mkSectionName ? (
-        name:
-        libStr.escape [
-          "["
-          "]"
-        ] name
-      ),
+      mkSectionName ? (name: libStr.escape [ "[" "]" ] name),
       # format a setting line from key and value
       mkKeyValue ? mkKeyValueDefault { } "=",
       # allow lists as values for duplicate keys
@@ -391,16 +379,10 @@ rec {
               ''"''
               "\${"
             ];
-            escapeMultiline =
-              libStr.replaceStrings
-                [
-                  "\${"
-                  "''"
-                ]
-                [
-                  "''\${"
-                  "'''"
-                ];
+            escapeMultiline = libStr.replaceStrings [ "\${" "''" ] [
+              "''\${"
+              "'''"
+            ];
             singlelineResult =
               ''"'' + concatStringsSep "\\n" (map escapeSingleline lines) + ''"'';
             multilineResult =

diff --git a/test/diff/idioms_lib_4/out-pure.nix b/test/diff/idioms_lib_4/out-pure.nix
@@ -521,33 +521,25 @@ rec {
       # the normalized name for macOS.
       macos = {
         execFormat = macho;
-        families = {
-          inherit darwin;
-        };
+        families = { inherit darwin; };
         name = "darwin";
       };
       ios = {
         execFormat = macho;
-        families = {
-          inherit darwin;
-        };
+        families = { inherit darwin; };
       };
       # A tricky thing about FreeBSD is that there is no stable ABI across
       # versions. That means that putting in the version as part of the
       # config string is paramount.
       freebsd12 = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
         name = "freebsd";
         version = 12;
       };
       freebsd13 = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
         name = "freebsd";
         version = 13;
       };
@@ -557,19 +549,15 @@ rec {
       };
       netbsd = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
       };
       none = {
         execFormat = unknown;
         families = { };
       };
       openbsd = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
       };
       solaris = {
         execFormat = elf;

diff --git a/test/diff/idioms_lib_4/out.nix b/test/diff/idioms_lib_4/out.nix
@@ -521,33 +521,25 @@ rec {
       # the normalized name for macOS.
       macos = {
         execFormat = macho;
-        families = {
-          inherit darwin;
-        };
+        families = { inherit darwin; };
         name = "darwin";
       };
       ios = {
         execFormat = macho;
-        families = {
-          inherit darwin;
-        };
+        families = { inherit darwin; };
       };
       # A tricky thing about FreeBSD is that there is no stable ABI across
       # versions. That means that putting in the version as part of the
       # config string is paramount.
       freebsd12 = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
         name = "freebsd";
         version = 12;
       };
       freebsd13 = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
         name = "freebsd";
         version = 13;
       };
@@ -557,19 +549,15 @@ rec {
       };
       netbsd = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
       };
       none = {
         execFormat = unknown;
         families = { };
       };
       openbsd = {
         execFormat = elf;
-        families = {
-          inherit bsd;
-        };
+        families = { inherit bsd; };
       };
       solaris = {
         execFormat = elf;

diff --git a/test/diff/idioms_nixos_1/out-pure.nix b/test/diff/idioms_nixos_1/out-pure.nix
@@ -284,9 +284,7 @@ in
     })
 
     (mkIf (!config.boot.isContainer) {
-      system.build = {
-        inherit kernel;
-      };
+      system.build = { inherit kernel; };
 
       system.modulesTree = [ kernel ] ++ config.boot.extraModulePackages;
 

diff --git a/test/diff/idioms_nixos_1/out.nix b/test/diff/idioms_nixos_1/out.nix
@@ -284,9 +284,7 @@ in
     })
 
     (mkIf (!config.boot.isContainer) {
-      system.build = {
-        inherit kernel;
-      };
+      system.build = { inherit kernel; };
 
       system.modulesTree = [ kernel ] ++ config.boot.extraModulePackages;