improve error reports somewhat #9874
Conversation
github-actions
bot
added
the
with-tests
|
also cc @9999years? |
| @@ -0,0 +1 @@ | |||
| [ 1 2 { __overrides = { y = { d = [ ]; }; }; c = [ ]; d = 4; x = { c = [ ]; }; y = «repeated»; } ] | |||
There was a problem hiding this comment.
y = «repeated»;
Mojibake?
There was a problem hiding this comment.
I'm not sure what's going on with the Unicode rendering here. Seems to be messed up but only in the Files tab. I checked out the PR locally and the encoding is correct there. It looks like the files are being decoded as Latin-1 for some reason:
$ python3
>>> "«repeated»".encode("utf-8").decode("latin-1")
'«repeated»'
Maybe the literal \r in tests/functional/lang/eval-fail-eol-2.nix is tripping GitHub up?
| @@ -1,5 +1,6 @@ | |||
| error: syntax error, unexpected ':', expecting '}' | |||
There was a problem hiding this comment.
Again not a blocker for this PR, but it would be nice if this said expecting ',', '?', or '}'.
There was a problem hiding this comment.
not with bison. we're working on replacing the parser completely with a pegtl-based variant, once that's done this should be a lot more possible.
src/libexpr/nixexpr.cc
Outdated
| if (p.id == 0) | ||
| return std::monostate{}; | ||
|
|
||
| // see above for justification. |
There was a problem hiding this comment.
| // see above for justification. | |
| // see `PosTable::operator[]` for justification. |
New methods could be added, making "above" ambiguous.
|
added a few changelog entries for the more commonly impactful things. line endings probably won't be interesting to most anywho :) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-02-12-nix-team-meeting-minutes-123/39775/1 |
This comment was marked as resolved.
This comment was marked as resolved.
|
🎉 All dependencies have been resolved ! |
|
rebased to master since #9847 had grown some more typo fixes. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-03-04-nix-team-meeting-minute-130/40830/1 |
Although if @tomberek or @9999years can confirm they reviewed it, that's good for me |
the parser modifies its inputs, which means that sharing them between the error context reporting system and the parser itself can confuse the reporting system. usually this led to early truncation of error context reports which, while not dangerous, can be quite confusing.
we already normalize attr order to lexicographic, doing the same for formals makes sense. doubly so because the order of formals would otherwise depend on the context of the expression, which is not quite as useful as one might expect.
previously we reported the error at the beginning of the binding block (for plain inherits) or the beginning of the attr list (for inherit-from), effectively hiding where exactly the error happened. this also carries over to runtime positions of attributes in sets as reported by unsafeGetAttrPos. we're not worried about this changing observable eval behavior because it *is* marked unsafe, and the new behavior is much more useful.
the parser treats a plain \r as a newline, error reports do not. this can lead to interesting divergences if anything makes use of this feature, with error reports pointing to wrong locations in the input (or even outside the input altogether).
this needs a string comparison because there seems to be no other way to get that information out of bison. usually the location info is going to be correct (pointing at a bad token), but since EOF isn't a token as such it'll be wrong in that this case. this hasn't shown up much so far because a single line ending *is* a token, so any file formatted in the usual manner (ie, ending in a line ending) would have its EOF position reported correctly.
we now keep not a table of all positions, but a table of all origins and their sizes. position indices are now direct pointers into the virtual concatenation of all parsed contents. this slightly reduces memory usage and time spent in the parser, at the cost of not being able to report positions if the total input size exceeds 4GiB. this limit is not unique to nix though, rustc and clang also limit their input to 4GiB (although at least clang refuses to process inputs that are larger, we will not). this new 4GiB limit probably will not cause any problems for quite a while, all of nixpkgs together is less than 100MiB in size and already needs over 700MiB of memory and multiple seconds just to parse. 4GiB worth of input will easily take multiple minutes and over 30GiB of memory without even evaluating anything. if problems *do* arise we can probably recover the old table-based system by adding some tracking to Pos::Origin (or increasing the size of PosIdx outright), but for time being this looks like more complexity than it's worth. since we now need to read the entire input again to determine the line/column of a position we'll make unsafeGetAttrPos slightly lazy: mostly the set it returns is only used to determine the file of origin of an attribute, not its exact location. the thunks do not add measurable runtime overhead. notably this change is necessary to allow changing the parser since apparently nothing supports nix's very idiosyncratic line ending choice of "anything goes", making it very hard to calculate line/column positions in the parser (while byte offsets are very easy).
The commit message for that is very helpful to understand what is happening. I wanted to ensure we are okay with the "virtual primop" mechanism. |
Motivation
error reports can still be improved somewhat, especially in the areas of "pointing to the right place" and "always printing things the same".
Context
changing the parser requires some changes to error reporting anyway. this is in preparation for a parser switch that will give locations differently than bison does.
depends on #9847
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.