Skip to content

builtins.catchEvalErrors #12705

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
Member

The motivation behind this is wanting to compare nixpkgs module system configurations.
By convention, configurations contain thunks that fail to evaluate with at least the following happening in a minimal NixOS configuration:

  • explicit throws
  • failed assertions
  • detectable infinite recursion ("black hole"?)
  • missing attribute

For the purpose of evaluating various NixOS derivations such as .config.system.build.toplevel those errors don't typically occur unless the configuration is "wrong" or unsupported.

But for trying to deeply evaluate a configuration, at least the above must be caught for this approach to succeed.
Of course, a different approach would be to modify the various modules to support deep evaluation, but that would be significantly more work, I suppose.

So we're asking for this builtin to make deep evaluation of module system configurations possible.
Of course, it may be found useful for other purposes.

We find it possibly unfortunate that ecosystem configurations cannot be deeply evaluated in the first place, but acknowledge that that kind of design is common is lazily evaluated data structures.

We think that this builtin does not compromise the integrity of the guarantees the language is bound by.

A related PR #5564 was rejected on the grounds that allowing observation of distinct error values would compromise purity.
Of course, in this PR we are not asking for distinct error values.
Contrary to #5564, this PR is backwards compatible.

From that PR:

... exception handing is not really compatible with a lazy functional language.

I suppose that the meaning of "handling" here has to do with distinct error values.

... tryEval was really only ever intended to allow Nixpkgs' release.nix to traverse pkgs, which requires being able to catch assertions.

We have a similar situation here, where we want to traverse a Nixpkgs-originating structure that happens to have some error types that are not caught by tryEval.

So if it starts catching stuff like type errors, it masks actual bugs. The solution to type errors is to fix them, not catch them.

I'm not sure whether this applies only to "it" in this sentence which is tryEval or it would apply also to catchEvalError.
So I will assume the former because I don't quite see a significant concern with catchEvalError myself.

In particular, aborts should not be caught. The whole point of abort is that it isn't caught.

Currently suggested implementation catches aborts as well. Is that specifically a no-no?

Co-authored-by: Alexander Foremny [email protected]

Motivation

Context


Add 👍 to pull requests you find important.

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

@Mic92 Mic92 added this to Nix team Mar 21, 2025
@github-project-automation github-project-automation bot moved this to To triage in Nix team Mar 21, 2025
Co-authored-by: Alexander Foremny <[email protected]>
@mightyiam mightyiam force-pushed the feature/primop-catch-eval-errors branch from 59e3862 to 9061f04 Compare March 21, 2025 13:21
- out of bounds indexing
- file not found such as `import` and `builtins.readFile`
- deserialization such as `builtins.fromJSON`
- detectable infinite recursion
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 never be caught since non-termination is undecidable. Our current blackhole detection is "best effort" since it was easy to implement, but for instance it doesn't currently work with the multi-threaded evaluator.

Similar to `builtins.tryEval` except that it catches
- `throw`
- `assert`
- `abort`
Copy link
Member

Choose a reason for hiding this comment

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

Allowing aborts to be caught is probably going to lead to a reallyAbort primop that isn't caught by catchEvalError...


And that `value` attribute exists only on success.
)",
.fun = prim_catchEvalError,
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 probably be gated behind an experimental feature or enabled by a CLI flag, since allowing a general catch-all sounds like it could lead to some really bad antipatterns (especially if Nixpkgs or the NixOS module system starts using it).

@mightyiam
Copy link
Member Author

We're going to pivot into making a new nix-instantiate and/or nix eval CLI flag, instead. --replace-eval-errors or similar.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2025-03-24-nix-team-meeting-minutes-221/62113/1

@piegamesde
Copy link
Member

Does this catch any runtime type errors?

@mightyiam
Copy link
Member Author

Does this catch any runtime type errors?

I think type errors fall under the category of evaluation errors, yes:

nix-repl> builtins.catchEvalError (toString {})
{ success = false; }

nix-repl> builtins.catchEvalError ({}.a)
{ success = false; }

@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/12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To triage
Development

Successfully merging this pull request may close these issues.

4 participants