Skip to content

lib: make extender available on self-references#210976

Merged
roberth merged 1 commit intoNixOS:masterfrom
clhodapp:fix/extensible-sets
Jan 20, 2023
Merged

lib: make extender available on self-references#210976
roberth merged 1 commit intoNixOS:masterfrom
clhodapp:fix/extensible-sets

Conversation

@clhodapp
Copy link
Contributor

@clhodapp clhodapp commented Jan 15, 2023

Description of changes

Existing Behavior

At present, recursive self-references to extensible sets do not themselves have a reference to the extender function:

Welcome to Nix 2.12.0. Type :? for help.

nix-repl> masterLib = (builtins.getFlake "github:NixOS/nixpkgs").lib

nix-repl> r1 = masterLib.makeExtensible (self: { inherit self; })

nix-repl> r1
{ __unfix__ = «lambda @ (string):1:28»; extend = «lambda @ /nix/store/al18j0766bggz6hf2da2hrsmbyg8inij-source/lib/fixed-points.nix:111:25»; self = { ... }; }

nix-repl> r1.self
{ __unfix__ = «lambda @ (string):1:28»; self = { ... }; }

nix-repl> r2 = r1.extend (final: prev: { inherit final; })

nix-repl> r2
{ __unfix__ = «lambda @ /nix/store/al18j0766bggz6hf2da2hrsmbyg8inij-source/lib/fixed-points.nix:69:24»; extend = «lambda @ /nix/store/al18j0766bggz6hf2da2hrsmbyg8inij-source/lib/fixed-points.nix:111:25»; final = { ... }; self = «repeated»; }

nix-repl> r2.self
{ __unfix__ = «lambda @ /nix/store/al18j0766bggz6hf2da2hrsmbyg8inij-source/lib/fixed-points.nix:69:24»; final = { ... }; self = «repeated»; }

nix-repl> r2.final
{ __unfix__ = «lambda @ /nix/store/al18j0766bggz6hf2da2hrsmbyg8inij-source/lib/fixed-points.nix:69:24»; final = { ... }; self = «repeated»; }

Note: What you should be noticing is that r1 and r2 contain a function, extend, but none of the captured self-references have access to it.

Behavior With Change

With this change, the extender function becomes available:

nix-repl> ourLib = (builtins.getFlake "github:clhodapp/nixpkgs/fix/extensible-sets").lib

nix-repl> r3 = ourLib.makeExtensible (self: { inherit self; })    

nix-repl> r3
{ __unfix__ = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:110:11»; extend = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:111:25»; self = { ... }; }

nix-repl> r3.self
{ __unfix__ = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:110:11»; extend = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:111:25»; self = { ... }; }

nix-repl> r4 = r3.extend (final: prev: { inherit final; }) 

nix-repl> r4
{ __unfix__ = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:110:11»; extend = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:111:25»; final = { ... }; self = «repeated»; }

nix-repl> r4.self
{ __unfix__ = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:110:11»; extend = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:111:25»; final = { ... }; self = «repeated»; }

nix-repl> r4.final
{ __unfix__ = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:110:11»; extend = «lambda @ /nix/store/n6pm7h50pvva9kpbval5ifb1rhc33gvi-source/lib/fixed-points.nix:111:25»; final = { ... }; self = «repeated»; }

It behaves just as you would expect it would; Any extension made on the parent set shows up in captured child extensions:

nix-repl> r5 = ourLib.makeExtensible (self: { a = self.extend (self': prev: { b = self'.x;});})

nix-repl> r6 = r5.extend (self: prev: { x = 1; })

nix-repl> r6.a.b
1

Why?

This obviously makes the self-references "higher-fidelity" in the sense that they more closely match the final composed set. However, one might ask "Why do you actually need this?", given that the examples given so far seem somewhat abstract and arcane. The answer is: (NixOS, etc.) module composition!

The lib arg passed to modules happens to be an extensible set self-reference. Here's a manual traceback of code showing how that happens:

The effect of this is that the lib received by modules is not a full-featured nixpkgs lib. The impact of this is that you can't do the thing I'm trying to do: compose an extended nixpkgs lib in a module.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 15, 2023
@infinisil
Copy link
Member

Fyi this is also what #157056 (and the more minimal #160459) do

@clhodapp
Copy link
Contributor Author

clhodapp commented Jan 19, 2023

Well, this one certainly seems like the most minimal / cleanest of the three PRs to me (but of course I'm biased because I wrote it 😄).

My goal here was less ambitious than @ncfavier's: I seek only to make the final reference actually be a reference to the final attribute set, as opposed to a truncated version that is missing the extend function that it's supposed to have.

I am not trying to replace the lib that will be passed to other modules from within a module, I'm just trying to make a new lib locally that has been extended.

Perhaps this PR might be less contentious than the others due to the fact that is pretty much a pure bugfix?

@ncfavier
Copy link
Member

Yes, this change should go through regardless.

There were discussions about the change in my PR: #157056 (comment) #157056 (comment)

@ncfavier
Copy link
Member

ncfavier commented Jan 19, 2023

The evaluator report is a bit worrying though. +6.20% in cpu time? Let's double check
@ofborg eval

EDIT: that's better.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useful beyond lib.extend and fits the pattern of mkDerivation

mkDerivation (self: { passthru.xyz = self.overrideAttrs foo; })

@roberth
Copy link
Member

roberth commented Jan 20, 2023

The evaluator report is a bit worrying though. +6.20% in cpu time?

This happens every now and then. 40%? No problem.
It is hard to kill performance without also killing the memory stats.

Those are quite interesting actually.

~50 invocations of extend

nix-repl> 861199 / 50                                
17223

nix-repl> lib.length (lib.attrNames haskellPackages)
17212

Coincidence? Yes, but also no.

Anyway, the numbers are in the ballpark.

@roberth roberth merged commit a1b2177 into NixOS:master Jan 20, 2023
@clhodapp clhodapp deleted the fix/extensible-sets branch January 20, 2023 00:25
@ElvishJerricco
Copy link
Contributor

Am I crazy for thinking thart +6.20% is actually quite huge for a change almost no one will benefit from?

@clhodapp
Copy link
Contributor Author

clhodapp commented Jan 20, 2023

Yeah, @ncfavier was expressing concern about this change if it really increases evaluation time by that much. However, he re-ran the evaluator and it came back with +0.15% CPU time. I'm guessing that these timings are somewhat noisy because we are running in a very cloudy environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants