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

Add functionInfo builtin. #8961

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

vkryachko
Copy link
Contributor

This function returns full information about the function, including its formal arguments, whether formals have an ellipse, and optionally the name of the @-pattern argument.

Motivation

It's sometimes useful to know if the function uses @-pattern to know if it's safe to call it with callPackage-style, i.e. only passing its "formals".

Context

Useful in #254212.
A similar change has previously bee proposed as well #7317
It's also a less hacky solution to #194992

Priorities

Add 👍 to pull requests you find important.

This function returns full information about the function, including its
formal arguments, whether formals have an ellipse, and optionally the
name of the @-pattern argument.

Motivation: it's sometimes useful to know if the function uses @-pattern
to know if it's safe to call it with callPackage-style, i.e. only
passing its "formals" to it. i.e. useful in
[254212](NixOS/nixpkgs#254212).
A similar change has previously bee proposed as well NixOS#7317
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Sep 10, 2023
@roberth
Copy link
Member

roberth commented Sep 11, 2023

#8908 also implements most of the reflection logic proposed here, as separate primops.

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.

A mostly equivalent change is proposed as part of

Differences to consider

  • return an attrset or have multiple functions?
    • 8908 is more consistent with functionArgs and avoids committing the language to what I consider to be a bad name, formals
    • this could be considered to promote consistency by having a single format for the combined info
  • use semantically meaningful names or syntactic names? The latter make interoperability with a possibly different syntax ugly. (think cue or python, or functions defined through the bindings)
  • expose the difference between a@{...}: and a:? the prior of which is strict and may perhaps warrant a custom error or warning in some cases

// !!! should optimise booleans (allocate only once)
formals.alloc(i.name, i.pos).mkBool(i.def);
auto attrs = state.buildBindings(3);
if(args[0]->lambda.fun->arg) attrs.alloc("arg").mkString(state.symbols[args[0]->lambda.fun->arg]);
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should not expose this information. It is not needed, and it will introduce unnecessary coupling that users will have to be aware of when renaming what is otherwise a completely internal name.

Comment on lines +2812 to +2814
for (auto & i : args[0]->lambda.fun->formals->formals)
// !!! should optimise booleans (allocate only once)
formals.alloc(i.name, i.pos).mkBool(i.def);
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be common with the functionArgs primop and should be factored out.

auto attrs = state.buildBindings(3);
if(args[0]->lambda.fun->arg) attrs.alloc("arg").mkString(state.symbols[args[0]->lambda.fun->arg]);
attrs.alloc("ellipsis").mkBool(args[0]->lambda.fun->formals->ellipsis);
attrs.alloc("formals").mkAttrs(formals);
Copy link
Member

Choose a reason for hiding this comment

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

I believe formals is an unfortunate name, and it has so far only been part of documentation, and not the language itself. This would be the last chance to fix that mistake.
args would match functionArgs. Will come back to this.

@roberth
Copy link
Member

roberth commented Mar 28, 2024

I've written #10350, to be triaged by the Nix team.

@vkryachko
Copy link
Contributor Author

I've written #10350, to be triaged by the Nix team.

Cool, thanks @roberth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants