search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.16k stars 14.18k forks source link

All options should have types #76184

Closed dasJ closed 2 years ago

dasJ commented 4 years ago

Describe the bug

To make the usage of options and errors more clear, every option should have a type. Currently, more than 300 199 151 117 27 20 18 options don't have a type.

To Reproduce Steps to reproduce the behavior:

```nix let config = { }; pkgs = import ./. {}; lib = import ./lib; scrubDerivations = namePrefix: pkgSet: lib.mapAttrs (name: value: let wholeName = "${namePrefix}.${name}"; in if lib.isAttrs value then scrubDerivations wholeName value // (lib.optionalAttrs (lib.isDerivation value) { outPath = "\${${wholeName}}"; }) else value ) pkgSet; scrubbedEval = lib.evalModules { modules = import ./nixos/modules/module-list.nix; specialArgs = { pkgs = scrubDerivations "pkgs" pkgs; }; }; manual = import ./nixos/doc/manual { inherit config pkgs; inherit (scrubbedEval) options; version = "master"; revision = "master"; }; in manual.optionsJSON ``` ```bash jsonFile="$(NIX_PATH='' nix-build types.nix)/share/doc/nixos/options.json" jq -r 'to_entries[] | select(.value.type == "unspecified") | [ "`" + .key + "`", "[" + .value.declarations[0] + "](../blob/master/" + .value.declarations[0] + ")" ] | join(" - ")' < "${jsonFile}" | sed 's/^\(.*\)$/- [ ] \1/g' ```

Expected behavior

All options should have a type.

Additional context

Options without types:

dasJ commented 4 years ago

cc @ajs124 @Infinisil @Scriptkiddi @Ma27 @Lassulus

aanderse commented 4 years ago

@dasJ great! Any plan of attack, or just track it here and people can chip away at it?

BTW - I noticed services.wordpress.commonOptions but no such option exists that I'm aware of.

dasJ commented 4 years ago

@aanderse I'd love to attack this somehow, but as I honestly don't have any idea what most modules do, I'm too afraid to break anything. I'd love people to chip away at it, and I'll try to do as much as I can.

As for the Wordpress options: I accidentially added our company modules into the list. I'm currently regenerating it ;)

bbjubjub2494 commented 4 years ago

I looked at some of those options to try and type them. I ran into an interesting situation with services.tor.tsocks.config: the implementation of the module sets this option by itself, which means that if the user uses it, NixOS does the default thing which appears to be to concatenate everything together without separators, in the style of types.string. (as opposed to types.str which cant be merged) I concluded that, if we want to put a type on such an option while preserving its behavior by oral contract, the deprecated types.string is what should be used. Of course every option should be looked at case-by-case. (Also I think the tsocks module should be nuked at some point.)

I'm working on my branch here. Do we want to centralize at least the trivial cases somewhere instead of opening loads of small PRs?

dasJ commented 4 years ago

@aanderse I finally regenerated the list and updated the command to make it usable for everyone

dasJ commented 4 years ago

@lourkeur Yeah, I think that some larger PRs (like #76551 which fixes all nagios options at once) would be a lot easier to merge.

stale[bot] commented 4 years ago

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.
bbjubjub2494 commented 4 years ago

Updated command: curl -L https://hydra.nixos.org/job/nixos/trunk-combined/nixos.options/latest/download/1/options.json | jq -r 'to_entries[] | select(.value.type == "unspecified") | .key | "- [ ] " + .' result:

That's good progress but it's not all done.

bbjubjub2494 commented 4 years ago

Related: #91813

xaverdh commented 3 years ago

was this closed intentionally or by accident?

dasJ commented 3 years ago

Thanks to some newfound motivation for this issue (and procrastination from my regular work :eyes:) I chose to redo my generation code (goes from master instead of the release branch now) and reopen this. Good news is that a lot of the items are already fixed so I'm going to just replace the list above.

Update: List now includes clickable links

dasJ commented 3 years ago

Probably cc @Infinisil do you have any idea why services.compton shows up here? It's mkAliasOptionModule'd to services.picom but the type does not seem to be properly applied (showing as types.unspecified).

infinisil commented 3 years ago

@dasJ Isn't types.unspecified what your script looks for?

dasJ commented 3 years ago

Yes that's why I'm wondering. Shouldn't the type be the same as the type of services.picom? Or is the type implicit unspecified because it's not a real option?

infinisil commented 3 years ago

Oh I see the problem: The mkAliasOptionModule code just tries to get the type from options.services.picom.type, which in this case doesn't exist, because services.picom isn't an option, it's just an attribute set of options.

infinisil commented 3 years ago

@dasJ Fixing that in https://github.com/NixOS/nixpkgs/pull/110392 :)

turboMaCk commented 3 years ago

I think this change will have one downside. Some options might be taking function as an argument. In these cases the underlying implementation within module does the passing of value into this function. Many of these functions are not covered by test suite and will fail during rebuild of systems while not being fixable by users without defining overlays. I think errors like can happen way too easily. See for instance https://github.com/NixOS/nixpkgs/pull/111451#discussion_r567434993

I'm not saying that I'm strictly against these changes but I think we need to take this to account and try to minimize the risk of introducing similar regressions. Putting runtime type checks on options is simply not like turning nix to statically typed language.

Scriptkiddi commented 3 years ago

I think this change will have one downside. Some options might be taking function as an argument. In these cases the underlying implementation within module does the passing of value into this function. Many of these functions are not covered by test suite and will fail during rebuild of systems while not being fixable by users without defining overlays. I think errors like can happen way too easily. See for instance #111451 (comment)

I'm not saying that I'm strictly against these changes but I think we need to take this to account and try to minimize the risk of introducing similar regressions. Putting runtime type checks on options is simply not like turning nix to statically typed language.

I actually have only encountered a handful of options like this, I reverted changes where I probably did the same error. Let's see what the solution for the xmonad issue is going to be an see if we can apply that to the last remaining options.

stale[bot] commented 3 years ago

I marked this as stale due to inactivity. → More info

infinisil commented 3 years ago

Still relevant, and my #132448 should help with this

Scriptkiddi commented 3 years ago

@dasJ could you regenerate the list? some things are out of date

dasJ commented 3 years ago

Done