This can be removed when NixOS/nixpkgs#274077 is merged, just a hack to get tests passing.

The lib test suite has a few test cases that check the output of nix-instantiate --eval-only. I don't think that output is guaranteed by any compatibility policy, so I think we're good to change it, but we do need to patch the tests. Once we have the output finalized here, we can merge this PR in nixpkgs and then update the Flake input reference and remove this patch.

For some reason updating the nixpkgs input makes nix flake check want to rebuild LLVM and GHC? So that's hitting the 1hr CI timeout, but nix flake check does pass locally.

I don't think that output is guaranteed by any compatibility policy

I would feel the same, but in Nix that thinking tends to lead to problems for users.

We're currently already interacting with Nixpkgs a bit, so that makes merging this line a bit hard.
To get this merged, I'd prefer to keep the "ascii" syntax around, perhaps through a bool flag?

rebuild LLVM and GHC

I would expect this to be resolved now, but I'm surprised that this happened. We do care about avoiding this.

I'm beginning to think the test suite should just use the JSON output instead. Also an easy change to make: NixOS/nixpkgs#275264

Any machine parsing like the test suite should definitely use the JSON output. If the output of human-interface CLI tools shouldn't be changed to improve readability then that defeats the purpose of having CLI tools

And more broadly, if we want to be able to actually improve the experience of using Nix, we have to be comfortable saying "no, that's not part of the public interface, you'll need to change your code because we haven't made a guarantee to keep this stable and writing a shell script that depends on it doesn't change that".

It sounds like there is need for a stable output layer that can encode (e.g.) thunks and functions, so maybe we should add a second JSON output layer that has an actual public schema that we can stick to. EDIT: It sounds like this is the purpose of the XML output layer that already exists! We should leverage that.

We'd just add another layer to disambiguate types, so we'd encode a thunk as {"type": "thunk"} (possibly with auxiliary information like where it comes from, if it's a cycle, etc.), and we could encode an attrset as {"type": "attrset", "values": {...}}. This would be machine-readable (without writing a Nix parser!) and easy to keep stable.

But in the meantime there don't exist any use-cases that can't be easily patched by using the JSON output and (for checking laziness) checking the error message for "cannot convert thunk to JSON".

Marking this resolved as we got NixOS/nixpkgs#275264 merged and I updated the Nixpkgs input, so we're all good to release now.

If I remember correctly: This is annoying for the Tvix people (cc @sternenseemann) because they reuse the Nix language test suite as a compatibility check, and that test suite uses the result of nix-instantiate --eval.

I don't think that's really right (as you said, it's not great to rely on that output), but since it is the de facto canonical language test, there's not a lot of leeway.

All that to say, changing the pretty printer would probably require changing the test suite first to not rely on the pretty printing (but use the XML or JSON format) first.

It seems that the purpose of this header is to resolve a cyclic dependency between value.hh and print.hh.

If you were to use a reference type in value.hh, you could use a forward declaration struct PrintOptions; in value.hh instead. That way we can keep the actual declaration close to its use and avoid the need for this header file.

If I do that, I have to remove the default options value:

struct Value {
    void print(EvalState &state, std::ostream &str, PrintOptions & options);
};

Then I have to patch the callsites and I get errors like this:

src/nix-instantiate/nix-instantiate.cc:59:46: error: taking the address of a temporary object of type 'nix::PrintOptions' [-Waddress-of-temporary]
                vRes.print(state, std::cout, &PrintOptions{});

I can fix it like this, but it feels a lot less "clean":

auto options = PrintOptions {};
vRes.print(state, std::cout, options);

I think our header files are way too cluttered anyways, and they would be a lot easier to work with if each type had a separate header.

y no longer exists in the example printouts.

Fixed in #9755.

Arguably they have the same type at this time, tThunk, maybe “despite not being equal” would be better? In any case, this is sort of the point of not forcing the value completely, nix eval will also print the same output for non-equal values/values where the type isn't the same.

Wouldn't be clearer to just say that the problem with the output is that it produces something that can be parsed as a Nix expression, but would not return the same result?

I agree and actually it is ambiguous what the ambiguity is. It can be either

• intentional ambiguity because --strict wasn't specified,
• or ambiguity in the mind of the reader, who might "naively" (can't blame) read it is an expression instead of a mere value, while non-value expressions are not printed by this command.

Fixed in #9755.