Show both attrPos and select pos in trace - #16249
Merged
Merged
Conversation
github-actions
Bot
added
the
with-tests
roberth
changed the title
roberth
commented
Jul 31, 2026
| into `(rec {..., body = ...}).body'. */ | ||
| | LET '{' binds '}' | ||
| { $3->recursive = true; $3->pos = CUR_POS; $$ = state->exprs.add<ExprSelect>(state->exprs.alloc, noPos, $3, state->s.body); } | ||
| { $3->recursive = true; $3->pos = CUR_POS; $$ = state->exprs.add<ExprSelect>(state->exprs.alloc, CUR_POS, $3, state->s.body); } |
Member
Author
There was a problem hiding this comment.
I would gladly leave let/body alone, but this improvement keeps
the rest of the code tidy.
roberth
marked this pull request as draft
July 31, 2026 13:36
roberth
force-pushed
the
feat-attr-select-two-pos
branch
from
July 31, 2026 16:32
d7ca78f to
594d5c9
Compare
roberth
marked this pull request as ready for review
July 31, 2026 16:32
xokdvium
reviewed
Jul 31, 2026
roberth
force-pushed
the
feat-attr-select-two-pos
branch
from
August 2, 2026 13:24
594d5c9 to
752ee13
Compare
Member
Author
|
(trivial rebase) |
Exercises `ExprSelect`'s source-distinguishing error trace on a String source, making output changes observable, avoiding error message regressions. Assisted-By: Claude Opus 4.8
I would gladly leave let/body alone, but this improvement keeps the rest of the code tidy. *Foreshadowing* - this has no observable effect, because it's the attribute's definition that tends to get printed. Assisted-By: Claude Opus 4.8
Without this, you may have no way to know what's responsible for accessing the wrong attr. A `span` right in `ExprSelect` would be nice, but GC makes me a bit wary of actually doing that, even though it is almost certainly fine. Assisted-By: Claude Opus 4.8
roberth
force-pushed
the
feat-attr-select-two-pos
branch
from
August 5, 2026 22:15
752ee13 to
8c4d12e
Compare
Member
Author
xokdvium
approved these changes
Aug 5, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Motivation
tl;dr error context good.
1:
In a lazy language, any value may be fallible.
Fallible values are ok, as long as you don't eval them.
So in practice this is an acceptable pattern.
2:
Predicting whether a value fails requires more evaluation, making the expression less lazy.
For instance
foo' = removeAttrs foo (f foo)can "sanitize" your foo, but makesfoo'strict inf foo, which can be a problem.So aforementioned pattern is desirable.
Given 1 and 2, not just the origin of a failure matters, but also where and how it is used.
Without this change, you may have no way to know who's responsible for accessing the failing attr.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.