Miscellaneous improvements for positioning in eval-errors #4440
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It would be great to see before/after example! |
|
You're absolutely right, sorry for that!
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
Looks good to me. The only concern is adding a |
|
@edolstra yeah, I'm afraid so. As I'm not as experienced with the codebase as you are, may I ask if you have an alternative idea with a smaller memory footprint? Otherwise I'd probably do some benchmarking with a few more complex expressions to see the actual difference. As soon as we've resolved that (and I have sufficient time ^^), I'd continue on that PR here :) |
|
@edolstra so I did a few brief tests now by comparing Nix master (f15f0b8) vs my branch (locally rebased against f15f0b8). For comparison, I decided to use a few NixOS tests:
Nix from my branch: As you can see the difference is ~20MB peak memory consumption.
Nix from my branch: It's kinda interesting that my branch actually consumes less memory than Nix master (I tried this three times in a row with an equal result). @edolstra may have an idea why this is happening. I assume that the increase is a bit too much to use this by default (feel free to disagree though). I'm wondering now how we should proceed here: do you have an idea if we can reduce the memory consumption there somehow? Or by making this behavior optional? |
|
cc @edolstra given my latest comment, how shall we proceed here? :) |
|
Rebased onto latest master. cc @edolstra given my latest comments, how shall we proceed here? :) |
|
Peak memory consumption is a bit tricky to measure since it depends on whether/when GC kicks in. A more reproducible metric is to run with |
|
@edolstra @domenkozar so I just evaluated the First of all, let me share the raw results here:
So we basically have |
edolstra
approved these changes
Apr 13, 2021
* The position of the `name`-attribute appears in the trace. * If e.g. `meta` has no `outPath`-attribute, a `cannot coerce set to string` error will be thrown where `pos` points to `name =` which is highly misleading.
When working on some more complex Nix code, there are sometimes rather unhelpful or misleading error messages, especially if coerce-errors are thrown. This patch is a first steps towards improving that. I'm happy to file more changes after that, but I'd like to gather some feedback first. To summarize, this patch does the following things: * Attrsets (a.k.a. `Bindings` in `libexpr`) now have a `Pos`. This is helpful e.g. to identify which attribute-set in `listToAttrs` is invalid. * The `Value`-struct has a new method named `determinePos` which tries to guess the position of a value and falls back to a default if that's not possible. This can be used to provide better messages if a coercion fails. * The new `determinePos`-API is used by `builtins.concatMap` now. With that change, Nix shows the exact position in the error where a wrong value was returned by the lambda. To make sure it's still obvious that `concatMap` is the problem, another stack-frame was added. * The changes described above can be added to every other `primop`, but first I'd like to get some feedback about the overall approach.
This now takes care of providing positioning for both the faulting value and the faulting function call in case of an error.
…'s pos if attr-set pos == noPos
Ma27
changed the title
|
@edolstra thanks for your review! I applied your suggestions and rebased onto latest master, so this should be good to go. A few ideas for this would be to
What do you think of this? I'm happy to work on this somewhere in the future (can't promise that I'll get to it though) if you'd consider this a reasonable thing to do :) |
|
@edolstra do you think we can merge this? Also, what do you think of the idea for further changes regarding error messages? :) |
|
@Ma27 Thanks! Yeah, more position info is generally a good thing if it doesn't take too much memory. |
|
Yup :) Will do some further experiments when I have the time and check the new memory footprint with NIX_SHOW_STATS :) |
Ma27
referenced
this pull request
in NixOS/nixpkgs
Jul 18, 2021
When inlining a module with a problematic declaration, you usually get
get a not-so helpful error like this:
$ cat flake.nix
{
description = "A very basic flake";
inputs.nixpkgs.url = path:../.;
outputs = { self, nixpkgs }: {
nixosConfigurations.foo = nixpkgs.lib.nixosSystem {
system = "x86_64-linux";
modules = [
({ lib, ... }: { services.wrong = 2; })
{ services.nginx.enable = true; }
];
};
};
}
$ nixos-rebuild build --flake .#foo -L
error: The option `services.wrong' does not exist. Definition values:
- In `<unknown-file>': 2
While it's certainly possible to guess where this comes from, this is
IMHO fairly confusing for beginners (and kinda reminds me of the
infamous "infinite recursion at undefined position"-error).
The module-system determines the position of a declaration using the
`_file`-key: this is either `toString path` if `path` is e.g. a value
from `imports = [ ./foo.nix ]` or the file used as `NIXOS_CONFIG` in
`<nixpkgs/nixos>`.
However such a mechanism doesn't exist (yet) for inlined flake modules,
so I tried to implement this in a fairly basic way:
* For non-path declarations, the position of `modules` inside the
`flake.nix` which declares these modules is determined by doing
`unsafeGetAttrPos` on the `modules`-argument of `lib.nixosSystem`.
So the `flake.nix` from above would now raise the following
error-message:
$ nixos-rebuild build --flake .#foo -L
error: The option `services.wrong' does not exist. Definition values:
- In `/nix/store/4vi3nhqjyma73ygs4f93q38qjkhkaxw8-source/flake.nix': 2
Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
Co-authored-by: Silvan Mosberger <github@infinisil.com>
Co-authored-by: Robert Hensing <robert@roberthensing.nl>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is still a work-in-progress PR to improve the position of errors showed by Nix. I'm happy to do more work on this to get this actually mergable, but first I'd like to gather some feedback (cc @domenkozar @edolstra @bburdette @Ericson2314 ).
To summarize, the following changes are on this branch:
primops/derivation: use position of currently evaluated attribute
name-attribute appears in the trace.metahas nooutPath-attribute, acannot coerce set to stringerror will be thrown wherepospoints toname =which ishighly misleading.
libexpr: misc improvements for proper error position
When working on some more complex Nix code, there are sometimes rather
unhelpful or misleading error messages, especially if coerce-errors are
thrown.
This patch is a first steps towards improving that. I'm happy to file
more changes after that, but I'd like to gather some feedback first.
To summarize, this patch does the following things:
Attrsets (a.k.a.
Bindingsinlibexpr) now have aPos. This ishelpful e.g. to identify which attribute-set in
listToAttrsisinvalid.
The
Value-struct has a new method nameddeterminePoswhich triesto guess the position of a value and falls back to a default if that's
not possible.
This can be used to provide better messages if a coercion fails.
The new
determinePos-API is used bybuiltins.concatMapnow. Withthat change, Nix shows the exact position in the error where a wrong
value was returned by the lambda.
To make sure it's still obvious that
concatMapis the problem,another stack-frame was added.
The changes described above can be added to every other
primop, butfirst I'd like to get some feedback about the overall approach.