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

Fix functionArgs for primops #4062

Closed
wants to merge 1 commit into from
Closed

Conversation

infinisil
Copy link
Member

Both functions and primops have builtins.typeOf v == "lambda", and they also both return true for builtins.isFunction, yet builtins.functionArgs fails for primops without this change:

$ nix-instantiate --eval -E 'builtins.isFunction builtins.add'
true

$ nix-instantiate --eval -E 'builtins.functionArgs builtins.add'
error: 'functionArgs' requires a function, at (string):1:1

For simplicity, primops aren't assumed to have named function arguments, which is correct for most of them, so builtins.functionArgs now just returns {} for all of them

Both functions and primops have `builtins.typeOf v == "lambda"`, and
they also both return true for `builtins.isFunction`, yet
`builtins.functionArgs` fails for primops without this change:

  $ nix-instantiate --eval -E 'builtins.isFunction builtins.add'
  true

  $ nix-instantiate --eval -E 'builtins.functionArgs builtins.add'
  error: 'functionArgs' requires a function, at (string):1:1

For simplicity, primops aren't assumed to have named function arguments,
which is correct for most of them, so `builtins.functionArgs` now just
returns `{}` for all of them
@edolstra
Copy link
Member

This looks similar to #3626. There was some discussion there about whether this is a good idea.

@infinisil
Copy link
Member Author

Oh yeah this is actually the very same change, I'll direct my comments to the other PR.

@infinisil infinisil closed this Sep 24, 2020
@infinisil infinisil deleted the primop-funargs branch September 24, 2020 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants