Path coercion in Flakes causes files to be added to the store twice

[infinisil]

Describe the bug

When coercing paths to strings (which adds said paths into the Nix store) in Flakes, all files are added to the store twice, leading to doubled space usage.

I understand the underlying mechanism and why this happens, but this sounds like a bug to me.

Steps To Reproduce

Create this flake.nix:

{
  outputs = { ... }: {
    result = "${./.}";
  };
}

Then run:

$ nix eval .#result
"/nix/store/jmnphp5bc4m32y80jw6ms16s8gb5g1gy-x2xsp09mx900wf1h7pglh9xlwy3r1349-source"
$ cat /nix/store/jmnphp5bc4m32y80jw6ms16s8gb5g1gy-x2xsp09mx900wf1h7pglh9xlwy3r1349-source/flake.nix
{
  outputs = { ... }: {
    result = "${./.}";
  };
}
$ cat /nix/store/x2xsp09mx900wf1h7pglh9xlwy3r1349-source/flake.nix
{
  outputs = { ... }: {
    result = "${./.}";
  };
}

Notice how the flake.nix file was copied to two store paths.

Expected behavior

Only a single store path is used.

nix-env --version output

nix-env (Nix) 2.18.1

Additional context

This issue is sponsored by Antithesis ✨

Priorities

Add 👍 to issues you find important.

[thufschmitt]

I suspect that it would be solved by #6530 when/if it lands (though that doesn't prevent it from being a bug).

A possible workaround is to use self rather than path interpolation

[tomberek]

had some random ideas... this happens often and is a common confusion due to the simple syntax and non-intuitive outcome. Note: most of these will change evaluation values for historical expressions, not backwards compatible.

• nix-instantiate --strict --eval "/nix/store/HASH-t/default.nix" (with a similar "./." construct in it) will have the same doubling take place - does that behavior need to be preserved? or is this a bug for both flakes/non-flakes?

• what should be the value of the nixpkgs "path" attribute in Nix? when forced to a string? https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/all-packages.nix#L83-L84 .

• Would we still want something like ${./some-dir/data-file} to be re-copied into the store into its own storepath even when ./some-dir or ./. has already been added? That is duplication as well. So we need to somehow distinguish these cases.

• possible to declare that all nix store contents do not get re-added, but become paths referring to existing content. eg ./t-2/hi becomes /nix/store/HASH-t2/hi, but this has the drawback of potentially storing much more content than necessary, eg: ./data/only-need-this-dir

• We can detect re-adding an existing store path and short-circuit the copyPath. (TODO: add some checks to ensure they really are the same contents modulo the directory name).

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index bf6b6f8c1..f7c37ae21 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -2345,6 +2345,9 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat
     auto dstPath = i != srcToStore.end()
         ? i->second
         : [&]() {
+            if (context.empty() && store->isValidPath(path.baseName())) {
+                return store->parseStorePath(path.path.abs());
+            }
             auto dstPath = path.fetchToStore(store, path.baseName(), FileIngestionMethod::Recursive, nullptr, repair);
             allowPath(dstPath);
             srcToStore.insert_or_assign(path, dstPath);

• This is not really flake related, but due to our path primitives taking into account the source directory's name and that the Nix store model allows for the same content to appear under different names. What is the name of the source directory in the example provided? Flakes give their source inputs the "-source" name by default to alleviate this problem, so at least that footgun has been improved.

• Similar bugs and confusions have been caused by this throughout Nixpkgs for which idioms and policy have been applied. The evaluation result of any path in non-flakes will have a non-deterministic output:
  nix-instantiate --eval --expr "./." has my home directory and username in it! Do we promise reproducibility for only string values, or path values as well?

• this is essentially the same test, two people running the same thing but in different directories get different results.

mkdir t1 t2
echo hi > t1/hi
echo hi > t2/hi
( cd t1 && nix-instantiate --eval --expr '"${./.}"' ;)
( cd t2 && nix-instantiate --eval --expr '"${./.}"' ;)

Context that others may be interested in:

• Basename of current directory goes into derivation for filterSource on ./. #1305
• builtins.path
• builtins.filterSource (see the note: https://nixos.org/manual/nix/stable/language/builtins.html?highlight=builtin#builtins-filterSource)

[edolstra]

While undesirable, this is not a bug, not something we can fix in a backward compatible way, and not flake-specific.

[infinisil]

Importing a local directory (that's not already in the store) twice when using ./. only happens with Flakes, so it is Flakes-specific. This cannot be reproduced with neither just the new experimental CLI nor the stable CLI..

And the point of merging features as experimental is that it allows experimentation without having to worry about backwards compatibility. Sure there's a lot of people using this now, but we can still deprecate current experimental features within years rather than never.

In particular, pure evaluation could be implemented in a way that doesn't need to import everything into the store first, which would fix this.

[infinisil]

So I guess it's not exclusively flake-specific, but flakes does cause this to trigger when you wouldn't expect it. But yes, this can also be fixed without involving flakes, fair enough.

[lf-]

I am working on hacks to work around this in nixpkgs, since it is required for NixOS/nixpkgs#254405

Related to the hacks I am doing, as a fallback: #5868