Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib.lazyFunction: init #194514

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

roberth
Copy link
Member

@roberth roberth commented Oct 4, 2022

Description of changes

Adds a function that makes attrset-matching functions lazy using reflection.
This is similar to what the module system does for module arguments, but not powerful enough to abstract that.

This isn't used in nixpkgs yet, but may be used in #193336; however, I'd like for this to be reviewed separately.

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/)
  • 22.11 Release Notes (or backporting 22.05 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.

cc @infinisil

@roberth roberth mentioned this pull request Oct 4, 2022
20 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 4, 2022
lib/trivial.nix Outdated Show resolved Hide resolved
lib/trivial.nix Outdated
@@ -467,6 +471,41 @@ rec {
then v
else k: v;

/*
Make a function lazy in its arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Make a function lazy in its arguments.
Make a function lazy in its argument.

I don't like how this detail matters, but it does because there's only one actual argument here, the destructuring only makes it seem like multiple arguments. And lazy in arguments could be interpreted as making the attribute values lazy instead of strict (but that part is always lazy)

Comment on lines +479 to +480
can return a the (partially evaluated) function body (`{ r = ...; }`).

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
can return a the (partially evaluated) function body (`{ r = ...; }`).
can return a the (partially evaluated) function body (`{ r = ...; }`).
`lazyFunction` can change such a function to not evaluate the attribute
names of the argument before the body is returned.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've rewritten a lot to make better use of the example instead.

lib/trivial.nix Outdated
Comment on lines 481 to 482
`lazyFunction` uses reflection on the passed in function and it only works
for functions defined with a "set pattern".
Copy link
Member

Choose a reason for hiding this comment

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

It using reflection (which is not really a known term) is an implementation detail, can be omitted. And the note about it only working with set patterns should be moved down.

Suggested change
`lazyFunction` uses reflection on the passed in function and it only works
for functions defined with a "set pattern".

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's relevant because using reflection breaks the function abstraction, which is a big deal.
I've removed it for now, because I agree describing the consequences is more important.

lib/trivial.nix Outdated
Comment on lines 484 to 485
It makes a function like `{ a, b }: { r = foo a b; }` behave like
`args: { r = foo args.a args.b; }`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It makes a function like `{ a, b }: { r = foo a b; }` behave like
`args: { r = foo args.a args.b; }`.
In other words, it makes a function like `{ a, b }: { r = foo a b; }` behave like
`args: { r = foo args.a args.b; }` instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this to show the "generated wrapper" instead. I think that helps more with understanding.

lib/trivial.nix Outdated
Comment on lines 487 to 492
It is not compatible with ellipsis patterns, `{ ... }:`, because only the
explicitly declared parameters are passed through. For example, the
following will not work: `lazyFunction(args@{ a, ... }: args.b)`;
even if the caller provides `b`.

It also removes the checks against unexpected arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It is not compatible with ellipsis patterns, `{ ... }:`, because only the
explicitly declared parameters are passed through. For example, the
following will not work: `lazyFunction(args@{ a, ... }: args.b)`;
even if the caller provides `b`.
It also removes the checks against unexpected arguments.
This function has some limitations:
- It doesn't work for functions that don't do attribute destructuring like `arg: { r = foo arg.a arg.b; }` (`a`/`b` wouldn't be accessible)
- It doesn't work for functions with ellipses (`...`) like `arg@{ a, ... }: { r = foo a arg.b; }` (`b` wouldn't be accessible)
- It removes checks against unexpected arguments, so `lazyFunction ({ a }: a) { a = "a"; b = "b"; }` doesn't give an error.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just calling it attribute destructuring here instead of "set pattern", because nobody calls it that

Copy link
Member

Choose a reason for hiding this comment

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

Somewhat jokingly I wrote #194992, which could check for some of these limitations in the code itself

Copy link
Member Author

Choose a reason for hiding this comment

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

An author of the Nix manual uses this term.

(functionArgs f)
)
)
(functionArgs f) ;
Copy link
Member

Choose a reason for hiding this comment

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

We could try to prevent some misuse by throwing an error when functionArgs f is {}, but that might not be worthwhile.

Copy link
Member Author

Choose a reason for hiding this comment

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

That could be a problem if someone uses lazyFunction to construct another function where constant functions are a valid argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Super bad example, but better than no example?

let
  testCase.foo = { a }: a * 2 == a + a;
  testCase.bar = { }: true;
in mapAttrs (k: lazyFunction) testCase

@roberth roberth mentioned this pull request Dec 18, 2022
13 tasks
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants