Merge pull request #307: Tweak operation absorbtion (ex. Unindent Concatenation)

Author: piegamesde

src/Nixfmt/Parser.hs

@@ -487,6 +487,16 @@ opCombiner (Op Postfix TQuestion) =
       <$> operator TQuestion
       <*> selectorPath
 opCombiner (Op Postfix _) = undefined
+-- HACK: We parse TPlus as left-associative, but convert it to right-associative in the AST
+-- This is allowed because addition is fully associative
+-- This is necessary because some downstream formatting code needs to match on the first operand,
+-- and doing that with a left-associative chain is not possible.
+-- A proper solution would be to flatten all associative operators into a list, but that would be a lot harder to do
+opCombiner (Op InfixL TPlus) = MPExpr.InfixL $ flip mkOp <$> operator TPlus
+  where
+    mkOp :: Expression -> Leaf -> Expression -> Expression
+    mkOp (Operation one op1 two) op2 three | op1 == op2 = Operation one op1 (Operation two op2 three)
+    mkOp left op right = Operation left op right
 opCombiner (Op InfixL tok) = MPExpr.InfixL $ flip Operation <$> operator tok
 opCombiner (Op InfixN tok) = MPExpr.InfixN $ flip Operation <$> operator tok
 opCombiner (Op InfixR tok) = MPExpr.InfixR $ flip Operation <$> operator tok

src/Nixfmt/Pretty.hs

@@ -56,10 +56,12 @@ import Nixfmt.Types (
   Trivium (..),
   Whole (..),
   ann,
+  hasPreTrivia,
   hasTrivia,
   mapFirstToken,
   mapFirstToken',
   mapLastToken',
+  matchFirstToken,
   tokenText,
  )
 import Nixfmt.Util (isSpaces)
@@ -151,7 +153,7 @@ instance Pretty Binder where
   pretty (Assignment selectors assign expr semicolon) =
     group $
       hcat selectors
-        <> nest (hardspace <> pretty assign <> nest rhs)
+        <> nest (hardspace <> pretty assign <> rhs)
         <> pretty semicolon
     where
       rhs =
@@ -262,7 +264,7 @@ instance Pretty ParamAttr where
     group $
       pretty name
         <> hardspace
-        <> nest (pretty qmark <> nest (absorbRHS def))
+        <> nest (pretty qmark <> absorbRHS def)
         <> pretty maybeComma
   -- `...`
   pretty (ParamEllipsis ellipsis) =
@@ -500,6 +502,34 @@ prettyApp indentFunction pre hasPost f a =
             (\fRendered -> group' RegularG $ fRendered <> line <> absorbLast a <> post)
       <> (if hasPost && not (null comment') then hardline else mempty)
 
+prettyOp :: Bool -> Expression -> Leaf -> Doc
+prettyOp forceFirstTermWide operation op =
+  let -- Walk the operation tree and put a list of things on the same level.
+      -- We still need to keep the operators around because they might have comments attached to them.
+      -- An operator is put together with its succeeding expression. Only the first operand has none.
+      flatten :: Maybe Leaf -> Expression -> [(Maybe Leaf, Expression)]
+      flatten opL (Operation a opR b) | opR == op = flatten opL a ++ flatten (Just opR) b
+      flatten opL x = [(opL, x)]
+
+      -- Called on every operand except the first one (a.k.a. RHS)
+      absorbOperation :: Expression -> Doc
+      absorbOperation (Term t) | isAbsorbable t = hardspace <> pretty t
+      -- Force nested operations to start on a new line
+      absorbOperation x@(Operation{}) = group' RegularG $ line <> pretty x
+      -- Force applications to start on a new line if more than the last argument is multiline
+      absorbOperation (Application f a) = group $ prettyApp False line False f a
+      absorbOperation x = hardspace <> pretty x
+
+      prettyOperation :: (Maybe Leaf, Expression) -> Doc
+      -- First element
+      prettyOperation (Nothing, Term t) | isAbsorbableTerm t && forceFirstTermWide = prettyTermWide t
+      prettyOperation (Nothing, expr) = pretty expr
+      -- The others
+      prettyOperation (Just op', expr) =
+        line <> pretty (moveTrailingCommentUp op') <> nest (absorbOperation expr)
+  in group' RegularG $
+      (concatMap prettyOperation . flatten Nothing) operation
+
 prettyWith :: Bool -> Expression -> Doc
 -- absorb the body
 prettyWith True (With with expr0 semicolon (Term expr1)) =
@@ -588,50 +618,48 @@ absorbRHS expr = case expr of
   -- 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)
+        nest $ hardspace <> group (absorbExpr False expr)
   -- Absorbable expression. Always start on the same line, and force-expand attrsets
-  _ | isAbsorbableExpr expr -> hardspace <> group (absorbExpr True expr)
+  _ | isAbsorbableExpr expr -> nest $ 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
+  (Term (Parenthesized open expr' close)) -> nest $ hardspace <> absorbParen open expr' close
   -- Not all strings are absorbable, but in this case we always want to keep them attached.
   -- Because there's nothing to gain from having them start on a new line.
-  (Term (SimpleString _)) -> hardspace <> group expr
-  (Term (IndentedString _)) -> hardspace <> group expr
+  (Term (SimpleString _)) -> nest $ hardspace <> group expr
+  (Term (IndentedString _)) -> nest $ hardspace <> group expr
   -- Same for path
-  (Term (Path _)) -> hardspace <> group expr
+  (Term (Path _)) -> nest $ hardspace <> group expr
   -- Non-absorbable term
   -- If it is multi-line, force it to start on a new line with indentation
-  (Term _) -> group' RegularG (line <> pretty expr)
+  (Term _) -> nest $ group' RegularG (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 False line False f a
-  (With{}) -> group' RegularG $ line <> pretty expr
-  -- Special case `//` and `++` 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) (LoneAnn op) b)
-    | isAbsorbable t
-        && isUpdateOrConcat op
-        -- Exclude further operations on the RHS
-        -- Hotfix for https://github.com/NixOS/nixfmt/issues/198
-        && case b of (Operation{}) -> False; _ -> True ->
-        group' RegularG $ line <> group' Priority (prettyTermWide t) <> line <> pretty op <> hardspace <> pretty b
-  -- Case 2a: LHS fits onto first line, RHS is an absorbable term
+  (Application f a) -> nest $ prettyApp False line False f a
+  (With{}) -> nest $ group' RegularG $ line <> pretty expr
+  -- Special case `//` and `++` and `+` operations to be more compact in some cases
+  -- The following code assumes all of these operators are parsed with right-handed associativity
+  -- (even though in Nix, addition technically is considered left-associative)
+  -- Case 1: LHS is absorbable term, unindent concatenations
+  -- https://github.com/NixOS/nixfmt/issues/228
+  (Operation (Term t) op@(Ann{value}) _)
+    | isAbsorbableTerm t
+        && matchFirstToken (not . hasPreTrivia) t
+        && isUpdateConcatPlus value ->
+        hardspace <> prettyOp True expr op
+  -- Case 2: LHS fits onto first line, RHS is an absorbable term
   (Operation l (LoneAnn op) (Term t))
-    | isAbsorbable t && isUpdateOrConcat op ->
-        group' RegularG $ line <> pretty l <> line <> group' Transparent (pretty op <> hardspace <> group' Priority (prettyTermWide t))
-  -- Case 2b: LHS fits onto first line, RHS is a function application
-  (Operation l (LoneAnn op) (Application f a))
-    | isUpdateOrConcat op ->
-        line <> group l <> line <> prettyApp False (pretty op <> hardspace) False f a
+    | isAbsorbable t && isUpdateConcatPlus op ->
+        nest $ group' RegularG $ line <> pretty l <> line <> group' Transparent (pretty op <> hardspace <> group' Priority (prettyTermWide t))
   -- 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 expr
+  _ -> nest $ line <> group expr
   where
-    isUpdateOrConcat TUpdate = True
-    isUpdateOrConcat TConcat = True
-    isUpdateOrConcat _ = False
+    isUpdateConcatPlus TUpdate = True
+    isUpdateConcatPlus TConcat = True
+    isUpdateConcatPlus TPlus = True
+    isUpdateConcatPlus _ = False
 
 instance Pretty Expression where
   pretty (Term t) = pretty t
@@ -712,31 +740,7 @@ instance Pretty Expression where
     | op' == TLess || op' == TGreater || op' == TLessEqual || op' == TGreaterEqual || op' == TEqual || op' == TUnequal =
         pretty a <> softline <> pretty op <> hardspace <> pretty b
   -- all other operators
-  pretty operation@(Operation _ op _) =
-    let -- Walk the operation tree and put a list of things on the same level.
-        -- We still need to keep the operators around because they might have comments attached to them.
-        -- An operator is put together with its succeeding expression. Only the first operand has none.
-        flatten :: Maybe Leaf -> Expression -> [(Maybe Leaf, Expression)]
-        flatten opL (Operation a opR b) | opR == op = flatten opL a ++ flatten (Just opR) b
-        flatten opL x = [(opL, x)]
-
-        -- Called on every operand except the first one (a.k.a. RHS)
-        absorbOperation :: Expression -> Doc
-        absorbOperation (Term t) | isAbsorbable t = hardspace <> pretty t
-        -- Force nested operations to start on a new line
-        absorbOperation x@(Operation{}) = group' RegularG $ line <> pretty x
-        -- Force applications to start on a new line if more than the last argument is multiline
-        absorbOperation (Application f a) = group $ prettyApp False line False f a
-        absorbOperation x = hardspace <> pretty x
-
-        prettyOperation :: (Maybe Leaf, Expression) -> Doc
-        -- First element
-        prettyOperation (Nothing, expr) = pretty expr
-        -- The others
-        prettyOperation (Just op', expr) =
-          line <> pretty (moveTrailingCommentUp op') <> nest (absorbOperation expr)
-    in group' RegularG $
-        (concatMap prettyOperation . flatten Nothing) operation
+  pretty operation@(Operation _ op _) = prettyOp False operation op
   pretty (MemberCheck expr qmark sel) =
     pretty expr
       <> softline

src/Nixfmt/Types.hs

@@ -34,9 +34,11 @@ module Nixfmt.Types (
   Trivium (..),
   removeLineInfo,
   hasTrivia,
+  hasPreTrivia,
   LanguageElement,
   mapFirstToken,
   mapFirstToken',
+  matchFirstToken,
   mapLastToken',
   mapAllTokens,
   operators,
@@ -96,6 +98,9 @@ hasTrivia :: Ann a -> Bool
 hasTrivia (LoneAnn _) = False
 hasTrivia _ = True
 
+hasPreTrivia :: Ann a -> Bool
+hasPreTrivia Ann{preTrivia} = preTrivia /= []
+
 -- | Create a new annotated value without any annotations
 ann :: Pos -> a -> Ann a
 ann l v =
@@ -242,6 +247,10 @@ class LanguageElement a where
   -- returned. This is useful for getting/extracting values
   mapFirstToken' :: (forall b. Ann b -> (Ann b, c)) -> a -> (a, c)
 
+  -- Convenience method. Special case of `mapFirstToken'` that takes a predicate and doesn't modify the token
+  matchFirstToken :: (forall b. Ann b -> Bool) -> a -> Bool
+  matchFirstToken f = snd . mapFirstToken' (\b -> (b, f b))
+
   -- Same as mapLastToken, but the mapping function also yields a value that may be
   -- returned. This is useful for getting/extracting values
   mapLastToken' :: (forall b. Ann b -> (Ann b, c)) -> a -> (a, c)

standard.md

@@ -1282,6 +1282,27 @@ Notable special cases are:
       dependencies
     ];
   ```
+- Operator chains (specifically, `++`, `//` and `+`) where the first element is absorbable are treated specially to avoid diff churn. Assuming the following input:
+  ```nix
+  foo = [
+    bar
+  ];
+  ```
+  Normally (see "otherwise newline and indent" above), concatenating items onto that list would be formatted like this, causing the entire list to re-indent:
+  ```nix
+  foo =
+    [
+      bar
+    ]
+    ++ baz;
+  ```
+  Instead, we format it as such:
+  ```nix
+  foo = [
+    bar
+  ]
+  ++ baz;
+  ```
 
 **Alternatives**
 

test/diff/attr_set/in.nix

@@ -291,4 +291,101 @@
     ] [
     ];
   }
+  # https://github.com/NixOS/nixfmt/issues/228
+  {
+    foo = aaaaaaaaaaaaaaaaaaaaaaaaa
+    + bbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+    + ccccccccccccccccccccccccccccc
+    + ddddddddddddddddddddddddddddd;
+
+    boot.kernelParams = [ aaaaaaaaaaaaaa ]
+      ++ optionals config.boot.vesa [
+        "vga=0x317"
+        "nomodeset"
+      ];
+
+    foo2 = [ bar ]
+      ++ baz # newline!
+      ++ meow;
+
+    foo3 =
+      some function application that kinda is long /* and */ multiline
+      ++ [
+        a
+        list
+      ];
+    some.long.attribute # with a comment
+      = [ stuff ]
+      ++ more stuff;
+    some.long.attribute1 # with a comment
+      = [ stuff a b c ]
+      ++ more stuff;
+
+    foo4 # nasty
+      = # comments
+      [ bar ]
+      ++ baz # newline!
+      ++ meow;
+
+    environment.systemPackages =
+      # Include the PAM modules in the system path mostly for the manpages.
+      [ package ]
+      ++ lib.optional config.users.ldap.enable pam_ldap;
+
+    environment.systemPackages2 =
+      # Include the PAM modules in the system path mostly for the manpages.
+      [ package ]
+      ++ lib.optional config.users.ldap.enable pam_ldap
+      ++ lib.optional config.services.kanidm.enablePam config.services.kanidm.package
+      ++ lib.optional config.services.sssd.enable pkgs.sssd
+      ++ lib.optionals config.security.pam.krb5.enable [
+        pam_krb5
+        pam_ccreds
+      ];
+
+    buildInputs1 = lib.optionals onePlatform [
+      a
+      b
+      c
+    ];
+
+    buildInputs2 = lib.optionals onePlatform [
+      a
+      b
+      c
+    ]
+    ++ lib.optionals anotherPlatform [
+      d
+      e
+      f
+    ];
+
+    programs.ssh.knownHosts =
+    someStuff functionArg
+    // lib.mapAttrs (host_name: publicKey: {
+      inherit publicKey;
+      extraHostNames = [
+        "${host_name}.m-0.eu"
+        "${host_name}.vpn.m-0.eu"
+        "${host_name}.lo.m-0.eu"
+      ];
+    }) secret-config.ssh-hosts
+    // {
+      foo = "bar";
+    };
+
+    programs.ssh.knownHosts2 = [] ++ (with expr;
+    someStuff functionArg)
+    // lib.mapAttrs (host_name: publicKey: {
+      inherit publicKey;
+      extraHostNames = [
+        "${host_name}.m-0.eu"
+        "${host_name}.vpn.m-0.eu"
+        "${host_name}.lo.m-0.eu"
+      ];
+    }) secret-config.ssh-hosts
+    // {
+      foo = "bar";
+    };
+  }
 ]