Skip to content

New CLI flag --replace-eval-errors #13095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mightyiam
Copy link

Supersedes #12705

Co-authored-by: Farid Zakaria [email protected]

Motivation

Following up from #12705 but provides for the use case with a new CLI flag.

Context

Ditto.


Add 👍 to pull requests you find important.

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

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Apr 25, 2025
Comment on lines 59 to 60
If set to `true`, the Nix evaluator will replace evaluation errors
with a fixed value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should mention "in JSON output".

@@ -53,6 +53,13 @@ struct EvalSettings : Config

std::vector<PrimOp> extraPrimOps;

Setting<bool> replaceEvalErrors{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a setting but a flag to nix eval. We should avoid global settings that change what the output of commands looks like (e.g. nix eval --json), because then a user's settings might change the semantics of scripts that use those commands.

bool isFileNotFoundError = dynamic_cast<FileNotFound *>(&e);
// Restrict replaceEvalErrors only only evaluation errors
if (state.settings.replaceEvalErrors && (isEvalError || isFileNotFoundError)) {
out.emplace(state.symbols[a->name], "«evaluation error»");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the semantics of builtins.toJSON and derivation with structured attrs (since it calls printValueAsJSON()), making them ignore exceptions which seems like a bad thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ties in with the comment about making it a flag instead of a setting.
In line with that, this could then refer to a new C++ parameter on printValueAsJSON, instead of state.settings.

@@ -48,6 +48,13 @@ R""(
# cat ./out/subdir/bla
123

* Replace evaluation errors:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Replace evaluation errors" is very vague (though the example does make it clear).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is clear when you have in mind the model of evaluation as term rewriting, although that's not how we introduce evaluation at all. In fact, we don't have a description of evaluation in the manual.

Reminded me of

although that doesn't go all that much into term rewriting either.

Comment on lines +117 to +118
When used with `--eval` and `--json`, replace any evaluation errors with the string
`"«evaluation error»"`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really what it does. It should say something like: any attribute that throws an evaluation error is represented by a JSON property with the string value «evaluation error».


```console
$ nix-instantiate --eval --json --replace-eval-errors --expr '{ a = throw "fail"; }'
{"a":"«evaluation error»"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this representation of eval errors is ambiguous, since

nix-instantiate --eval --json --expr '{ a = "«evaluation error»"; }'
{"a":"«evaluation error»"}

produces the same output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is true, we already have such ambiguity, for instance:

nix-instantiate --eval --json --expr --strict '{ foo.outPath = "bar"; }'
{"foo":"bar"}

That said, something like {"__nix_evaluation_error": null} would be subjectively less ambiguous, and allows for a message or other data to be put where it says null.

Considering that this originated from a tryEval improvement, we don't have to be so afraid of the "chaos" in the message anymore!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/comparing-module-system-configurations/59654/14

@fzakaria
Copy link
Contributor

fzakaria commented May 6, 2025

With this change (or a variant of it), I tried running it on a nixosConfiguration and the evaluator OOM :(

@mightyiam
Copy link
Author

With this change (or a variant of it), I tried running it on a nixosConfiguration and the evaluator OOM :(

I've been there. Gotta take care to avoid infinite recursion.

@fzakaria
Copy link
Contributor

fzakaria commented May 8, 2025

I wonder if this something max-call-depth can help us with.
(CC @roberth)

Since now we have a way to deterministically replace "errors" with a fixed string; we can set an upper-bound and fail when we breach it.

Of course, this might not then solve the issue we sought to fix which was "a way to determine if someone sneaked in some nefarious NixOS options waiting to be enabled".

@mightyiam
Copy link
Author

Some infinite recursion are currently detectable by Nix. They're the kind that is trivial to detect (evaluating thunk x during evaluation of thunk x). Those are referred to as "black holes".

$ nix eval --json --expr 'let a = a; in a'
error: infinite recursion encountered
       at «string»:1:9:
            1| let a = a; in a
             |

But there's another kind, and that is the infinitely growing data structure.

$ nix eval --json --expr 'let a = {} // { inherit a;}; in a'
error: stack overflow (possible infinite recursion)

It is the latter kind that you are likely stumbled across. And I think there are not many of those. During Ocean Sprint 2025 the last of them may have been removed:

NixOS/nixpkgs#390717

So, I think we don't need to support those.

@mightyiam mightyiam force-pushed the nix-flag-replace-eval-errors branch from ddbc00b to 7925634 Compare May 12, 2025 11:44
@mightyiam mightyiam force-pushed the nix-flag-replace-eval-errors branch from 7925634 to 88705f8 Compare May 12, 2025 12:03
@fzakaria
Copy link
Contributor

fzakaria commented May 19, 2025

Interesting these have different outcomes when --json is presented and not.

nix eval --expr 'let a = {} // { inherit a;}; in a'
{ a = «repeated»; }

nix eval --json --expr 'let a = {} // { inherit a;}; in a'
error: stack overflow (possible infinite recursion)

We are building a demo file with some potential evaluation errors.

{
  missingAttr = let bar = { }; in bar.notExist;
  insideAList = [ (throw "a throw") ];
  deeper = { v = throw "v"; };
  failedAssertion = assert true; assert false; null;
  missingFile = builtins.readFile ./missing-file.txt;
  missingImport = import ./missing-import.nix;
  outOfBounds = builtins.elemAt [ 1 2 3 ] 100;
  failedCoersion = "${1}";
  failedAddition = 1.0 + "a string";
  unicodeError = throw "» injecttion: error: v»";
  repeatedRecursion = { a = let b = {inherit b;}; in b;};
}

Fails (obviously) pretty bad when --json is given but looks to be fine without it
(I added newlines)

nix eval -f tests/functional/replace-eval-errors.nix
{
deeper = { v = «error: v»; };
failedAddition = «error: cannot add a string to a float»;
failedAssertion = «error: assertion 'false' failed»;
failedCoersion = «error: cannot coerce an integer to a string: 1»;
insideAList = [ «error: a throw» ];
missingAttr = «error: attribute 'notExist' missing»;
missingFile = «error: opening file '/home/fmzakari/code/github.com/NixOS/nix/tests/functional/missing-file.txt': No such file or directory»;
missingImport = «error: path '/home/fmzakari/code/github.com/NixOS/nix/tests/functional/missing-import.nix' does not exist»;
outOfBounds = «error: 'builtins.elemAt' called with index 100 on a list of size 3»;
repeatedRecursion = { a = { b = «repeated»; }; };
unicodeError = «error: » injecttion: error: v»»;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants