BUG (regression?): Identical values from multiple aspects conflict

[dnlzro]

Description

When two aspects define the same value on an option, I'd expect those values to merge without issue. But after a recent change (I think), mergeableType in nix/lib/aspects/fx/edges/route.nix is throwing on equal values.

e.g., I had configured enable = true on the same option across multiple aspects, which was fine before, but now my config is broken.

Reproduction

{ denTest, inputs, ... }:
{
  flake.tests.scalar-merge = {
    test-identical-derivations-merge = denTest (
      { config, den, ... }:
      {
        imports = [ inputs.den.flakeOutputs.packages ];

        den.hosts.x86_64-linux.igloo.users.tux = { };

        den.aspects.pkg-one.packages =
          { pkgs, ... }: { shared = pkgs.writeText "shared" "same"; };
        den.aspects.pkg-two.packages =
          { pkgs, ... }: { shared = pkgs.writeText "shared" "same"; };

        den.schema.flake-system.includes = [
          den.aspects.pkg-one
          den.aspects.pkg-two
        ];

        expr = builtins.readFile config.flake.packages.x86_64-linux.shared;
        expected = "same";
      }
    );
  };
}

This fails with den: the option 'shared' has conflicting definitions from multiple aspects.

[sini]

I'll take a look. It's possible it's related to the recently landed #671.

[dnlzro]

I can confirm that pinning Den to commit 28f6b2e (just before #671 landed) fixes the issue. But it looks like #671 just revealed a bug that has been there since #574.

[sini]

This isn't going to be a quick fix as it touches on how nixpkgs's eval modules works and functions in nix don't carry identity and closures break the eval plane -- this becomes an eval within nixpkgs and outside of den's context. I would say it's a wont-fix but I'm working on something that can potentially resolve this and will keep you posted. I'll ask an LLM to review it in case there's something simple I'm missing but I think there's a deeper issue here.

[sini]

Fixed in #675. Thank you for the clean repro -- it went into the test suite unchanged, as a regression test.

Your bisect is correct on both points.

What went wrong

Two aspects can write a definition to the same option. Den merges those definitions. The merge function looks into the two values. Attrsets merge key by key. Lists join end to end. All other values must agree, or den reports a conflict.

A derivation is an attrset. Den does not merge a derivation key by key, and that is deliberate: two packages must not mix their fields. A derivation therefore never reaches the attrset branch.

This is what broke your configuration. Two aspects built the same package. The two definitions agreed. But den did not test for agreement, so it reported a conflict.

#574 added this merge function. That commit says the function follows NixOS module semantics. It does not. NixOS uses mergeEqualOption. That function accepts definitions that agree, and reports a conflict only when the definitions differ. So the fault came in with #574, and #671 only made the path reachable for your configuration.

The fix

Den now accepts definitions that all hold the same value. Den makes this test last, immediately before the conflict error. Den therefore reads the values only on the path that failed before.

What the fix does not do

The fix compares two values. In Nix, this comparison gives no useful answer for a function. These are measurements against the current nixpkgs:

expression	result
(x: x) == (x: x)	false
let f = x: x; in f == f	false
{ a = 1; f = x: x; } == { a = 1; f = x: x; }	false

A function is never equal to a function. This includes a comparison of a function with itself. One function anywhere in an attrset makes the full comparison false.

A derivation does not have this problem. Nix compares two derivations by their output path. This is why your example is fully repairable.

If your configuration holds a function at the option, and not a derivation, the conflict continues. Please test against the branch before I close this issue. If the conflict continues, please send me that repro.

Why den cannot do more here

Nix does not let you look inside a function. No builtin gives you the body of a function. No builtin gives you the values that the function captured from around it. There is thus nothing to compare, and nothing to hash.

Den could build an identity from the parts that it can see. The argument names are one example. This is not safe. Two different functions can have the same argument names. Den would then treat the two functions as equal. Den would keep one definition, discard the other, and give you no message.

That result is bug #572. The merge function exists to prevent it.

The two possible mistakes do not cost the same. If den says "these agree" and den is wrong, you get a wrong configuration and no message. If den says "these differ" and den is wrong, you get an error message. The second mistake is much cheaper. The current code therefore refuses to answer.

The longer-term fix

Den must stop calculating the identity of a function from the function. Den must carry the identity together with the function.

The theory is Palmer's 2024 work on intensional functions. In that model, a function carries two things: its name, and the values that it captured. You can read both as ordinary data. You then compare those records. You do not compare the functions.

Palmer's model uses the compiler to collect the captured values. Nix cannot do this. So the version that I can build must be an approximation. The author of the function states its identity. The module system then holds the author to that statement.

This is a weaker guarantee than the paper gives, and I prefer to say so. The guarantee comes from a contract with the author. It does not come from an inspection of the function.

The new module system already has one half of this. It refuses to give an identity to a function, and it does not invent one. The second half is not complete. That half lets a function declare its identity, so that den can compare two declarations.

I will keep this thread posted.

[dnlzro]

Awesome, thank you! I don't anticipate ever running into that issue with lambdas, so for my purposes, I would consider this issue closed. But I'll leave that up to you if you want to keep it open for tracking.

[sini]

No worries. I'll probably close it and leave the comment -- my LLM got a bit eager in posting a response. This is the paper I've been working on implementing (in addition to some other papers) for the nix ecosystem. :)

https://dl.acm.org/doi/10.1145/3689714