Behaviour of nested attribute sets depends on definition order

[tazjin]

While implementing attribute set behaviour for Tvix, we have noticed that there is an dependency on definition order in nested attribute sets. For example:

nix-repl> { a.b = 1; a = rec { c = b * 2; }; }.a
error: undefined variable 'b' at (string):1:26

nix-repl> { a = rec { c = b * 2; }; a.b = 1; }.a 
{ b = 1; c = 2; }

The nested attribute set at key a will only be recursive if the first time it is defined is an attribute set literal with the rec keyword. This behaviour seems to emerge from the nested key merging logic that the parser currently has.

As far as I can tell there is no reason why a later definition of a rec set at the same key should not be able to "toggle" the rec then. If that's not desired, I think that nested rec should be forbidden altogether - but this behaviour dependent on definition order seems to go, to quote @sternenseemann, "against the spirit of the language".

[fogti]

I think a clean solution to this would be forbidding mixing a = rec {...} (explicit attribute set) and a.b (inferred attribute set) definitions.

[tazjin]

That'd be easy in Tvix, probably not so much in Nix as the entire nesting is done through a recursive AST rewrite in the parser.

[fogti]

well, we could record how the attrset came into existence, right? (implicit vs explicit), and then enforce every implicit fold to happen only when the initial fold was also implicit, and every explicit assignment must happen to an item not yet initialized.

[fogti]

like

nix-repl> { a = 1; a.b = 2; }.a                  
error: attribute 'a.b' already defined at (string):1:3

       at «string»:1:10:

            1| { a = 1; a.b = 2; }.a
             |          ^

we also can't suddenly change types, so we should be able to differentiate just between two "declaration types" of attrsets while parsing (and discard it as soon as we start evaluating).

[sternenseemann]

@zseri is not wrong. That error is even created as a ParseError (via dupAttr in the snippet below):

https://github.com/NixOS/nix/blob/050fcd391bce0d36893446d4bc975fef4cb38af3/src/libexpr/parser.y#L98-L156

So it should be possible to check if we are merging something rec and non rec and error out on that.

[ncfavier]

We might also consider whether it should even be possible to merge two rec attrsets, as then the recursive scope is split across different parts of the file, intensifying @shlevy's concerns.

I'd go for an error if either attrset is recursive.

[JojOatXGME]

Just noticed this ticket. Was this always possible? I thought have explicitly checked that some years back while working on NixOS/nix-idea. Might have been 2020, with Nix 2.3.

{
  x.a = "a";
  x = { b = "b"; };
  x.c = "c";
  x = { d = "d"; };
}

There is no denying that the code works now without errors. Seems a bit strange, even without recursive attribute sets in the mix. I think I would consider it bad practice to write such code in most cases, as it might misguide the reader.

[sternenseemann]

@JojOatXGME Yes, this has been possible in 2.3 and probably long before. The relevant code hasn't changed for a long time, i.e. the parser.

[JojOatXGME]

I just tested my file against some old versions of Nix. Looks like this feature was introduced with Nix 2.3 and nixpkgs 19.09. Probably not important information, but anyway.

> nix-shell -p nix -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/release-19.03.tar.gz --run "nix-instantiate --version"
nix-instantiate (Nix) 2.2.2

> nix-shell -p nix -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/release-19.03.tar.gz --run "nix-instantiate --eval -A x i7111.nix"
warning: unknown setting 'sandbox-fallback'
error: attribute 'x' at /home/nixos/test/i7111.nix:3:3 already defined at /home/nixos/test/i7111.nix:2:3

> nix-shell -p nix -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/release-19.09.tar.gz --run "nix-instantiate --version"
nix-instantiate (Nix) 2.3.3

> nix-shell -p nix -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/release-19.09.tar.gz --run "nix-instantiate --eval -A x i7111.nix"
{ a = "a"; b = "b"; c = "c"; d = "d"; }