I tried this on my personal flake and nixfmt
complained about a couple of files that created a different AST after formatting.
Shall I create an additional bugreport as mentioned in the warning?
I tried this on my personal flake and nixfmt
complained about a couple of files that created a different AST after formatting.
Shall I create an additional bugreport as mentioned in the warning?
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
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.
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.
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.
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.
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.
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"
+ ]
+ )
];
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.
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).
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.
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
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
:
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)
More improvements have been made to the implementation, feel free to give it a shot:
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?
Please read the corresponding RFC. But in short:
Let’s clean up these folks to avoid reformat-induced rebuilds!