Input::fetchToStore(): Don't try to substitute

[edolstra]

Motivation

Having a narHash doesn't mean that we have the other attributes returned by the fetcher (such as lastModified or rev). For instance,

$ nix flake metadata github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f
Last modified: 2024-01-15 10:51:22

but

$ nix flake metadata github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f?narHash=sha256-PPXqKY2hJng4DBVE0I4xshv/vGLUskL7jl53roB8UdU%3D
(does not print a "Last modified")

The latter only happens if the store path already exists or is substitutable, which made this impure behaviour unpredictable.

Fixes #10601. Note: lazy-trees already had this fix, because it no longer requires all locked nodes to have a narHash.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[roberth]

(thoughts)

I'd still be concerned about similar bugs, because the code isn't structured in a way that would make such a bug somewhat harder to write.
We have the requirement that

• Given any input to fetchTree, it must only succeed with one possible return value - ie referential transparency in the happy path.

This crucial property is hard to test, so we should try to structure the code in a way that makes this property easy to determine (I won't say prove; too absolute to be realistic).
A mutable map makes that hard.

In my previous attempt to improve this, I focused on giving types to the input side, which wasn't all that fruitful, given the amount of change and effort required. Maybe it'd be more effective to put the output into a struct before returning it as attributes?

[roberth]

Is it necessary not to substitute? What if the the missing fields can be obtained from the lock file instead?
I suppose we'd have to tell fetchTree not to return any new attributes except outPath. IIRC we don't have such a flag yet?

[edolstra]

Substitution could still be done for locked flake inputs, because there we know that we have all the attributes.

We do have the isLocked() method, but we can't use it for the tarball fetcher, since the HTTP headers can return additional lock attributes like lastModified and rev.

[roberth]

but we can't use it for the tarball fetcher, since the HTTP headers can return additional lock attributes like lastModified and rev.

Aren't those saved to the lock then?

[edolstra]

They are, this problem only occurs on the CLI if you pass narHash but not the other attributes.

[edolstra]

I guess part of the issue is the removal of hasAllInfo(), though even before that the tarball fetcher had this issue (since its hasAllInfo() just returned true).

Regardless, correctness is more important than performance, and the substitution shortcut makes it harder to reason about the correctness of fetchers, so it's probably best to get rid of it for now.

[roberth]

Fair enough. Some optimizations can be done later, unless we move towards a non-optimizable design. I don't think that's the case here.

EDIT: Some optimizations