Don't require ellipsis in NixOS module definitions.#254212
Don't require ellipsis in NixOS module definitions.#254212vkryachko wants to merge 1 commit intoNixOS:masterfrom
Conversation
Having used `callPackage` a bit, it seems a bit odd and inconsistent that modules require `...` in their signatures as opposed to figuring out the signature and passing only what's requested, like `callPackage` does. Now I don't know if there is an actual reason for why it is important for modules to require `...`, but I could not find any and upon a brief chat with @infinisil, it seemed like there might not be a reason. So if you know of a reason, please let me know. Worth noting that, as currently implemented, it's is a breaking change, namely modules that use at-pattern arg capture don't work (`{ ... }@args`, args used to contain all available module args, but is now empty, see newly added failing test for details). I don't have any data on how common it is to write modules with at-pattern capturing, so would like to get some guidance on how important it is to preserve this behavior. It's possible to make it a non-breaking change by detecting `...` and/or `@args` with something like NixOS#194992 or rather with a new builtin like was proposed in NixOS#7317, and passing all available arguments if detected. Would like to get your thoughts on the idea to see if there are any strong objections to pursuing this further.
github-actions
bot
added
6.topic: module system
NixOSInfra
added
the
12.first-time contribution
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
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
is a breaking change
That's correct and I don't see the benefit of doing this.
Even with more function reflection info, I don't believe we should do this.
Reflection/introspection make programs divert from normal language semantics, so it should only be used as a last resort, such as to avoid breaking changes and help users migrate their code. For example NixOS/nix@e9e1897
Even in these cases, users should be migrated off the weird behavior to something that fits within normal language semantics, e.g. with a deprecation warning.
wegank
added
the
2.status: stale
Actually, it is likely not abreaking change after all. I did some archeology on the module system source code and stumbled upon this PR #6794 Its description states the following
I've done some testing and this appears to be still the case, i.e. any custom args that I don't mention in the formals, does not appear in So if that's accurate, we can improve ergonomics of writing modules at a very low cost @roberth wdyt? does the fact that it appears backwards compatible, change anything for you? |
stale
bot
removed
the
2.status: stale
Many modules will keep working, that is true, but it is a breaking change nonetheless. Consider this module: top@{ ... }: {
foo = { config, ... }: {
bar = something config top.config.qux;
}
qux = <...>;
}By simply merging this PR, we do break it, because I think you're right that with better reflection, we could pick the right behavior, but that's not available yet. Unfortunately that means this will be blocked for a long time, considering Nixpkgs' range of Nix version support. |
| # operator is used to make the "args@{ ... }: with args.lib;" notation | ||
| # works. | ||
| in f (args // extraArgs); | ||
| in f (builtins.intersectAttrs (functionArgs f) (args // extraArgs)); |
There was a problem hiding this comment.
Looks like performance can improve slightly with this change:
| in f (builtins.intersectAttrs (functionArgs f) (args // extraArgs)); | |
| in f extraArgs; |
(and a rename would be in order)
wegank
added
the
2.status: stale
FliegendeWurst
added
the
2.status: needs-changes
stale
bot
removed
the
2.status: stale
wegank
added
the
2.status: stale
wegank
added
the
2.status: merge conflict
nixpkgs-ci
bot
removed
the
12.first-time contribution
5 participants
Description of changes
Having used
callPackagea bit, it seems a bit odd and inconsistent that modules require...in their signatures as opposed to figuring out the signature and passing only what's requested, likecallPackagedoes.Now I don't know if there is an actual reason for why it is important for modules to require
..., but I could not find any and upon a brief chat with @infinisil, it seemed like there might not be a reason. So if you know of a reason, please let me know.Worth noting that, as currently implemented, it's is a breaking change, namely modules that use at-pattern arg capture don't work (
{ ... }@args, args used to contain all available module args, but is now empty, see newly added failing test for details). I don't have any data on how common it is to write modules with at-pattern capturing, so would like to get some guidance on how important it is to preserve this behavior.It's possible to make it a non-breaking change by detecting
...and/or@argswith something like #8961, and passing all available arguments if detected. Similar prior art: #194992 #7317.Would like to get your thoughts on the idea to see if there are any strong objections to pursuing this further.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)