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

Freeform modules #82743

Merged
merged 8 commits into from
Aug 15, 2020
Merged

Freeform modules #82743

merged 8 commits into from
Aug 15, 2020

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Mar 16, 2020

Motivation for this change

This implement freeform modules, which are like a combination between types.submodule and types.attrsOf: The options declared with the submodule are type-checked, while the others are combined using a freeform type. This has been a desired feature in multiple occasions now:

How it looks like (updated for latest version) (evaluate with nix-instantiate --eval --strict example.nix -A config)

with import ./lib; evalModules {
  modules = [{
    options.settings = mkOption {
      default = {};
      type = with types; submodule {
        # All values that don't have an associated option have to be of this type
        freeformType = attrsOf int;

        options.port = mkOption {
          type = port;
          default = 80;
          description = "The port to use.";
        };
      };
    };

    # Works, even though foo isn't declared as an option
    config.settings.foo = 10;

    # error: The option value `settings.port' in `example.nix' is not of type
    # `16 bit unsigned integer; between 0 and 65535 (both inclusive)'.
    #config.settings.port = "not-a-port";

    ## Overrides default value
    config.settings.port = 8080;
  }];
}

Ping @roberth @Ma27 @rycee @Profpatsch @oxij

TODO
  • Write tests
  • Potentially separate orthogonal changes into a separate PR
  • Write docs
  • Check how this affects performance -> The current implementation is slower than it could be when many freeform values are defined, but since the number of them per module evaluation is expected to be rather low, I'll leave it at this for now.

@infinisil infinisil requested review from edolstra and nbp as code owners March 16, 2020 19:03
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Mar 16, 2020
@infinisil
Copy link
Member Author

I now separated the orthogonal changes into #82751

@infinisil infinisil force-pushed the partially-typed-v2 branch 2 times, most recently from 4534c07 to a7d6990 Compare March 16, 2020 23:17
@roberth
Copy link
Member

roberth commented Mar 17, 2020

I don't think attrValueType is necessary if you merge in one go at the root of the submodule and then recursiveUpdate the unstructured values back into config.
This puts less burden on the types and, if we don't hardcode recursiveUpdate, it allows the module author to keep the unstructured attributes separate if desired.

@infinisil
Copy link
Member Author

@roberth I don't think this would work without big changes, because currently it's not known upfront which values are unstructured and which aren't, and this could get complicated with nested submodules and attrsOf and such.

@roberth
Copy link
Member

roberth commented Mar 17, 2020

@infinisil mergeModules could return the option values and unstructured attrs separately. Doing so should also fix the laziness problem caused by generating fake option names for the unstructured definitions.

Quote for when the commit is lost:

TODO: This leads to infinite recursion in NixOS if it's always turned on, so some use cases won't be supported anymore because of unstructured attrs. Figure out what they are.

One feature I suspect may be lost due to this change is the ability to check for the existence of option declarations before defining them, which I think will be important when modules move into flakes and need to adapt a bit at runtime to their now variable environment.
I've used this to optionally define NixOps deployment keys in a NixOS-compatible module.
I'll make sure we have a test for this laziness property.

@roberth
Copy link
Member

roberth commented Mar 17, 2020

@infinisil test available: #82802

@infinisil
Copy link
Member Author

@roberth I think I see what you mean, this might be possible, I'll give it a try soon. Implementation would be a bit like: Do a recursive mergeModules' for declared options first. After that, determine which definitions haven't been matched with a declaration, and merge those with a single unstructured type. Then combine the results of the mergeModules' and the unstructured merge with something like recursiveUpdate (it will need to insert the fake option declarations). I don't think it needs to be configurable though, as it will essentially only allow unstructured values under option sets, which makes a lot of sense, and is also the behavior of the current implementation.

@infinisil infinisil force-pushed the partially-typed-v2 branch 2 times, most recently from 9ee0bf8 to 80a9553 Compare March 18, 2020 21:48
@infinisil
Copy link
Member Author

@roberth Done that! Commits need some cleanup, and other things are still left to do, but now the code is looking much nicer. I also did some very sweet cleanups related to this. Also I'm now not generating fake options anymore: Only .config will now contain unstructured values, .options doesn't, which should then also not get in the way of what you mentioned in #82802.

Unfortunately without attrValueType, a specific error message is significantly worse: With something like

{
  config._module.unstructuredType = attrsOf int;
  options.a.b = mkOption {};
  config.a.c = "foo";
}

it now throws

The option value `a' in `example.nix' is not of type `signed integer'.

and if you change it to

{ config.a = 0; }

you get (as expected)

error: The option path `a' is an attribute set of options,
  but it is defined to not be an attribute set in `example.nix'.
  Did you define its value at the correct and complete path?

Instead of the previous (code now in https://github.com/Infinisil/nixpkgs/tree/partially-typed-v2-old)

The option `a.c' defined in `example.nix' does not exist
  and the unstructured type `signed integer' of `a' does not support attribute sets.

I could get back the original error message if I add attrValueType again, but I'm not sure if it's worth it. Maybe that's better left for the future.

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Mar 19, 2020
@infinisil infinisil added the 6.topic: module system About "NixOS" module system internals label Mar 19, 2020
lib/modules.nix Show resolved Hide resolved
lib/modules.nix Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Mar 19, 2020

error message

This can probably be improved quite a bit by rethrowing the error with an extra message.
"The module at ${prefix} allows undeclared definitions to be mixed with known options, but a definition did not satisfy the type for undeclared definitions: ${caughtMsg}. This may also be the result of mistyping an otherwise known option."

@infinisil
Copy link
Member Author

@roberth Hm it does work with builtins.tryEval and a deepSeq on the unstructuredType.merge ..., but that's not very nice. I'd have to do some changes to how types throw errors to make it work without tryEval.

@infinisil infinisil closed this Mar 19, 2020
@infinisil infinisil force-pushed the partially-typed-v2 branch from dd1d716 to ec7db16 Compare March 19, 2020 23:35
@infinisil infinisil reopened this Mar 19, 2020
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 19, 2020
@infinisil infinisil force-pushed the partially-typed-v2 branch from 4a787f2 to 8279d9b Compare March 19, 2020 23:40
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 19, 2020
infinisil added a commit to infinisil/nixpkgs that referenced this pull request Aug 21, 2020
@infinisil infinisil mentioned this pull request Aug 21, 2020
1 task
@infinisil
Copy link
Member Author

Some more instances of this change finding mistakes/hacks:

I have opened #95932 to add release note entries for the feature and the backwards incompatibility this introduces.

infinisil added a commit to infinisil/nixpkgs that referenced this pull request Aug 21, 2020
infinisil added a commit to infinisil/nixpkgs that referenced this pull request Aug 21, 2020
jonringer pushed a commit that referenced this pull request Aug 21, 2020
jonringer pushed a commit that referenced this pull request Aug 21, 2020
@dali99
Copy link
Member

dali99 commented Oct 13, 2020

There doesn't seem to be any way to define typing for an optional setting, I was looking to set default = unset in some way, since some configuration handle null and "not set" differently.

@infinisil
Copy link
Member Author

@dali99 Indeed, that's something that could be improved in the future. See also #63553 which is related.

@aanderse aanderse mentioned this pull request Nov 28, 2020
10 tasks
@infinisil infinisil mentioned this pull request May 3, 2021
10 tasks
@infinisil infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants