[Reverted] Module-builtin assertions, disabling assertions and submodule assertions

[infinisil]

Motivation for this change

This implements checks (assertions and warnings) supported by the module system directly, instead of just being a NixOS option (see nixos/modules/misc/assertions.nix).

This has the following benefits:

• It allows cleanly redoing the user interface. The new implementation specifically allows:
  • disabling checks
  • converting errors to warnings
• Checks can now be specified easily from within submodules, which previously wasn't possible and needed workarounds
  • In particular this also allows using mkRemovedOptionModule and co. from within submodules

Usage

Checks can now be defined in any module (even submodules) like this:

{ config, options, ... }: {
  _module.checks.gpgSshAgent = {
    check = config.programs.gnupg.agent.enableSSHSupport -> !config.programs.ssh.startAgent;
    message = "If you have ${options.programs.gnupg.agent.enableSSHSupport} enabled,"
      + " you can't enable ${options.programs.ssh.startAgent} as well!";
  };

  _module.checks.grafanaPassword = {
    check = config.services.grafana.database.password == "";
    message = "The grafana password defined with ${options.services.grafana.database.password}"
      + " will be stored as plaintext in the Nix store!";
    # This is a non-fatal warning
    type = "warning";
  };
}

Triggered assertion will display the attribute name they were defined as in [..]:

trace: warning: [grafanaPassword] The grafana password defined with
  services.grafana.database.password will be stored as plaintext in the Nix store!
error: Failed checks:
- [gpgSshAgent] If you have programs.gnupg.agent.enableSSHSupport
  enabled, you can't enable programs.ssh.startAgent as well!

This allows you to easily disable them or convert an assertion to a warning by using the same attribute that was printed:

{
  # Change the assertion into a non-fatal warning
  _module.checks.gpgSshAgent.type = "warning";

  # We don't care about this warning, disable it
  _module.checks.grafanaPassword.enable = false;
}

See the manual changes for more details (such as how submodule assertions work). Here is a screenshot of it:

Fixes #96006

Ping @roberth @nbp @shlevy @edolstra @jD91mZM2 @aanderse

Best reviewed commit-by-commit

Note that in order to be able to change checks (disable or convert to warning), they need to be defined using _module.checks, such that they have a name you can reference. Checks defined using the NixOS options assertions or warnings can't be changed (even though they're implemented using _module.checks now). While the intention is that all of the assertions/warnings in NixOS be given a name and be converted to _module.checks eventually, this PR doesn't do that so it stays manageable in size.

Things done

• Wrote tests
• Wrote docs
• Tested performance (no measurable difference for my machines configs)

[bqv]

Awesome stuff!

[roberth]

Perhaps we could add syntax sugar to make it top-level like config, freeformType etc? I don't think we should recommend defining stuff in _module. This will be everywhere.

It'd be great to have a way to turn all warnings into errors too.

I'll have to take a closer look at triggerPath, but do you think it's still going to be possible to collect all assertions and warnings without relying on trace and throw? I suppose even if we can, some configurations may turn out to accidentally(?) rely on laziness to avoid some checks.

Which reminds me - checks, perhaps that's usable as a generalization of assertions and warnings? For that matter, even derivations can serve as checks. So you'd have {config, ...}: { checks.portConflict.type = "assertion"; checks.checkConfigSyntax.check = pkgs.runCommandNoCC .....; }. It does collide somewhat with flake checks though, terminology-wise. Not sure if too out-of-the-box, but it does affect the naming scheme, so it's worth considering.

[infinisil]

Perhaps we could add syntax sugar to make it top-level like config, freeformType etc? I don't think we should recommend defining stuff in _module. This will be everywhere.

This is a bit tricky because there's already the assertions NixOS option. Maybe the sugar could be moduleAssertions instead though.

It'd be great to have a way to turn all warnings into errors too.

While I don't think this is a good idea, this can already be done with

{
  options._module.assertions = mkOption {
    type = attrsOf (submodule {
      type = mkDefault "warning";
    });
  };
}

I'll have to take a closer look at triggerPath, but do you think it's still going to be possible to collect all assertions and warnings without relying on trace and throw? I suppose even if we can, some configurations may turn out to accidentally(?) rely on laziness to avoid some checks.

I played around with them being collected on a per-module eval basis, such that they could be forwarded from submodules to the main module eval. I might give this another go.

Which reminds me - checks, perhaps that's usable as a generalization of assertions and warnings? For that matter, even derivations can serve as checks. So you'd have {config, ...}: { checks.portConflict.type = "assertion"; checks.checkConfigSyntax.check = pkgs.runCommandNoCC .....; }. It does collide somewhat with flake checks though, terminology-wise. Not sure if too out-of-the-box, but it does affect the naming scheme, so it's worth considering.

I like calling this checks instead. Interesting idea, I'll think about it.

[Profpatsch]

This is a bit tricky because there's already the assertions NixOS option. Maybe the sugar could be moduleAssertions instead though.

I think he meant on the same level as config options and imports.

[infinisil]

@Profpatsch The problem is that if a module is just { assertions = ...; } this becomes ambiguous: Is it the NixOS option assertions or is it the module-builtin assertions? I'd rather avoid the confusion and explicitly call it something different

[bqv]

+1 for checks

[roberth]

The problem is that if a module is just { assertions = ...; } this becomes ambiguous: Is it the NixOS option assertions or is it the module-builtin assertions? I'd rather avoid the confusion and explicitly call it something different

The shorthand syntax is fundamentally ambiguous, so the same can be argued about imports.

We don't have any checks options at the root of any submodules so I don't expect this to be a problem in practice.

[Profpatsch]

The problem is that if a module is just { assertions = ...; } this becomes ambiguous:

