Improved module errors #98155

infinisil · 2020-09-17T12:10:31Z

Motivation for this change

This updates NixOS module system errors to print the actual values of the definitions. It also updates the error formatting a bit. Related issues:

Something like lib/modules.nix: provide detailed error locations #65024 could improve these errors with line and column info
Improve generators.toPretty #97133 will improve generators.toPretty, which is used by these error messages

Ping @Profpatsch @roberth @grahamc

Here is an overview of the new error messages, reproducable with the following file:

{ lib, config, pkgs, options, ... }: {

  _file = "/etc/nixos/configuration.nix";

  imports = [
    ({ lib, config, pkgs, ... }: {

      _file = "nixos/modules/services/foo.nix";

      options.services.foo = {
        dataDir = lib.mkOption {
          type = lib.types.str;
          readOnly = true;
          default = "/var/lib/foo";
          description = "";
        };

        dataDir' = lib.mkOption {
          type = lib.types.str;
          default = "/var/lib/foo";
          description = "";
        };

        fileContents = lib.mkOption {
          type = lib.types.lines;
          default = "";
          description = "";
        };

        arbitrary = lib.mkOption {
          description = "";
        };

        pkg = lib.mkOption {
          type = lib.types.package;
          description = "";
        };

        port = lib.mkOption {
          type = lib.types.port;
          description = "";
        };
      };

      config.environment.etc.foo.text = builtins.toJSON config.services.foo;
    })
    {
      _file = "/etc/nixos/extra.nix";
      services.foo.arbitrary = "hello";
      services.foo.pkg = pkgs.hello;
      services.foo.port = 8080;
    }
  ];

  config = {

    # Uncomment a single line of the ones below for a different error each

    #services.foo.enable = true;
    #services.foo.dataDir = "/data/foo";
    #services.foo.fileContents = { port = 10; foo = "foo"; };
    #services.foo.arbitrary = { foo = "foo"; };
    #services.foo.pkg = pkgs.openssh;
    #services.foo.port = 8000;


    boot.loader.grub.device = "nodev";
    fileSystems."/".device = "main/root";

  };
}

Non-existant option

error: The option `services.foo.enable' does not exist. Definition values:
- In `/etc/nixos/configuration.nix': true

Multiple readOnly definitions

error: The option `services.foo.dataDir' is read-only, but it's set multiple times. Definition values:
- In `nixos/modules/services/foo.nix': "/var/lib/foo"
- In `/etc/nixos/configuration.nix': "/data/foo"

Wrong type

error: A definition for option `services.foo.fileContents' is not of type `strings concatenated with "\n"'. Definition values:
- In `/etc/nixos/configuration.nix':
    {
      foo = "foo";
      port = 10;
    }

Unmergable definitions (when no type specified)

error: Cannot merge definitions of `services.foo.arbitrary'. Definition values:
- In `/etc/nixos/extra.nix': "hello"
- In `/etc/nixos/configuration.nix':
    {
      foo = "foo";
    }

Unique option defined multiple times

error: The unique option `services.foo.pkg' is defined multiple times. Definition values:
- In `/etc/nixos/extra.nix': <derivation /nix/store/5rj29hp44c8p6281imxz9h1klqa23ijs-hello-2.10.drv>
- In `/etc/nixos/configuration.nix': <derivation /nix/store/86zf5amc82hgs8g47xhbnfp34zsch51i-openssh-8.3p1.drv>

Note: This is pretty much the same error condition for readOnly options, however this failure is specified by the type itself, rather than the option. This happens e.g. with uniq (listOf str) or types.package

Conflicting definitions

error: The option `services.foo.port' has conflicting definitions values:
- In `/etc/nixos/extra.nix': 8080
- In `/etc/nixos/configuration.nix': 8000

Things done

Updated the tests for the new error messages

grahamc · 2020-09-17T13:28:53Z

I think this is really great, and the evaluation performance report indicates this is basically free -- nice!

One concern: - In /etc/nixos/configuration.nix': <δ:openssh-8.3p1>uses theδsymbol. However, as an uneducated plebe I didn't know whatδ` was, and didn't know how to ask about it beyond "what is this funny symbol?". I think it would be better to use Nix repl's description:

«derivation /nix/store/4pmrswlhqyclwpv12l1h7mr9qkfhpd1c-hello-2.10.drv»

I like this option better because it is a word somebody can say without needing to know more than English. (@infinisil suggested this specific change in a chat with him.)

grahamc · 2020-09-17T13:33:22Z

Furthermore, as @andir pointed out, the lambda and delta aren't rendered by the kernel in a TTY. (« and » do)

edolstra · 2020-09-17T13:37:41Z

What happens if definitions are very large values (or even circular/infinite, like derivations)?

infinisil · 2020-09-17T14:08:00Z

@edolstra toPretty doesn't recurse into derivations, those are just printed as shown above. Though I'll think about maybe adding a recursion and element limit to toPretty.

domenkozar · 2020-09-17T15:11:59Z

Not that it needs to be of scope in this PR (as it already improves the status quo),
but suggesting what users can do would really make it easier for beginners.

For example when there's a conflict the user can either remove one value or use mkForce.

infinisil · 2020-09-17T16:38:41Z

@domenkozar Oh great idea! I think I'll leave this for another PR indeed though.

@grahamc I just pushed a change to the toPretty improvement PR that will not use those special symbols anymore: #97133 (comment)

Ma27

Awesome work!

Profpatsch · 2020-09-18T18:06:06Z

@edolstra toPretty doesn't recurse into derivations, those are just printed as shown above. Though I'll think about maybe adding a recursion and element limit to toPretty.

Check it out:

nixpkgs/lib/debug.nix

Lines 88 to 109 in 93fd4b5

    
             /* Like `traceSeq`, but only evaluate down to depth n. 
        
                This is very useful because lots of `traceSeq` usages 
        
                lead to an infinite recursion. 
        
                Example: 
        
                  traceSeqN 2 { a.b.c = 3; } null 
        
                  trace: { a = { b = {…}; }; } 
        
                  => null 
        
              */ 
        
             traceSeqN = depth: x: y: with lib; 
        
               let snip = v: if      isList  v then noQuotes "[…]" v 
        
                             else if isAttrs v then noQuotes "{…}" v 
        
                             else v; 
        
                   noQuotes = str: v: { __pretty = const str; val = v; }; 
        
                   modify = n: fn: v: if (n == 0) then fn v 
        
                                 else if isList  v then map (modify (n - 1) fn) v 
        
                                 else if isAttrs v then mapAttrs 
        
                                   (const (modify (n - 1) fn)) v 
        
                                 else v; 
        
               in trace (generators.toPretty { allowPrettyValues = true; } 
        
                          (modify depth snip x)) y;

Profpatsch · 2020-09-18T18:06:41Z

Turns out this is what the allowPrettyValues argument to toPretty was for all along 😛

The type's check function already ensured that it can't be passed non-lists

For pretty-printing definitions, including file and values

If multiple definitions are passed, this evaluates them all as if they were the only one, for a better error message. In particular this won't show module-internal properties like `_type = "override"` and co.

infinisil · 2020-09-21T16:35:04Z

Changes since last revision:

The toPretty improvement PR is merged now. I rebased this PR on top of it. With it, the values are now printed on multiple lines, and derivations and functions use <derivation ..> and <function ..> instead of δ and λ
Values for error display are now evaluated with builtins.tryEval. If evaluation fails, no value is displayed
The pretty printed value is now limited to 5 lines only. If there's more than 5 lines, a ... is printed
The readOnly error message now doesn't show the module-internal _type attributes and co.
The error format is slightly updated: The value is now printed on a new line from the start if it has multiple lines

As an example showcasing all of this, the error you get when you set the system.requiredKernelConfig option to have readOnly = true is

error: The option `system.requiredKernelConfig' is read-only, but it's set multiple times. Definition values:
- In `/home/infinisil/src/nixpkgs/nixos/modules/system/boot/kernel.nix': [ ]
- In `/home/infinisil/src/nixpkgs/nixos/modules/virtualisation/xen-dom0.nix'
- In `/home/infinisil/src/nixpkgs/nixos/modules/system/boot/systemd.nix':
    [
      {
        assertion = <function>;
        configLine = "CONFIG_DEVTMPFS=y";
        message = "CONFIG_DEVTMPFS is not enabled!";
    ...
- In `/home/infinisil/src/nixpkgs/nixos/modules/system/boot/stage-1.nix':
    [
      {
        assertion = <function>;
        configLine = "CONFIG_TMPFS=y";
        message = "CONFIG_TMPFS is not yes!";
    ...
- In `/home/infinisil/src/nixpkgs/nixos/modules/system/boot/loader/systemd-boot/systemd-boot.nix'
- In `/home/infinisil/src/nixpkgs/nixos/modules/system/boot/kernel.nix':
    [
      {
        assertion = <function>;
        configLine = "CONFIG_MODULES=y";
        message = "CONFIG_MODULES is not yes!";
    ...
- In `/home/infinisil/src/nixpkgs/nixos/modules/services/hardware/udev.nix':
    [
      {
        assertion = <function>;
        configLine = "CONFIG_UNIX=y";
        message = "CONFIG_UNIX is not enabled!";
    ...
- In `/home/infinisil/src/nixpkgs/nixos/modules/programs/systemtap.nix'
- In `/home/infinisil/src/nixpkgs/nixos/modules/programs/criu.nix'
- In `/home/infinisil/src/nixpkgs/nixos/modules/hardware/video/amdgpu-pro.nix'
- In `/home/infinisil/src/nixpkgs/nixos/modules/config/zram.nix'
- In `/home/infinisil/src/nixpkgs/nixos/modules/config/swap.nix'

infinisil · 2020-09-25T15:03:01Z

This PR improves toPretty for it to be possible to render only the first couple lines, without evaluating the whole thing: #98761

infinisil requested review from edolstra and nbp as code owners September 17, 2020 12:10

infinisil added the 6.topic: module system About NixOS module system internals label Sep 17, 2020

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Sep 17, 2020

Ma27 approved these changes Sep 18, 2020

View reviewed changes

lib/types: Remove unreachable if branch

2ff7c3e

The type's check function already ensured that it can't be passed non-lists

infinisil force-pushed the improved-module-errors branch from 4538c0d to 465f66c Compare September 21, 2020 15:23

infinisil added 5 commits September 21, 2020 18:24

lib/options: Introduce showDefs

7c20e68

For pretty-printing definitions, including file and values

lib/modules: Improve error messages using showDefs

bdfcee2

lib/modules: Evaluate single defs for readOnly error

910dfdc

If multiple definitions are passed, this evaluates them all as if they were the only one, for a better error message. In particular this won't show module-internal properties like `_type = "override"` and co.

lib/tests: Allow grepping for newlines in error messages

4f0982b

lib/tests: Update for error message changes

8908766

infinisil force-pushed the improved-module-errors branch from 465f66c to 8908766 Compare September 21, 2020 16:25

infinisil mentioned this pull request Sep 22, 2020

Allow specifying a recursionLimit for toPretty #98477

Closed

10 tasks

edolstra merged commit ecc00bd into NixOS:master Sep 23, 2020

infinisil deleted the improved-module-errors branch September 23, 2020 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved module errors #98155

Improved module errors #98155

infinisil commented Sep 17, 2020 •

edited

grahamc commented Sep 17, 2020

grahamc commented Sep 17, 2020

edolstra commented Sep 17, 2020

infinisil commented Sep 17, 2020

domenkozar commented Sep 17, 2020

infinisil commented Sep 17, 2020

Ma27 left a comment

Profpatsch commented Sep 18, 2020

Profpatsch commented Sep 18, 2020

infinisil commented Sep 21, 2020 •

edited

infinisil commented Sep 25, 2020

Improved module errors #98155

Improved module errors #98155

Conversation

infinisil commented Sep 17, 2020 • edited

Motivation for this change

Non-existant option

Multiple readOnly definitions

Wrong type

Unmergable definitions (when no type specified)

Unique option defined multiple times

Conflicting definitions

Things done

grahamc commented Sep 17, 2020

grahamc commented Sep 17, 2020

edolstra commented Sep 17, 2020

infinisil commented Sep 17, 2020

domenkozar commented Sep 17, 2020

infinisil commented Sep 17, 2020

Ma27 left a comment

Choose a reason for hiding this comment

Profpatsch commented Sep 18, 2020

Profpatsch commented Sep 18, 2020

infinisil commented Sep 21, 2020 • edited

infinisil commented Sep 25, 2020

infinisil commented Sep 17, 2020 •

edited

infinisil commented Sep 21, 2020 •

edited