deduplicate inherit-from source expr work #9847
Conversation
github-actions
bot
added
the
with-tests
There was a problem hiding this comment.
Will review again as this
- depends on Refactor the parser somewhat #9776
|
🎉 All dependencies have been resolved ! |
|
@pennae would you mind to rebase this? |
|
rebased onto current master. also ran some perf tests, normal operation doesn't notice the change but |
|
@roberth when can we expect a review? this is kind of blocking parser work now (one dependent PR is up, another two in preparation). |
in place of inherited() — not quite useful yet since we don't distinguish plain and inheritFrom attr kinds so far.
this also has the effect of sorting let bindings lexicographically rather than by symbol creation order as was previously done, giving a better canonicalization in the process.
for plain inherits this is really just a stylistic choice, but for inherit-from it actually fixes an exponential size increase problem during expr printing (as may happen during assertion failure reporting, on during duplicate attr detection in the parser)
|
rebased to fix merge conflicts. |
|
I think @roberth already knows the answer, but I am surprised this fix seems more involved than I'd imagine { inherit (expr) a b c; }to let e = expr; in { inherit (e) a b c; }would be |
|
that's basically what this fix is, except that we also collapse the generated envs (for performance reasons) and don't enforce references to the synthetic env on non-inheriting thunks (as are common in nixpkgs where inherit-from is used). lifting the synthetic env from sibling to parent of the entire set would keep it alive much longer than necessary in many cases, and introduce complications with rec sets and lets (which would either need two parent envs or inaccessible elements in their single parent env). the simplest implementation would've been to create on env per directive, but that would've also been slower (and attrset construction is already not fast because it's overloaded so heavily) |
|
Thanks @pennae. That makes sense. Basically, I am not used to thinking about these env objects. They feel like something that ought to not exist in a lexically scoped language with just a little bit of work, but maybe I am just being too compiler-brained rather than interpreter-brained about it. |
|
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 |
|
uhm. hello? is this still on anyone's radar or already slipped? |
|
It still is.
…-------- Original Message --------
On 20 Feb 2024, 13:29, pennae wrote:
uhm. hello? is this still on anyone's radar or already slipped?
—
Reply to this email directly, [view it on GitHub](#9847 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AADZGP5QGUIVTOBDIJZSMZ3YUSJKDAVCNFSM6AAAAABCJXNI4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJUGEYTGNRWHE).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
then may we humbly request that you either make good on your claim that reviewing a fix for #9149 won't be a problem, or acknowledge that you don't have the capacity to do so and let someone else take over? |
| synopsis: "`inherit (x) ...` evalutates `x` only once" | ||
| prs: 9847 | ||
| --- | ||
|
|
||
| `inherit (x) a b ...` now evalutates the expression `x` only once for all inherited attributes rather than once for each inherited attribute. |
There was a problem hiding this comment.
| synopsis: "`inherit (x) ...` evalutates `x` only once" | |
| prs: 9847 | |
| --- | |
| `inherit (x) a b ...` now evalutates the expression `x` only once for all inherited attributes rather than once for each inherited attribute. | |
| synopsis: "`inherit (x) ...` evaluates `x` only once" | |
| prs: 9847 | |
| --- | |
| `inherit (x) a b ...` now evaluates the expression `x` only once for all inherited attributes rather than once for each inherited attribute. |
desugaring inherit-from to syntactic duplication of the source expr also duplicates side effects of the source expr (such as trace calls) and expensive computations (such as derivationStrict).
it's no longer widely used and has a rather confusing meaning now that inherit-from is handled very differently.
Co-authored-by: Robert Hensing <[email protected]>
|
Thank you @pennae |
Motivation
fix #9149. also fix exponential increase in error message size with nested inherit-from expressions. (cc @roberth)
Context
we now allocate an extra env for attrsets containing inherit-from directives for all the from expressions, as a sibling the in-set scope env. this lets us avoid keeping the entire scope around when inherited attributes are picked out, and it makes access to memoized values easier. considering how rare inherit-from are in practice it didn't seem worth the more complicated code needed to allocate one env for each directive.
fun fact:
rec { inherit (x) yevaluatesxin the inner scope wilerec { inherit yevaluatesyin the outer scope. this is inconsistent and feels like an implementation bug that is now become spec by virtue of having been there for way too long.depends on #9776. seemed more reasonable to not rebase this onto master and have to resolve conflicts after either gets merged.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.