assert: Report why values aren't equal
#11043
Conversation
roberth
added
error-messages
github-actions
bot
added
the
with-tests
|
Looking at the AST seems pretty hacky and error-prone to me. It would be cleaner to add an |
Evaluation is just rewriting. Rewrite rule heads matching AST constructors is nice, but not a requirement. I don't see why looking at the AST would be error prone. I don't like I might agree with you in cases where the language semantics is affected, but in this case it is only the error reporting. That's far from critical. |
src/libexpr/eval.cc
Outdated
| // eqValues has found a difference, and it should match | ||
| // its behavior. | ||
| error<EvalBaseError>( | ||
| "cannot compare %1% with %2%; is assertEqValues out of sync with eqValues?", showType(v1), showType(v2)) |
There was a problem hiding this comment.
Maybe it should say something like BUG!. To someone not reading the c++ code ( most of the nix users) it's not clear what "eqValues" and "assertEqValues" mean.
There was a problem hiding this comment.
Changed to:
"BUG: cannot compare %1% with %2%; did forceValue leave a thunk, or might assertEqValues be out of sync with eqValues?"
| } | ||
|
|
||
| switch (v1.type()) { | ||
| case nInt: |
There was a problem hiding this comment.
Is there a way to do enforce an exhaustive c++ switch case that will fail to compile when there cases missing? Rust has this.
There was a problem hiding this comment.
Good question, but we already enforce this with
'-Werror=switch',
'-Werror=switch-enum',This makes the error message very hypothetical. We'd have to undo those flags, or forceValue needs to silently fail, or we'd have to read garbage, or some other chaotic thing we don't need to consider.
There was a problem hiding this comment.
Could a default case shadow a potential error message and should be therefore removed?
There was a problem hiding this comment.
The flags already prevent this despite the default, so the default is a bit paranoid.
I've changed the exceptions to crashes to aid debugging, and I've documented that they're unreachable.
An nicer alternative to printError + abort, or assert(false /* foo */)
It's still paranoid, and probably a waste of words, but at least now it's consistent and readily identifyable from a log.
| logError(error.info()); | ||
| printError("This is a bug! An unexpected condition occurred, causing the Nix evaluator to have to stop. If you could share a reproducible example or a core dump, please open an issue at https://github.com/NixOS/nix/issues"); | ||
| abort(); |
There was a problem hiding this comment.
A generalization of this in libutil would be nice for a follow-up.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-07-22-nix-team-meeting-minutes-163/49544/1 |
Motivation
assert a == b; xshould report whyaisn't equal tob.With this change it does so rather well, I would say, especially when
aandbare nested structures. See for exampleeval-fail-assert-nested-bool.err.exp.Context
This adds code that duplicates existing behavior. I've done so to keep normal equality fast and simple.
While this approach does add a risk of the code paths diverging in the future, I believe this risk is well worth the benefit.
It makes the difference between knowing immediately why something fails, or a debugging session, changing code in a dependency, etc.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.