Skip to content

Commit 5b32b0a

Browse files
dyegoaureliodyegoaurelio
committed
Fix empty lists/sets with comments idempotency
Empty lists and attribute sets containing only comments were not formatting idempotently. The issue occurred because block comments (/* ... */) initially parsed as trailing comments on the opening bracket would convert to line comments (# ...) and move inside the Items during formatting. This AST structure change caused different rendering behavior between successive runs. Fixed by ensuring consistent handling of both states: - Modified `renderList` to use `hardline` separator when items contain only comments or when the opening bracket has trivia or the opening bracket has - Updated `isAbsorbable` to treat lists/sets with trivia or only comments as absorbable in assignments This ensures the same formatting output regardless of whether comments still are trailing comments for the opening brackets or have already been converted to list comment items.
1 parent 42e43d9 commit 5b32b0a

File tree

7 files changed

+225
-3
lines changed

7 files changed

+225
-3
lines changed

‎src/Nixfmt/Pretty.hs‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,17 +124,24 @@ renderItems separator (Items items) = go items
124124
go (item : rest) =
125125
pretty item <> if null rest then mempty else separator <> go rest
126126

127+
hasOnlyComments :: Items a -> Bool
128+
hasOnlyComments (Items xs) = not (null xs) && all isComment xs
129+
where
130+
isComment (Comments _) = True
131+
isComment _ = False
132+
127133
-- Render a list given the separator between items
128134
renderList :: Doc -> Ann Token -> Items Term -> Ann Token -> Doc
129135
renderList itemSep paropen@Ann{trailComment = post} items parclose =
130136
pretty (paropen{trailComment = Nothing})
131137
<> surroundWith sur (nest $ pretty post <> renderItems itemSep items)
132138
<> pretty parclose
133139
where
134-
-- If the brackets are on different lines, keep them like that
135140
sur
136-
| sourceLine paropen /= sourceLine parclose = hardline
137-
| null $ unItems items = hardspace
141+
| sourceLine paropen /= sourceLine parclose = hardline -- If the brackets are on different lines, keep them like that
142+
| hasOnlyComments items = hardline -- If the list has only comments, use hardline to ensure idempotency. https://github.com/NixOS/nixfmt/issues/362
143+
| hasTrivia paropen && null items = hardline -- even if the comment got associated with the opening bracket, keep the hardline
144+
| null items = hardspace -- making sure we're not potentially adding extra newlines for empty lists
138145
| otherwise = line
139146

140147
instance Pretty Trivia where
@@ -597,6 +604,11 @@ isAbsorbable (Set _ (Ann{sourceLine = line1}) (Items []) (Ann{sourceLine = line2
597604
| line1 /= line2 = True
598605
isAbsorbable (List (Ann{sourceLine = line1}) (Items []) (Ann{sourceLine = line2}))
599606
| line1 /= line2 = True
607+
-- Lists/sets with only comments are absorbable (https://github.com/NixOS/nixfmt/issues/362)
608+
isAbsorbable (List paropen items _)
609+
| hasTrivia paropen || hasOnlyComments items = True
610+
isAbsorbable (Set _ paropen items _)
611+
| hasTrivia paropen || hasOnlyComments items = True
600612
isAbsorbable (Parenthesized (LoneAnn _) (Term t) _) = isAbsorbable t
601613
isAbsorbable _ = False
602614

‎test/diff/attr_set/in.nix‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,61 @@
327327
++ baz # newline!
328328
++ meow;
329329

330+
# https://github.com/NixOS/nixfmt/issues/362
331+
foobar = [ /* foobar */ ];
332+
333+
foobar =
334+
[
335+
# foobar
336+
];
337+
338+
foobar =
339+
[ # foobar
340+
];
341+
342+
foobar =
343+
[ # foobar
344+
];
345+
346+
foobar = [
347+
# foobar
348+
];
349+
350+
foobar2 = [ /* foobar */
351+
352+
];
353+
354+
foobar3 = [
355+
356+
/* foobar */ ];
357+
358+
foobar = { /* foobar */ };
359+
360+
foobar =
361+
{
362+
# foobar
363+
};
364+
365+
foobar =
366+
{ # foobar
367+
};
368+
369+
foobar =
370+
{ # foobar
371+
};
372+
373+
foobar = {
374+
# foobar
375+
};
376+
377+
foobar2 = { /* foobar */
378+
379+
};
380+
381+
foobar3 = {
382+
383+
/* foobar */ };
384+
330385
environment.systemPackages =
331386
# Include the PAM modules in the system path mostly for the manpages.
332387
[ package ]

‎test/diff/attr_set/out-pure.nix‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,67 @@
432432
++ baz # newline!
433433
++ meow;
434434

435+
# https://github.com/NixOS/nixfmt/issues/362
436+
foobar = [
437+
# foobar
438+
];
439+
440+
foobar = [
441+
# foobar
442+
];
443+
444+
foobar = [
445+
# foobar
446+
];
447+
448+
foobar = [
449+
# foobar
450+
];
451+
452+
foobar = [
453+
# foobar
454+
];
455+
456+
foobar2 = [
457+
# foobar
458+
459+
];
460+
461+
foobar3 = [
462+
463+
# foobar
464+
];
465+
466+
foobar = {
467+
# foobar
468+
};
469+
470+
foobar = {
471+
# foobar
472+
};
473+
474+
foobar = {
475+
# foobar
476+
};
477+
478+
foobar = {
479+
# foobar
480+
};
481+
482+
foobar = {
483+
# foobar
484+
};
485+
486+
foobar2 = {
487+
# foobar
488+
489+
};
490+
491+
foobar3 = {
492+
493+
# foobar
494+
};
495+
435496
environment.systemPackages =
436497
# Include the PAM modules in the system path mostly for the manpages.
437498
[ package ] ++ lib.optional config.users.ldap.enable pam_ldap;

‎test/diff/attr_set/out.nix‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,67 @@
449449
++ baz # newline!
450450
++ meow;
451451

452+
# https://github.com/NixOS/nixfmt/issues/362
453+
foobar = [
454+
# foobar
455+
];
456+
457+
foobar = [
458+
# foobar
459+
];
460+
461+
foobar = [
462+
# foobar
463+
];
464+
465+
foobar = [
466+
# foobar
467+
];
468+
469+
foobar = [
470+
# foobar
471+
];
472+
473+
foobar2 = [
474+
# foobar
475+
476+
];
477+
478+
foobar3 = [
479+
480+
# foobar
481+
];
482+
483+
foobar = {
484+
# foobar
485+
};
486+
487+
foobar = {
488+
# foobar
489+
};
490+
491+
foobar = {
492+
# foobar
493+
};
494+
495+
foobar = {
496+
# foobar
497+
};
498+
499+
foobar = {
500+
# foobar
501+
};
502+
503+
foobar2 = {
504+
# foobar
505+
506+
};
507+
508+
foobar3 = {
509+
510+
# foobar
511+
};
512+
452513
environment.systemPackages =
453514
# Include the PAM modules in the system path mostly for the manpages.
454515
[ package ] ++ lib.optional config.users.ldap.enable pam_ldap;

‎test/diff/lists/in.nix‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,15 @@
8282

8383
]
8484

85+
[ /* foobar */ ]
86+
87+
[ # foobar
88+
]
89+
90+
[
91+
# foobar
92+
]
93+
8594

8695
[ [ multi line ] ]
8796
[ [ [ singleton ] ] ]

‎test/diff/lists/out-pure.nix‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,18 @@
108108

109109
]
110110

111+
[
112+
# foobar
113+
]
114+
115+
[
116+
# foobar
117+
]
118+
119+
[
120+
# foobar
121+
]
122+
111123
[
112124
[
113125
multi

‎test/diff/lists/out.nix‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,18 @@
111111

112112
]
113113

114+
[
115+
# foobar
116+
]
117+
118+
[
119+
# foobar
120+
]
121+
122+
[
123+
# foobar
124+
]
125+
114126
[
115127
[
116128
multi

0 commit comments

Comments
 (0)