Skip to content

Keep trailing comment in { and [ #324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Keep trailing comment in { and [ #324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/Nixfmt/Pretty.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,10 @@ prettySet _ (krec, paropen@(LoneAnn _), Items [], parclose@Ann{preTrivia = []})
sep = if sourceLine paropen /= sourceLine parclose then hardline else hardspace
-- Singleton sets are allowed to fit onto one line,
-- but apart from that always expand.
prettySet wide (krec, paropen@Ann{trailComment = post}, binders, parclose) =
prettySet wide (krec, paropen, binders, parclose) =
pretty (fmap (,hardspace) krec)
<> pretty (paropen{trailComment = Nothing})
<> surroundWith sep (nest $ pretty post <> prettyItems binders)
<> pretty paropen
<> surroundWith sep (nest $ prettyItems binders)
<> pretty parclose
where
sep =
Expand Down Expand Up @@ -223,9 +223,9 @@ prettyTerm (List paropen@Ann{trailComment = Nothing} (Items []) parclose@Ann{pre
sep = if sourceLine paropen /= sourceLine parclose then hardline else hardspace
-- General list
-- Always expand if len > 1
prettyTerm (List paropen@Ann{trailComment = post} items parclose) =
pretty (paropen{trailComment = Nothing})
<> surroundWith sur (nest $ pretty post <> prettyItems items)
prettyTerm (List paropen items parclose) =
pretty paropen
<> surroundWith sur (nest $ prettyItems items)
<> pretty parclose
where
-- If the brackets are on different lines, keep them like that
Expand Down
21 changes: 7 additions & 14 deletions test/diff/attr_set/out-pure.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
[
{ }
{
# a
{ # a
}
{ a = 1; }
{ a = 1; }
Expand All @@ -21,25 +20,21 @@
{
b = 1; # c
}
{
# a
{ # a
b = 1;
}
{
# a
{ # a
b = 1; # c
}

rec { c = 1; }
rec {
c = 1; # d
}
rec {
# b
rec { # b
c = 1;
}
rec {
# b
rec { # b
c = 1; # d
}
rec # a
Expand All @@ -51,13 +46,11 @@
c = 1; # d
}
rec # a
{
# b
{ # b
c = 1;
}
rec # a
{
# b
{ # b
c = 1; # d
}

Expand Down
21 changes: 7 additions & 14 deletions test/diff/attr_set/out.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
[
{ }
{
# a
{ # a
}
{ a = 1; }
{
Expand All @@ -24,25 +23,21 @@
{
b = 1; # c
}
{
# a
{ # a
b = 1;
}
{
# a
{ # a
b = 1; # c
}

rec { c = 1; }
rec {
c = 1; # d
}
rec {
# b
rec { # b
c = 1;
}
rec {
# b
rec { # b
c = 1; # d
}
rec # a
Expand All @@ -54,13 +49,11 @@
c = 1; # d
}
rec # a
{
# b
{ # b
c = 1;
}
rec # a
{
# b
{ # b
c = 1; # d
}

Expand Down
6 changes: 2 additions & 4 deletions test/diff/comment/out-pure.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@
```
*/

[
# 1
[ # 1
#2
a # 3
b
Expand All @@ -121,8 +120,7 @@
a = 123; # comment
}

{
# 1
{ # 1
#2
a = 1; # 3
b = 1;
Expand Down
6 changes: 2 additions & 4 deletions test/diff/comment/out.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@
```
*/

[
# 1
[ # 1
#2
a # 3
b
Expand All @@ -121,8 +120,7 @@
a = 123; # comment
}

{
# 1
{ # 1
#2
a = 1; # 3
b = 1;
Expand Down
3 changes: 1 addition & 2 deletions test/diff/idioms_lib_4/out-pure.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,8 +588,7 @@ rec {
families = { };
};
}
// {
# aliases
// { # aliases
# 'darwin' is the kernel for all of them. We choose macOS by default.
darwin = kernels.macos;
watchos = kernels.ios;
Expand Down
3 changes: 1 addition & 2 deletions test/diff/idioms_lib_4/out.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,8 +588,7 @@ rec {
families = { };
};
}
// {
# aliases
// { # aliases
# 'darwin' is the kernel for all of them. We choose macOS by default.
darwin = kernels.macos;
watchos = kernels.ios;
Expand Down
3 changes: 1 addition & 2 deletions test/diff/idioms_pkgs_4/out-pure.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,7 @@ in
cc =
let
nativePrefix =
{
# switch
{ # switch
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these test cases have two spaces

Suggested change
{ # switch
{ # switch

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about that, too - shouldn't nixfmt remove this extra whitespace?

Copy link
Member Author

@piegamesde piegamesde Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is introduced deliberately here in order to prevent idempotency issues in some very rare special cases. The test suite may not make it look like it but the entirety of Nixpkgs hits that edge case only once. Take a look at the input here:

nativePrefix = { # switch
  i686-solaris = "/usr/gnu";
  x86_64-solaris = "/opt/local/gcc47";
}.${system} or "/usr";

Naively, it would be formatted like this:

nativePrefix =
  { # switch
    i686-solaris = "/usr/gnu";
    x86_64-solaris = "/opt/local/gcc47";
  }
  .${system} or "/usr";

Because the comment and i686-solaris are vertically aligned, the comment will be associated with the i686-solaris and not with the {. This kind of respecting vertical alignment is necessary because too many people write code like:

foo = # comment
      and now the code

and without respecting that vertical alignment the comment would not be associated with its correct code. Anyways, because of this, a second formatting would wrongly associate the switch comment with solaris (which was not the intent of the original code), and thus produce

nativePrefix =
  {
    # switch
    i686-solaris = "/usr/gnu";
    x86_64-solaris = "/opt/local/gcc47";
  }
  .${system} or "/usr";

which is different from the output after one formatting, and thus an idempotency issue. The "fix" is to insert one single space before comments that should not be aligned with the line that follows, so that they will be parsed correctly:

nativePrefix =
  {  # switch
    i686-solaris = "/usr/gnu";
    x86_64-solaris = "/opt/local/gcc47";
  }
  .${system} or "/usr";

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Knowing it is a pre-existing behaviour makes me feel better. Although I'm unsure how I feel about the ugly hack now being more exposed by this PR.

I guess, although it is covered a lot in our test suite, in reality it should be fairly rare to run into comments that need to be explicitly mis-aligned with the following value?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naively, it would be formatted like this:

nativePrefix =
  { # switch
    i686-solaris = "/usr/gnu";
    x86_64-solaris = "/opt/local/gcc47";
  }
  .${system} or "/usr";

Because the comment and i686-solaris are vertically aligned, the comment will be associated with the i686-solaris and not with the {. This kind of respecting vertical alignment is necessary because too many people write code like:

foo = # comment
      and now the code

and without respecting that vertical alignment the comment would not be associated with its correct code.

I argue that in both cases the comment belongs to the whole block and should be formatted like this:

nativePrefix =
  # switch
  {
    i686-solaris = "/usr/gnu";
    x86_64-solaris = "/opt/local/gcc47";
  }
  .${system} or "/usr";

and respectively:

# comment
foo =
  and now the code

Aka: The rule "an end-of-line comment belongs to this line" should have priority over the "aligned comments" rule. Aligned comments should only be "only a comment on this line, nothing else" comments.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I argue that in both cases the comment belongs to the whole block and should be formatted like this:

Personally, I agree. However if I understand @piegamesde correctly, this may not be realistically achievable without significant work.

Apparently, the code responsible doesn't have any knowledge of the variable binding itself, or what lines surround the comment tokens.

i686-solaris = "/usr/gnu";
x86_64-solaris = "/opt/local/gcc47";
}
Expand Down
3 changes: 1 addition & 2 deletions test/diff/idioms_pkgs_4/out.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ in
cc =
let
nativePrefix =
{
# switch
{ # switch
i686-solaris = "/usr/gnu";
x86_64-solaris = "/opt/local/gcc47";
}
Expand Down
15 changes: 5 additions & 10 deletions test/diff/lists/out-pure.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@
foo2 = "barbar";
}
]
{
# List in attrset with comment
{ # List in attrset with comment

imports0 = [ ];

Expand Down Expand Up @@ -66,23 +65,19 @@
b # c
d # e
]
[
# a
[ # a
b
d
]
[
# a
[ # a
b
d # e
]
[
# a
[ # a
b # c
d
]
[
# a
[ # a
b # c
d # e
]
Expand Down
15 changes: 5 additions & 10 deletions test/diff/lists/out.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@
foo2 = "barbar";
}
]
{
# List in attrset with comment
{ # List in attrset with comment

imports0 = [ ];

Expand Down Expand Up @@ -69,23 +68,19 @@
b # c
d # e
]
[
# a
[ # a
b
d
]
[
# a
[ # a
b
d # e
]
[
# a
[ # a
b # c
d
]
[
# a
[ # a
b # c
d # e
]
Expand Down
21 changes: 7 additions & 14 deletions test/diff/monsters_4/out-pure.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@
}: # Foo
stdenv.mkDerivation # Foo
rec # Foo
{
# Foo
{ # Foo
pname # Foo
= # Foo
"contrast"; # Foo
Expand All @@ -45,8 +44,7 @@ stdenv.mkDerivation # Foo
src # Foo
= # Foo
# Foo
fetchFromGitLab {
# Foo
fetchFromGitLab { # Foo
domain # Foo
= # Foo
"gitlab.gnome.org"; # Foo
Expand All @@ -69,8 +67,7 @@ stdenv.mkDerivation # Foo
cargoDeps # Foo
= # Foo
rustPlatform.fetchCargoTarball # Foo
{
# Foo
{ # Foo
inherit # Foo
src
; # Foo
Expand All @@ -83,8 +80,7 @@ stdenv.mkDerivation # Foo
}; # Foo
nativeBuildInputs # Foo
= # Foo
[
# Foo
[ # Foo
desktop-file-utils # Foo
gettext # Foo
meson # Foo
Expand All @@ -100,8 +96,7 @@ stdenv.mkDerivation # Foo
]; # Foo
buildInputs # Foo
= # Foo
[
# Foo
[ # Foo
cairo # Foo
glib # Foo
gtk4 # Foo
Expand All @@ -120,8 +115,7 @@ stdenv.mkDerivation # Foo
= # Foo
with # Foo
lib; # Foo
{
# Foo
{ # Foo
description # Foo
= # Foo
"Checks whether the contrast between two colors meet the WCAG requirements"; # Foo
Expand All @@ -135,8 +129,7 @@ stdenv.mkDerivation # Foo
= # Foo
with # Foo
maintainers; # Foo
[
# Foo
[ # Foo
jtojnar # Foo
]; # Foo
platforms # Foo
Expand Down
Loading