From 157c95de4d8c582d21b7f3ec4e91da7853f75b6c Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Tue, 27 May 2025 00:33:49 +0200
Subject: [PATCH] Make formatter not strict in all systems and try to...

... avoid broken attributes in cases where we can predict that
we don't have any.

This is far from ideal, but it's the best we can do.
If it becomes a shit show, flake authors can opt out.
---
 dev/tests/nix-unit.nix | 64 +++++++++++++++++++++++++++++++++++++
 modules/formatter.nix  | 72 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/dev/tests/nix-unit.nix b/dev/tests/nix-unit.nix
index e9e00d6..412b2a0 100644
--- a/dev/tests/nix-unit.nix
+++ b/dev/tests/nix-unit.nix
@@ -464,4 +464,68 @@ in
       expected = "dogfood";
     };
   };
+
+  formatter = {
+    "test: conditional null throws helpful error" = {
+      expr =
+        let
+          result = mkFlake { inputs.self = { }; } {
+            systems = [ "x86_64-linux" "aarch64-darwin" ];
+            perSystem = { system, ... }: {
+              formatter =
+                if system == "x86_64-linux" then
+                  derivation { name = "fmt"; builder = "x"; system = system; }
+                else null;
+            };
+          };
+        in
+        result.formatter.aarch64-darwin;
+      expectedError.type = "ThrownError";
+      expectedError.msg = "could not determine statically(.|\n)*disabledModules(.|\n)*inputs\\.flake-parts\\.modules\\.formatter";
+    };
+
+    "test: empty when never defined" = {
+      expr =
+        let
+          result = mkFlake { inputs.self = { }; } {
+            systems = [ "x86_64-linux" "aarch64-darwin" ];
+            perSystem = { ... }: { };
+          };
+        in
+        result.formatter == { };
+      expected = true;
+    };
+
+    "test: present for all systems" = {
+      expr =
+        let
+          result = mkFlake { inputs.self = { }; } {
+            systems = [ "x86_64-linux" "aarch64-darwin" ];
+            perSystem = { system, ... }: {
+              formatter = derivation { name = "fmt"; builder = "x"; inherit system; };
+            };
+          };
+        in
+        result ? formatter && result.formatter ? x86_64-linux && result.formatter ? aarch64-darwin;
+      expected = true;
+    };
+
+    "test: lazy per-system" = {
+      expr =
+        let
+          result = mkFlake { inputs.self = { }; } {
+            systems = [ "x86_64-linux" "aarch64-darwin" ];
+            perSystem = { system, ... }: {
+              formatter =
+                if system == "x86_64-linux" then
+                  derivation { name = "fmt"; builder = "x"; system = system; }
+                else
+                  throw "should not evaluate aarch64-darwin formatter";
+            };
+          };
+        in
+        result.formatter.x86_64-linux.name;
+      expected = "fmt";
+    };
+  };
 }
diff --git a/modules/formatter.nix b/modules/formatter.nix
index e2959ed..36c6fa8 100644
--- a/modules/formatter.nix
+++ b/modules/formatter.nix
@@ -1,7 +1,6 @@
 { config, lib, flake-parts-lib, ... }:
 let
   inherit (lib)
-    filterAttrs
     mapAttrs
     mkOption
     optionalAttrs
@@ -10,11 +9,41 @@ let
   inherit (flake-parts-lib)
     mkPerSystemOption
     ;
+
+  # Do not copy this pattern! (probe, haveFormatterProbably, optionalAttrs)
+  # It kind of works somewhat for `formatter`, but it is bad.
+  # Nothing critical must rely on this!
+  # - `tryEval` makes debugging harder and more annoying
+  # - There's a performance cost to this otherwise useless evaluation
+  # - We only use it *in `formatter`* because the effects are limited to that
+  #   particular attribute. This makes the risk somewhat manageable.
+  probe =
+    config.perSystem
+      # Elaborate error message that users should never see.
+      # The reason to even do this, is that we can sidestep
+      # https://github.com/hercules-ci/flake-parts/issues/288 in many cases,
+      # without flake author intervention to keep `nix flake check` happy.
+      (throw ''
+        For the purpose of finding out whether an option may be unset for all systems, flake-parts probes the perSystem module without a valid `system` argument, and tries to catch this exception if it finds that `system` is required for this determination. If you see this message, it means that for some reason, flake-parts was unable to catch this exception.
+
+        This may be a bug in Nix or in flake-parts, but ultimately this is due to the quirky requirement of flakes that the "system" attribute does not come first.
+        Flake-parts tries its best to correct that UX, but ultimately, this needs to be solved in Nix.
+      '');
+
+  haveFormatterProbably =
+    let
+      ev =
+        builtins.tryEval
+          probe.formatter;
+    in
+    # If it fails, we can't assume that we don't have a formatter, because it may well evaluate for a real system.
+    !ev.success
+    || ev.value != null;
 in
 {
   options = {
     flake.formatter = mkOption {
-      type = types.lazyAttrsOf types.package;
+      type = types.lazyAttrsOf (types.nullOr types.package);
       default = { };
       description = ''
         An attribute set of per system a package used by [`nix fmt`](https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-fmt.html).
@@ -36,12 +65,39 @@ in
   };
   config = {
     flake.formatter =
-      mapAttrs
-        (k: v: v.formatter)
-        (filterAttrs
-          (k: v: v.formatter != null)
-          config.allSystems
-        );
+      # Work around `nix flake check` not allowing `null` values in output attributes.
+      optionalAttrs
+        haveFormatterProbably
+        (mapAttrs
+          (k: v:
+            if v.formatter != null then v.formatter
+            else
+              throw ''
+                flake-parts could not determine statically that no formatter is defined for *all* systems.
+
+                What happened?
+
+                1. For performance reasons, flake-parts must not query `perSystem` for every system, so it uses a heuristic to determine whether a formatter is defined for all systems.
+                2. Unfortunately, this heuristic is not perfect, and it wasn't able to determine for your flake that `perSystem.formatter` is always `null` (if it even is always `null`).
+                As a consequence of (1), flake-parts had to provide the output attribute `formatter.${k}`, but as a consequence of (2), you're seeing this error.
+
+                What to do?
+
+                This whole situation should be temporary. `nix flake check`/`show` can be changed to allow `null` values, which gives flake-parts and other frameworks a way to avoid this situation.
+
+                If you don't want a `formatter` output attribute, you may remove it in your `mkFlake` call.
+                (`disabledModules` must not be used in modules that are intended for reuse)
+
+                    { inputs, ... }:
+                    {
+                      disabledModules = [
+                        inputs.flake-parts.modules.formatter
+                      ];
+                      # perSystem = ...; etc
+                    }
+
+              '')
+          config.allSystems);
 
     perInput = system: flake:
       optionalAttrs (flake?formatter.${system}) {