Skip to content

Comments

lib: add apply and callWith#206619

Open
alex-ameen wants to merge 4 commits intoNixOS:masterfrom
alex-ameen:lib-apply-cw
Open

lib: add apply and callWith#206619
alex-ameen wants to merge 4 commits intoNixOS:masterfrom
alex-ameen:lib-apply-cw

Conversation

@alex-ameen
Copy link
Contributor

Description of changes

Adds two new lib routines:
lib.apply and lib.callWith as members of customization.nix.

Things done

Ran lib/tests/misc.nix tests.

  • 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 Dec 17, 2022
Comment on lines 306 to 307
nix-repl> lib.apply ( { x, y }: x + y ) { x = 1; y = 2; z = -1; }
3
Copy link
Member

Choose a reason for hiding this comment

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

This has a very similar effect to

Do we need the body to be strict in the attribute names? If not, lazyFunction is strictly more useful, pardon the pun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that this has some overlap with lazyFunction ( I like your style, https://github.com/aakropotkin/ak-nix/blob/7242b7d7c49037700def339ea232d594b67dcd7c/lib/funk.nix#L346 there's no denying that we've got similar taste ); but my goal with this routine was intentionally limited to filtering a set of auto args rather than making them lazy ( which is just a side effect ).

If you've got any recommendations to avoid stepping on each others toes let me know. We wouldn't want to add redundant routines and you definitely beat me to the punch with this PR in October.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think yours actually makes it lazy though?

nix-repl> lib.fix (lib.applyArgs ({ a, b }: { a = b; b = 1; }) )
error: infinite recursion encountered

Maybe this looks like making up requirements, which in a way it is, but I'm concerned about function sprawl. As in the Fairbairn threshold.

Copy link
Member

Choose a reason for hiding this comment

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

It's a real tragedy that we have an incentive to inline frequently called functions like this to gain better performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, I wouldn't expect that lib.applyArgs snippet to work because you've just got a lambda there. Lets review the definition:

  applyArgs = f: args: let
    fn = if lib.isFunction f then f else import f;
  in fn ( builtins.intersectAttrs ( lib.functionArgs fn ) args );

The note I have about "this is only meant for functions that accept attrsets" was alluding to this a bit.

I'd describe this routine as more of "useful idiom"/short-hand rather than a generalized "works everywhere, do what I mean" utility ( we've got more robust routines for those situations anyways ).

Copy link
Member

Choose a reason for hiding this comment

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

It's not just a lambda, but equivalent to

nix-repl> let r = lib.applyArgs ({ a, b }: { a = b; b = 1; }) r; in r
error: infinite recursion encountered

Copy link
Contributor Author

@alex-ameen alex-ameen Dec 18, 2022

Choose a reason for hiding this comment

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

Wait I just reread this again.

The applyArgs here is right to blow up because your second argument isn't an attrset.

This could be fixed with an assert builtins.isAttrs args; so that the error message is more sensible, but at bottom this is a type mismatch.

applyArgs :: function -> attrs -> any

So calling a partial applyArgs function function is an error. The message about infinite recursion is misleading but you'd get the same error with let r = ( { a, b }: { b = 1; a = b + 1; } ) r; in r. The evaluator is griping in an unhelpful but technically accurate way.

* apply: rename to applyArgs.

* callWith: change references to "thunk" to "auto args".
@alex-ameen alex-ameen requested review from roberth and removed request for edolstra, infinisil and nbp December 18, 2022 18:13
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants