Call for testing: Nix formatter

[piegames]

RFC 166 is slowly approaching the finish line. We are currently ironing out the last kinks in preparation for going into FCP hopefully soon.

The implementation of the RFC format has been added to Nixpkgs as nixfmt-rfc-style. So now is the best time to try it out and dogfood it at scale, so that we catch all the weird corner cases before applying it to the entire Nixpkgs.

You can easily run nixfmt like any other formatter with treefmt. For now, please run it with the --verify flag, which will do basic correctness checks on the formatted output to catch bugs.

[NobbZ]

I tried this on my personal flake and nixfmt complained about a couple of files that created a different AST after formatting.

https://gist.github.com/NobbZ/01330e8da7ac740c7b1b49432b8c1a76

Shall I create an additional bugreport as mentioned in the warning?

[piegames]

This looks like a bug in the verification check. Please try out the following patch:

diff --git a/src/Nixfmt/Types.hs b/src/Nixfmt/Types.hs
index e3719ab..03e75d3 100644
--- a/src/Nixfmt/Types.hs
+++ b/src/Nixfmt/Types.hs
@@ -145,7 +145,15 @@ data Expression
 -- initial trivia.
 data Whole a
     = Whole a Trivia
-    deriving (Eq, Show)
+
+-- | Equality of annotated syntax is defined as equality of their corresponding
+-- semantics, thus ignoring the annotations.
+instance Eq a => Eq (Whole a) where
+    Whole x _ == Whole y _ = x == y
+
+-- Trivia is ignored for Eq, so also don't show
+instance Show a => Show (Whole a) where
+    show (Whole a _) = show a
 
 type File = Whole Expression

[Eriim]

I’m sure that this isn’t the number one priority for this stage of the project but it would be great if we could retain the stdout that nixpkgs-fmt produces to let the user know which files (if any) have been formatted.

[elaforge]

Can you give some more details, like how to actually run this? I figured it out by copy pasting the flake command above, and the PR has some references, but probably more people will test it if they have an easy way to do it.

Anyway, I had a quick glance and it mostly looks pretty good, except function pattern match arguments get unwrapped in a way that looks pretty bad. This:

let
  function1 =
    { arg1
    , arg2
    # comment on arg3
    , arg3
    , arg4
    }: arg1 + arg2;

  function2 =
    { deps
    , srcs ? [ ]
    # comment
    , outputs ? [ "out "]
    # comment
    , moreArgs
    # comment
    , blah
    }: 1;
in 1

becomes

let
  function1 = { arg1, arg2
    # comment on arg3
    , arg3, arg4 }:
    arg1 + arg2;

  function2 = { deps, srcs ? [ ]
      # comment
    , outputs ? [
      "out "
    ]
    # comment
    , moreArgs
    # comment
    , blah }:
    1;
in 1

The RFC seems to favor a trailing commas style, but this doesn’t fare very well either:

let
  function1 = {
    arg1,
    arg2,
    # comment on arg3
    arg3,
    arg4,
  }: arg1 + arg2;

  function2 = {
    deps,
    srcs ? [ ],
    # comment
    outputs ? [ "out "],
    # comment
    moreArgs,
    # comment
    blah,
  }: 1;
in 1

becomes

let
  function1 = { arg1, arg2,
    # comment on arg3
    arg3, arg4, }:
    arg1 + arg2;

  function2 = { deps, srcs ? [ ],
    # comment
    outputs ? [ "out " ],
    # comment
    moreArgs,
    # comment
    blah, }:
    1;
in 1

Presumably it should notice that the whole {...}: syntax doesn’t fit on one line, and then wrap each element consistently, rather than trying to fit as many as possible on each line.

The other major issue is that lists get unwrapped when they shouldn’t be. Unfortunately it’s impossible for a formatter to know which lists are small and will remain small, and which ones will be frequently added to and removed from, so should be one-per-line, that’s domain knowledge. As a workaround, I’ve taken to putting in a comment to disable the unwrap, e.g.:

deps = [
  # don't unwrap me nixfmt
  "a"
  "b"
];

So this isn’t anything nixfmt could fix, just a note of a workaround.

[piegames]

Hi, it looks like you ran the “original” Nixfmt, before it got modified to match the RFC style. The easiest way to get is to simply use the nixfmt-rfc-style package from Nixpkgs. If you want the latest patches then check out this branch instead: GitHub - piegamesde/nixfmt at rfc101-style. Unfortunately I can’t help you with Flakes, sorry.

[VTimofeenko]

Are there plans to not write back file if it is already formatted? It breaks the treefmt specification:

Whenever there is a change to the code formatting, the code formatter MUST write to the changes back to the original location.

If there is no changes to the original file, the formatter MUST NOT write to the original location

In case treefmt is invoked by a pre-commit hook, this causes false negative failures.

To illustrate:

❯ nix shell "github:piegamesde/nixfmt?ref=rfc101-style"
❯ cat sample.nix
{ }
❯ stat --format="[Just created] Change: %z" ./sample.nix
[Just created] Change: 2024-02-08 12:43:38.863146206 -0800

❯ nixfmt --verify ./sample.nix
❯ stat --format="[After nixfmt verify] Change: %z" ./sample.nix
[After nixfmt verify] Change: 2024-02-08 12:47:53.098421081 -0800

❯ nixfmt ./sample.nix                                         
❯ stat --format="[After nixfmt] Change: %z" ./sample.nix      
[After nixfmt] Change: 2024-02-08 12:48:05.108901076 -0800

Not sure if they are using stat to check for changes, this is just to illustrate that the file is changing on the filesystem.

It is possible to use a somewhat hacky workaround but it seems a bit wasteful to run nixfmt twice and de-parallelize its execution.

[piegames]

See `numtide/treefmt` compatibility · Issue #88 · serokell/nixfmt · GitHub. This has been fixed on master already, and will thus be incorporated once the RFC branch gets merged.

[sorrel]

I got a warning about idempotency from running nix run nixpkgs#nixfmt-rfc-style -- --verify . in https://github.com/sersorrel/sys (using nixpkgs e92b6015881907e698782c77641aa49298330223; nixfmt reports itself as nixfmt v0.5.0) – happy to open an issue if you want, but unsure where to do so:

./flake.nix: Nixfmt is not idempotent. This is a bug in nixfmt. Please report it at https://github.com/serokell/nixfmt

After one formatting:
{
  description = "dependency flake for my system configurations";

  inputs = {
  };

  outputs = inputs: {

    darwinModules.default =
      { ... }:
      {
        imports = inputs.self.lib.importDir ./darwin;
        _module.args.sysDir = ./.;
      };

    homeModules.default =
      { lib, ... }:
      {
        _module.args.sysDir = ./.;
        imports = inputs.self.lib.importDir ./hm;
        programs.home-manager.enable = true;
        home.enableNixpkgsReleaseCheck = true;
      };

    nixosModules.default =
      { lib, ... }:
      {
        _module.args.sysDir = ./.;
        imports = inputs.self.lib.importDir ./nixos;
        boot.loader.grub.configurationLimit = 30;
        boot.loader.systemd-boot.configurationLimit = 30;
        boot.tmp.cleanOnBoot = true;
        environment.shellAliases = {
          l = null;
          ll = null;
          ls = null;
        };
        systemd.network.wait-online.anyInterface = true;
        users.mutableUsers = false;
      };

    lib =
      let
        inherit (builtins)
          attrNames
          concatMap
          stringLength
          substring
          ;
        concatMapAttrsToList = f: attrs: concatMap (name: f name attrs.${name}) (attrNames attrs); # see lib.mapAttrsToList
        hasPrefix = prefix: s: substring 0 (stringLength prefix) s == prefix; # from nixpkgs
        hasSuffix =
          suffix: s:
          let
            length = stringLength s;
            suffixLength = stringLength suffix;
          in
          length >= suffixLength && substring (length - suffixLength) length s == suffix; # from nixpkgs
      in
      {
        importDir =
          dir:
          concatMapAttrsToList
            (
              name: value:
              if
                (value == "regular" && hasSuffix ".nix" name) || (value == "directory" && !hasPrefix "." name)
              then
                [ (dir + "/${name}") ]
              else
                [ ]
            )
            (builtins.readDir dir);
      };
  };
}


After two:
{
  description = "dependency flake for my system configurations";

  inputs = { };

  outputs = inputs: {

    darwinModules.default =
      { ... }:
      {
        imports = inputs.self.lib.importDir ./darwin;
        _module.args.sysDir = ./.;
      };

    homeModules.default =
      { lib, ... }:
      {
        _module.args.sysDir = ./.;
        imports = inputs.self.lib.importDir ./hm;
        programs.home-manager.enable = true;
        home.enableNixpkgsReleaseCheck = true;
      };

    nixosModules.default =
      { lib, ... }:
      {
        _module.args.sysDir = ./.;
        imports = inputs.self.lib.importDir ./nixos;
        boot.loader.grub.configurationLimit = 30;
        boot.loader.systemd-boot.configurationLimit = 30;
        boot.tmp.cleanOnBoot = true;
        environment.shellAliases = {
          l = null;
          ll = null;
          ls = null;
        };
        systemd.network.wait-online.anyInterface = true;
        users.mutableUsers = false;
      };

    lib =
      let
        inherit (builtins)
          attrNames
          concatMap
          stringLength
          substring
          ;
        concatMapAttrsToList = f: attrs: concatMap (name: f name attrs.${name}) (attrNames attrs); # see lib.mapAttrsToList
        hasPrefix = prefix: s: substring 0 (stringLength prefix) s == prefix; # from nixpkgs
        hasSuffix =
          suffix: s:
          let
            length = stringLength s;
            suffixLength = stringLength suffix;
          in
          length >= suffixLength && substring (length - suffixLength) length s == suffix; # from nixpkgs
      in
      {
        importDir =
          dir:
          concatMapAttrsToList
            (
              name: value:
              if
                (value == "regular" && hasSuffix ".nix" name) || (value == "directory" && !hasPrefix "." name)
              then
                [ (dir + "/${name}") ]
              else
                [ ]
            )
            (builtins.readDir dir);
      };
  };
}

Style-wise, whilst I freely admit I have not been involved in discussions at all, I don’t really understand why it consistently performs this expansion (presumably optimised for nixpkgs derivations, where the number of inputs can easily be enormous), which makes up a huge amount of the diff on my repo:

-{ config, lib, pkgs, ... }:
+{
+  config,
+  lib,
+  pkgs,
+  ...
+}:

when it also wants to compress onto one line things like:

-  config = lib.mkIf config.sys.comma.enable {
-    home.packages = [ pkgs.comma ];
-  };
+  config = lib.mkIf config.sys.comma.enable { home.packages = [ pkgs.comma ]; };

This seems like it is in violation of “The kind of quotes used in strings (" vs '') must be preserved from the input.” (though, I see that the strings section was only added a few days ago, so maybe my version of nixfmt is too old or it’s not yet updated for that):

         character = {
-          success_symbol = ''[\$](bold green)'';
-          error_symbol = ''[\$](bold red)'';
-          vicmd_symbol = ''[:](bold green)'';
+          success_symbol = "[\\$](bold green)";
+          error_symbol = "[\\$](bold red)";
+          vicmd_symbol = "[:](bold green)";
         };

And lastly this seems clearly just worse (solvable with a builtins.split "\\." in lib.doRename and updating all the callers to pass strings, I guess…):

imports = [
-    (lib.mkRenamedOptionModule [ "sys" "disableSudoTimeout" ] [ "sys" "sudo" "disableTimeout" ])
+    (lib.mkRenamedOptionModule
+      [
+        "sys"
+        "disableSudoTimeout"
+      ]
+      [
+        "sys"
+        "sudo"
+        "disableTimeout"
+      ]
+    )
   ];

[fricklerhandwerk]

I agree, it also bothers me that multiline expansion is done indiscriminately. There is a lot of value in one-liners capturing a unit of intent, and breaking that up destroys a lot of coherence in code.

I remember having read somewhere – but don’t know where – that lines should be preserved to follow authors‘ intent as long as they stay within length limits. I’d support that. Then multiline expressions can be expanded following diff-optimising rules.

[dpc]

FWIW. I tried switching from nixpkgs-fmt and I like the generated formatting much better. . I’m not very picky about formatting, as long as it is happening automatically and is somewhat consistent it’s fine with me. Leading to trailing comma switch is much appreciated, as it was inconvenient and annoying (I still just lived with it).

[Arsleust]

The formatting looks generally okay, though as @fricklerhandwerk said I’d prefer it to not break everything into many lines. Some one liners are fine.

Stupid question: would that become a new standard for nixpkgs? Would it be promoted as the official formatting for Nix? e.g. is it intended to be shipped in IDE extensions/LSPs?
I’d be happy if that was the case. Nixpkgs is a bit of a pain because I can’t even use nixpkgs-fmt automatically in there because some parts are not properly formatted.

[Carrot]

ran:
nix run nixpkgs#nixfmt-rfc-style -- --verify .

it errored on a result directory.

nixfmt: ./result/etc/cups/ssl: getDirectoryContents:openDirStream: permission denied (Permission denied)

removed result/ and now it did run

[ReedClanton]

I’d prefer if blank lines at the end were allowed. Also I got this bug.

[chrism]

It seems that the -w flag doesn’t respect its setting. A before formatting pic:

The ruler shown to the right in that screenshot is at 80 columns.

Then after a nixfmt -w80 flake.nix:

[Infinisil]

@Arsleust

Stupid question: would that become a new standard for nixpkgs? Would it be promoted as the official formatting for Nix? e.g. is it intended to be shipped in IDE extensions/LSPs?
I’d be happy if that was the case. Nixpkgs is a bit of a pain because I can’t even use nixpkgs-fmt automatically in there because some parts are not properly formatted.

Check out the RFC, it answers essentially all of these questions!

[piegames]

This is on purpose. Inserting a line break after the = may reduce the line length by a couple of characters, which when the string is very long has little effect apart from making the layout worse.

In your case doing line breaks would actually help to stay below the line length limit, but in practice this is rather uncommon. (Simply by using plain http or git urls instead our custom github scheme would make most lines of your example go over too)

[piegames]

More improvements have been made to the implementation, feel free to give it a shot:

nixfmt-rfc-style: 2024-01-31 -> 2024-03-01

[tazjin]

Trying this on the TVL codebase (cl/11151). We are currently using nixpkgs-fmt, and fairly happy with it.

Without having dove too much into the details yet, the diffstat of -21796 +41991 indicates that this creates significantly more noisy formatting than nixpkgs-fmt (e.g. many fewer cases where multiple tokens are allowed on one line etc.), is that intentional?

Have you run this on nixpkgs to see by how much it increases the total line count there?

[piegames]

Please read the corresponding RFC. But in short:

• Yes, the wide format was chosen by design, and is documented and justified in the RFC text
• Literally every single commit of the implementation within the last ~9months was run against the entirety of Nixpkgs, and the diff was closely inspected each time.
• The RFC has been approved and merged. The overall format is fixed and will not be discussed anymore except for minor details and edge cases. This thread is about the implementation in nixfmt and finding bugs in it before going live on larger code bases.