deduplicate inherit-from source expr work

[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) y evaluates x in the inner scope wile rec { inherit y evaluates y in 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 :+1: to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[dpulls[bot]]

:tada: All dependencies have been resolved !

[roberth]

@pennae would you mind to rebase this?

[pennae]

rebased onto current master. also ran some perf tests, normal operation doesn't notice the change but nix-env -qaP --out-path sees about 5% runtime improvement.

[pennae]

@roberth when can we expect a review? this is kind of blocking parser work now (one dependent PR is up, another two in preparation).

[pennae]

rebased to fix merge conflicts.

[Ericson2314]

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

[pennae]

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)

[Ericson2314]

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.

[thufschmitt]

Discussed during the Nix maintainers meeting on 2024-02-12. Assigned to @roberth.

Fixing a very weird behaviour on the evaluator (not memoizing the subexpression of inherit).

Agreement that we want it, @roberth to review it

[nixos-discourse]

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

[pennae]

uhm. hello? is this still on anyone's radar or already slipped?

[roberth]

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, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[pennae]

It still is.

then may we humbly request that you either make good on your claim that reviewing a fix for https://github.com/NixOS/nix/issues/9149 won't be a problem, or acknowledge that you don't have the capacity to do so and let someone else take over?

[roberth]

Thank you @pennae