Do we have any modules that are only assertions? In that case we can migrate it manually; it seems low-chance enough that we can do a breaking change here (provided we have a good error message and migration documentation in the NixOS release notes).

Is it currently possible to define a module that has only e.g. options defined? I was under the impression that it will check for configs existence and based on that use the shortcut or not.

[roberth]

Part of this feature could be a generalized facility for values that propagate to the root of the configuration monoidally, not unlike a writer monad when used in a tree traversal. That's also how assertions and warnings should propagate, so the only extra bits of code you need are the assert and trace calls near evalModules returned config attribute.
Such a feature may be one of the ingredients for a radically different style of module writing, where modules stand alone like functions and somewhat more naturally solve the infamous "multiple database services" problem. It could be similar to an effect + handler system.

If this resonates with you, great! Otherwise feel free to ignore.

[infinisil]

@roberth I kinda get it, but I think that's a good discussion for another time 😃

Regarding introducing syntactic sugar, I thought long and hard about this now, and am coming to the conclusion that we should not do it.

The main insight I had was that _module doesn't have a _ at the start because it's for module-internal things, but rather to avoid clashing with option names. While @roberth pointed out that we don't have a checks option at the root, these options are added to submodules as well. So introducing sugar for this could cause problems for any module or submodule which has a checks option. While there is no such option in nixpkgs, we can't be sure that users don't have one on their own.

In addition, syntactic sugar would interact in non-optimal ways with submodules: You can't do

{
  myServices.foo.checks.portConflict.enable = lib.mkForce false;
}

because due to how submodules are implemented (for the default of shorthandOnlyDefinesConfig = true), this turns into config.checks = ..., which is not right, and as far as I can see, there's no reasonable way to make this work nicely. So people would still have to use _module.checks in such a case anyways.

---

In other news, it turns out that triggerPath isn't needed at all, so I'll remove that.

I'll also do the renaming from _module.assertions to _module.checks

And I think after that this PR should be about ready.

[infinisil]

Also, I'll introduce a _module.checks.<name>.check option, which is the condition that needs to hold true for the check to succeed. While this is very similar to _module.checks.<name>.enable, having this be separate is easier to reason about, and it allows reenabling the check after it's been turned off with enable = false.

[roberth]

The effect of this PR has made clear that we can't perform checks locally (entirely within small parts of the configuration), because it introduces strictness.

Instead, what we need is to have two global "phases". In the first phase, module evaluation happens exactly as it does now. It must not and can not depend on the checks. In the second phase, the checks are performed. This may still evaluate some parts that weren't evaluated during the first, but as said, won't refer back to the second phase to cause trouble.

While we can't enforce this in arbitrary Nix expressions, we can make this easy with the module system.
The first phase can be represented by a new (evalModules x).lazyConfig. The second will be represented by (evalModules x).config. The latter is defined as something like builtins.seq (runChecks lazyConfig._module.checks) lazyConfig, so nothing is duplicated.

Now it's impossible for modules/submodules to cause the problem, but we haven't actually wired up the checks from the submodules yet. Even without this, the change is already a positive because it standardizes the interface for writing checks and running checks without causing strictness issues.

Module authors could wire it up manually with something like

{
  config = mkIf cfg.enable {
    _module.checks = f options.services.foo.virtualHosts._module.checks;
  };
}

if we expose _module in options; similar to value, selecting _module instead of removing it.

Alternatively or additionally, we could automate this by scanning all options for submodules.

nodes are defined to be the final system config of all defined nodes, here. The problem is that the node references its own config through the nodes value, which with the latest state of this PR triggers assertions, which of course depend on the nodes config itself, therefore the infinite recursion.

This won't be a problem with the above proposal, if the nodes are defined through attrsOf submodule.

If that's not feasible (I'd be surprised) the code could just use lazyConfig and do the seq foo.config on its own.

A triggerPath will alleviate the problem since it would only trigger assertions at accessing of e.g. system.build.toplevel

This can still cause strictness issues that then can't be avoided. Assertions won't be able to use toplevel, which is not top level anyway. Image modules use it and you could reverse the toplevel and activation dependency for immutable systems. Another potential problem is that you could evaluate a configuration for only some options that do not include toplevel but should be checked nonetheless.

[roberth]

This seems to be one of the few 'architectural' weaknesses that can't be addressed by good module design.

[roberth]

Supermodules (#152785) are another use case for an out-of-band (non-config) module return value.

[infinisil]

I want to look into this again. For now I just rebased this PR on top of latest master to evaluate it again, here's that for reference: infinisil@b013442

[roberth]

I think what I'd try is

1. Extend types and options so we can retrieve all submodules out-of-band,
2. Write a function for inheriting checks from an option,
3. Rename evalModules to evalModulesLazy in all of lib/,
4. Write evalModules using evalModulesLazy, but performing the checks before returning config. This solves the strictness problem by removing all intermediate "seqs", while module system users still run checks by default.

Making (2) automatic would be nice but also seems like a performance hazard, especially without a minimal module list. It'd be a one-liner inside the mkIf cfg.enable. Something like

config = mkIf cfg.enable {
  checks = inheritChecks options.services.foo;
};

or maybe it should be like this

config = mkIf cfg.enable { ..... };
checks = inheritChecks cfg.enable options.services.foo;

[roberth]

Not having this is a layering violation, as lib.mkRenamedOptionModule and friends reference these options, that are implemented by NixOS and not exposed separately.

Encountered this again while working on the NixOS test framework #176557.

[infinisil]

A discussion relating to this PR is happening here: #207187

[roberth]

"lint" would be a nice additional type: like warnings, but not shown by default, because they are low priority and may be wrong.
The user would have to enable lint visibility with an option to turn them into trace messages (warnings?), or use a tool like nix repl to find the lint messages.