Fix builtins.tryEval and error catching #1098
Labels
No labels
Affects/CppNix
Affects/Nightly
Affects/Only nightly
Affects/Stable
Area/build-packaging
Area/cli
Area/evaluator
Area/fetching
Area/flakes
Area/language
Area/lix ci
Area/nix-eval-jobs
Area/profiles
Area/protocol
Area/releng
Area/remote-builds
Area/repl
Area/repl/debugger
Area/store
awaiting
author
awaiting
contributors
bug
Context
contributors
Context
drive-by
Context
maintainers
Context
RFD
crash 💥
Cross Compilation
devx
docs
Downstream Dependents
E/easy
E/hard
E/help wanted
E/reproducible
E/requires rearchitecture
Feature/S3
imported
Language/Bash
Language/C++
Language/NixLang
Language/Python
Language/Rust
Needs Langver
OS/Linux
OS/macOS
performance
regression
release-blocker
stability
Status
blocked
Status
invalid
Status
postponed
Status
wontfix
testing
testing/flakey
Topic/Large Scale Installations
ux
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
lix-project/lix#1098
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The introduction of
builtins.tryEvalin its current form has made a lot of people very angry and been widely regarded as a bad move. The most prominent issues are:throws andasserts, and not any other runtime evaluation errors. Notably, it cannot catch type errors, missing attributes or nulls. This means that it is generally fit for the use case of "try to wrap this code which may not evaluate and recover gracefully".andir/npins@3329242072/src/default.nix (L232-L238)[ my list ] ++ "zero" ++ (throw "error")the type checking of the first two arguments must be eager before the evaluation of the last argument, because changing it to be more lazy would turn the type error (not catchable) into a throw error (catchable)Results over exceptions for error handling) or bytecode/JIT optimization stages (implementing an exception handling runtime within a VM is decidedly not fun, nor performant)Given the current limitations, no easy improvements can be made. Here are some options:
tryEval. Unclear how to deal with that taint though and also this sounds prohibitively expensivetryEval2(and, unrelatedly, a bettertoString2) that is better at catching all meaningful errors. Might require langver, and does not mitigate the implementation-side issuestryEvalaltogether, possibly in favor of some top-level CLI error handling facilities (e.g. for the Nixpkgs test suite which currently usestryEval)tryEvaltounsafeTryEvaland drop all purity and stability guarantees on the output of that function. This allows the Nixpkgs tests still to work while allowing us to evolve the evaluator without having to care too much. Other usage in Nixpkgs, especially in derivations, would be forbidden.tryEvalfrom pure evluation contextsThe last solution is the one I currently prefer most and would like to implement going forwards. Some of the solutions are not mutually exclusive, and interim solutions that will be complemented by more proper solutions at a later point in time are a valid option.
Reminds us a little of the "poison context" idea that was floated around a while ago. The most common case for
throwing in Nixpkgs is to prevent a derivation from being instantiated without e.g.config.allowUnfree. That doesn't over cases likesomeHandler.${system} or (throw "unsupported system ${system}"), though.we're also in favor of deprecate-renaming tryEval as a first step. long-term we should either remove it entirely (and add result-returning variants to all builtins that cannot have pre-checks, like readFile in the linked example), or make it catch all eval errors unsafely including type errors and parse errors (which aren't currently eval errors at all). in fact we should add result-returning variants for all builtins that are fallible due to side effects regardless of what we do about tryEval.
@qyriad wrote in #1098 (comment):
poison context underapproximates this due to context discarding and
sequencing. we'd argue that the nixpkgs checks would be better approximated with an assertion that can deliver a custom message if the goal is to prevent instantiation. assertions having printed a facsimile of their predicate AST in the past does suggest something like this was the goal at some point, but we can't be sure whether it was this or that there were no source annotations for errors when assert was introduced :/