lib.modules: Let module declare options directly in bare submodule

[roberth]

Motivation for this change

Let module declare options directly in bare submodule, where a bare submodule is an option that has a type like submoduleWith x, as opposed to attrsOf (submoduleWith x).

This makes migration unnecessary when introducing a freeform type in an existing option tree and it removes some unwieldy syntax. Refs https://github.com/NixOS/nixpkgs/pull/156503/files#r790768951

Mainly, it allows the introduction of a freeformType at any place in an existing options tree without breaking compatibility with existing option declarations inside the new freeformType.

Another use case is for modules that should be imported in multiple locations in the options tree, such as systemd and systemd.user.

Closes #146882

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[infinisil]

This leads to an error when a users option is declared with just types.submodule:

let
  lib = import ./lib;
in lib.evalModules {
  modules = [
    {
      _file = "a.nix";
      options.foo = lib.mkOption {
        type = lib.types.submodule {};
        default = {};
      };
    }
    {
      _file = "b.nix";
      options.foo.bar = lib.mkOption {
        type = lib.types.str;
      };
    }
  ];
}

gives

error: A submoduleWith option is declared multiple times with conflicting shorthandOnlyDefinesConfig values
(use '--show-trace' to show detailed location information)

Not sure if this can be fixed nicely or if it even makes sense.

[roberth]

submodule is the legacy one, but yeah, not great. Seems like "don't care" needs to be representable and pick true as the default. false was probably a bad idea.

[roberth]

Added null support, but only when merging.

[infinisil]

I wonder if it would be feasible for option trees (like options.foo.bar.baz = mkOption { ... }) to just be desugared to submodules (like options.foo = mkOption { type = submodule { options.bar = mkOption { type = submodule { options.baz = mkOption { ... }; }; }; }; }) and in the process cleaning up the code 🤔

[roberth]

I wonder if it would be feasible for option trees to just be desugared to submodules and in the process cleaning up the code

It occurred to me too, but it wasn't clear to me whether it will be effective.
Calling back to "high-level" functions like submodule may not be great for performance, so it seems like it'd be a significant rethink. Not something I'll explore in this PR.

[infinisil]

Since null is now an allowed value, we should also define its semantics and document it. The semantics should be that if this value is needed to make a decision and it's null, an error is thrown. This means that if you do any shorthand definitions, you should get an error, unless another option declaration specifies the value of shorthandOnlyDefinesConfig, e.g. whereas

let
  lib = import ./lib;
in lib.evalModules {
  modules = [
    {
      options.foo = lib.mkOption {
        type = lib.types.submoduleWith {
          modules = [];
          shorthandOnlyDefinesConfig = null;
        };
      };

      config.foo.bar = 10;
    }
  ];
}

with this PR currently throws

error: value is null while a Boolean was expected

       at /home/infinisil/src/nixpkgs/lib/types.nix:538:23:

          537|           then setFunctionArgs (args: unify (value args)) (functionArgs value)
          538|           else unify (if shorthandOnlyDefinesConfig then { config = value; } else value);
             |                       ^
          539|
(use '--show-trace' to show detailed location information)

it should instead be something like

error: shorthandOnlyDefinesConfig is null
(use '--show-trace' to show detailed location information)

[roberth]

Since null is now an allowed value

I disagree. You'd have to read the implementation to find that null is acceptable in some situations.

To keep things simple and, I've decided to default it to true. I don't see much point in documenting this though.

[infinisil]

After a lot of trial and error, trying to prove why fixupOptionType should be used here or not, I figured it out: It's needed for the sake of file locations in error messages. E.g. with

let lib = import ./lib;
in lib.evalModules {
  modules = [
    {
      _file = "a.nix";
      options.foo = lib.mkOption {
        type = lib.types.submodule {
          options.x = lib.mkOption {
            type = lib.types.int;
          };
        };
        default = {};
      };
    }
    {
      _file = "b.nix";
      options.foo.x = lib.mkOption {
        type = lib.types.str;
      };
    }
  ];
}

it currently produces this error:

$ nix-instantiate --eval -A config.foo.x
error: The option `foo.x' in `<unknown-file>' is already declared in `<unknown-file>'.
(use '--show-trace' to show detailed location information)

whereas with a fixupOptionType here, it's

error: The option `foo.x' in `a.nix' is already declared in `b.nix'.
(use '--show-trace' to show detailed location information)

would be good to have a test case for this

[roberth]

Done.

[infinisil]

This should have a comment explaining the reasoning behind this: How it's a bit hacky but needed as a default, that null is only an internally valid value, that it's only safe here because of the merging function patch and because it's always going to be merged with at least one other module.

In the future this might become obsolete with #162398

[roberth]

Added

                # `null` is not intended for use by modules. It is an internal
                # value that means "whatever the user has declared elsewhere".
                # This might become obsolete with https://github.com/NixOS/nixpkgs/issues/162398

[infinisil]

Looking good!

[bobvanderlinden]

Could you explain the example for systemd a bit?

Would this allow something like:

let
  units = {
    options = {
      units = mkOption { ... };
    };
    config = { ... };
  };
in
{
  options = {
    boot.systemd = mkOption {
      type = types.submoduleWith {
        modules = [ units services mounts ... ];
      };
    };
  };
}

(That would be amazing)

[roberth]

@bobvanderlinden Yes, while this was already valid, it can now be mixed with "top-level" options like

{
  options.boot.systemd.units.foo = mkOption { };
}

This was not allowed to coexist with your example.
So the refactoring you want to do has become less risky and possible to do incrementally, without having to migrate all options to the submodule at once.

[roberth]

I wonder if it would be feasible for option trees (like options.foo.bar.baz = mkOption { ... }) to just be desugared to submodules (like options.foo = mkOption { type = submodule { options.bar = mkOption { type = submodule { options.baz = mkOption { ... }; }; }; }; }) and in the process cleaning up the code thinking

Today pennae said on IRC:

submodules merge at the level of their type in the hierarchy, so that assignment also contains an enable value

and it struck me that such a refactoring probably increases strictness by merging attrsets too soon.

I don't think this insight affects this PR itself, because while it makes the introduction of submodules in existing option trees more feasible, it is not this change but those introductions that cause the increased strictness.

Perhaps a better solution would be to treat bare submodules as option trees with a prefix. This eliminates the extra strictness caused by bare submodule introductions.

[roberth]

tl;dr we can improve laziness by translating bare submodules to option trees rather than the other way around, or we can implement a special "import at prefix" that does only what we need.

Submodules are more than just option trees though, so a "prefixing" operation on modules seems like it'd need quite a lot of code.
A more restricted "import at prefix" feature that only deals with options and configs may be easier to implement, as it doesn't have to translate freeformTypes. Maybe I'm overestimating the complexity of translating all submodule functionality though.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/genodepkgs-extending-nixpkgs-nixos-to-genode/8779/7

[roberth]

This PR reinterprets parts of the option tree based on information of explicitly declared options.
This could be generalized to not just the improvement of attrsets in the option tree, but also plain functions in the option tree.
Specifically, we could perhaps make the optionType itself responsible for interpreting the subexpressions of options for the purpose of static merging.

I call it static merging for two reasons:

• It generalizes type merging. Note that types are static information. (generally! You can easily do a dependent type, but this is meant to be descriptive, not prescriptive use of the term)
• It is static and can be used for declaring nested docs (e.g. docs in a submodule). This sets it apart from the config part, which is dynamic and not queried for docs.

Motivating example: hercules-ci/flake-parts#142

[Atry]

I wonder if we could allow for merging an option with its default values, no matter what the type is.