From 48e00a963a1f0b8e32c293d95d6ee3bc56e6dfba Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 14 May 2024 20:24:08 +0200 Subject: [PATCH] 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 | 19 ++++---------- src/Nixfmt/Pretty.hs | 19 +++----------- src/Nixfmt/Types.hs | 44 +++++++++++++++----------------- test/diff/attr_set/out.nix | 2 ++ test/diff/idioms_lib_3/out.nix | 2 ++ test/diff/idioms_lib_4/out.nix | 2 ++ test/diff/idioms_lib_5/out.nix | 2 ++ test/diff/idioms_nixos_1/out.nix | 5 ++++ test/diff/idioms_nixos_2/out.nix | 4 +++ test/diff/idioms_pkgs_4/out.nix | 2 ++ test/diff/idioms_pkgs_5/out.nix | 2 ++ test/diff/key_value/out.nix | 1 + test/diff/lists/in.nix | 4 +++ test/diff/lists/out.nix | 7 +++++ test/diff/monsters_5/out.nix | 6 +++++ 15 files changed, 69 insertions(+), 52 deletions(-) diff --git a/src/Nixfmt/Parser.hs b/src/Nixfmt/Parser.hs index 63a47918..9dbcae53 100644 --- a/src/Nixfmt/Parser.hs +++ b/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 diff --git a/src/Nixfmt/Pretty.hs b/src/Nixfmt/Pretty.hs index c1b5e9ee..67ff37c8 100644 --- a/src/Nixfmt/Pretty.hs +++ b/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) ) ([], []) diff --git a/src/Nixfmt/Types.hs b/src/Nixfmt/Types.hs index 4afc6351..c2f435af 100644 --- a/src/Nixfmt/Types.hs +++ b/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] diff --git a/test/diff/attr_set/out.nix b/test/diff/attr_set/out.nix index 397512e3..7e5ec04b 100644 --- a/test/diff/attr_set/out.nix +++ b/test/diff/attr_set/out.nix @@ -87,6 +87,7 @@ c = 1; e = 1; + } rec @@ -102,6 +103,7 @@ e = 1; # f + } { x = diff --git a/test/diff/idioms_lib_3/out.nix b/test/diff/idioms_lib_3/out.nix index 990350f6..aec21686 100644 --- a/test/diff/idioms_lib_3/out.nix +++ b/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 + '' diff --git a/test/diff/idioms_lib_4/out.nix b/test/diff/idioms_lib_4/out.nix index 3cbd159a..2ed68217 100644 --- a/test/diff/idioms_lib_4/out.nix +++ b/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}"; ################################################################################ + } diff --git a/test/diff/idioms_lib_5/out.nix b/test/diff/idioms_lib_5/out.nix index 0428fc01..af81eea0 100644 --- a/test/diff/idioms_lib_5/out.nix +++ b/test/diff/idioms_lib_5/out.nix @@ -604,8 +604,10 @@ let yes = true; } .${validity.valid}; + }; in + { inherit assertValidity commonMeta; } diff --git a/test/diff/idioms_nixos_1/out.nix b/test/diff/idioms_nixos_1/out.nix index ca3f3229..8fc22d24 100644 --- a/test/diff/idioms_nixos_1/out.nix +++ b/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; + }) + ]; + } diff --git a/test/diff/idioms_nixos_2/out.nix b/test/diff/idioms_nixos_2/out.nix index dd0e43fd..6553191d 100644 --- a/test/diff/idioms_nixos_2/out.nix +++ b/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" ]; diff --git a/test/diff/idioms_pkgs_4/out.nix b/test/diff/idioms_pkgs_4/out.nix index bc384da7..7e0b9772 100644 --- a/test/diff/idioms_pkgs_4/out.nix +++ b/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 [ ]; }; }) + ] diff --git a/test/diff/idioms_pkgs_5/out.nix b/test/diff/idioms_pkgs_5/out.nix index 25825c7d..2bdedb35 100644 --- a/test/diff/idioms_pkgs_5/out.nix +++ b/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 diff --git a/test/diff/key_value/out.nix b/test/diff/key_value/out.nix index 92004523..1d93de5b 100644 --- a/test/diff/key_value/out.nix +++ b/test/diff/key_value/out.nix @@ -67,4 +67,5 @@ rec { ; p = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa { } a; + } diff --git a/test/diff/lists/in.nix b/test/diff/lists/in.nix index 0839c920..012e7300 100644 --- a/test/diff/lists/in.nix +++ b/test/diff/lists/in.nix @@ -7,6 +7,10 @@ ] ] + [ "string" + + ] + [ { # multiline foo = "bar"; diff --git a/test/diff/lists/out.nix b/test/diff/lists/out.nix index b389f22a..fed594cd 100644 --- a/test/diff/lists/out.nix +++ b/test/diff/lists/out.nix @@ -9,6 +9,11 @@ ] ] + [ + "string" + + ] + [ { # multiline @@ -85,6 +90,7 @@ b d + ] [ @@ -97,6 +103,7 @@ d # e + ] [ diff --git a/test/diff/monsters_5/out.nix b/test/diff/monsters_5/out.nix index ad6ecb84..d93c025e 100644 --- a/test/diff/monsters_5/out.nix +++ b/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 nvidia_x11. ''; + }; boot.kernelPatches @@ -262,5 +267,6 @@ in "[ pkgs.kernelPatches.ubuntu_fan_4_4 ]"; description = "A list of additional patches to apply to the kernel."; }; + }; }