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

Make functionArgs primitive accept primops (fix #3624) #3626

Merged
merged 1 commit into from Sep 25, 2020

Conversation

W95Psp
Copy link
Contributor

@W95Psp W95Psp commented May 25, 2020

Hi!

I ran into the bug I documented in #3624, that is functionArgs not accepting primops or partial applications or primops.
This PR just makes functionArgs returns {} when a primitive operation is given.

It compiles properly for me, and as expected, evaluating (import <nixpkgs>).lib.functionArgs map indeed yields {}.

@puckipedia
Copy link
Contributor

This is not correct behavior, as there are primops (like builtins.fetchGit) that take an attrset of a certain shape, which can't be verified with this.

@W95Psp
Copy link
Contributor Author

W95Psp commented May 28, 2020

Oh, right, I did not think about such primops.
However, it seems that most of primops that accept attrset as one of their formal argumentrs (from what I saw, this includes prim_fetchMercurial, prim_fetchTree, prim_fetchurl, prim_fetchTarball, prim_fetchGit, and others) are polymorphic and accepts other types (such as strings). Such a "polymorphic" function f in nix would looks like x: if builtins.typeOf x = "set" then ... else ..., and then functionArgs f yields {}.

Therefore, what should be the correct behavior for such "polymorphic" primops?

I found prim_genericClosure that takes an attrset and nothing else, I guess there's a couple more. But then, what is the good strategy? Decide such primops are rare and add manually cases in functionArgs would be rather ugly. Another option would I guess be to change addPrimOp so that instead of simply taking an arity, it takes a list of maybe attr names (say list (maybe (list name))), and reflect that change in PrimOp struct.

This would allow functionArgs to have the proper behaviour for both primops and primops partial applications.

Not too sure it's a good idea though.

@puckipedia
Copy link
Contributor

oh huh, i was not aware you could builtins.fetch* strings. huh

@infinisil
Copy link
Member

infinisil commented Sep 24, 2020

I think this change is correct and should be merged because:

  • The semantics of builtins.functionArgs are that any arguments it returns have to be accepted by the function, but that doesn't mean that other arguments can't be accepted. So returning {} for primops is an entirely consistent and safe behavior
  • In fact, primops are implemented very much like args: ... args.foo ... Nix functions, taking attributes from args whenever they need. And builtins.functionArgs for such Nix functions also obviously returns {}
  • In addition, the current behavior of builtins.isFunction returning true for primops, yet builtins.functionArgs failing is very inconsistent

@Ericson2314
Copy link
Member

All this stuff is wack, but @infinisil does make sense. And if functionArgs is a conservative approximation, I guess we can give it more info about primops later?

@edolstra edolstra merged commit cbb9862 into NixOS:master Sep 25, 2020
@edolstra
Copy link
Member

Thanks, makes sense!

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

5 participants