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

lib/modules: improve error-message for undeclared options if prefix contains no options #95446

Merged
merged 1 commit into from Aug 18, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Aug 14, 2020

Motivation for this change

An easy-to-make mistake when declaring e.g. a submodule is the accidental
confusion of options and config:

types.submodule {
  config = {
    foo = mkOption { /* ... */ };
  };
}

However the error-message

  The option `[definition 1-entry 1].foo' defined in `<expr.nix>' does not exist.

is fairly unhelpful because it seems as the options are declared at the
first sight. In fact, it took a colleague and me a while to track down such
a mistake a few days ago and we both agreed that this should be somehow caught
to save the time we spent debugging the module in question.

At first I decided to catch this error in the submodules-type directly
by checking whether options is undeclared, however this becomes fairly
complicated as soon as a submodule-declaration e.g. depends on existing
config-values which would've lead to some ugly builtins.tryExec-heuristic.

This patch now simply checks if the option's prefix has any options
defined if a point in evaluation is reached where it's clear that the
option in question doesn't exist. This means that this patch doesn't
change the logic of the module system, it only provides a more detailed
error in certain cases:

  The option `[definition 1-entry 1].foo' defined in `<expr.nix>' does not exist.

  However it seems as there are no options defined in [definition 1-entry 1]. Are you sure you've
  declared your options properly? This happens if you e.g. declared your options in `types.submodule'
  under `config' rather than `options'.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@infinisil infinisil added the 6.topic: module system About NixOS module system internals label Aug 15, 2020
@Ma27 Ma27 force-pushed the improve-opt-not-defined-error branch from bbaa5cc to f476ac9 Compare August 16, 2020 12:19
@Ma27
Copy link
Member Author

Ma27 commented Aug 16, 2020

Rebased onto master. @infinisil as you authored those changes, could you please take a look? :)

lib/modules.nix Outdated Show resolved Hide resolved
@Ma27 Ma27 force-pushed the improve-opt-not-defined-error branch from f476ac9 to aa5188f Compare August 16, 2020 16:13
@Ma27
Copy link
Member Author

Ma27 commented Aug 17, 2020

@infinisil anything else tbd? :)

lib/modules.nix Outdated Show resolved Hide resolved
lib/modules.nix Outdated Show resolved Hide resolved
@Ma27 Ma27 force-pushed the improve-opt-not-defined-error branch from aa5188f to de70b3b Compare August 18, 2020 10:38
lib/modules.nix Outdated Show resolved Hide resolved
…ontains no options

An easy-to-make mistake when declaring e.g. a submodule is the accidental
confusion of `options` and `config`:

    types.submodule {
      config = {
        foo = mkOption { /* ... */ };
      };
    }

However the error-message

  The option `[definition 1-entry 1].foo' defined in `<expr.nix>' does not exist.

is fairly unhelpful because it seems as the options are declared at the
first sight. In fact, it took a colleague and me a while to track down such
a mistake a few days ago and we both agreed that this should be somehow caught
to save the time we spent debugging the module in question.

At first I decided to catch this error in the `submodules`-type directly
by checking whether `options` is undeclared, however this becomes fairly
complicated as soon as a submodule-declaration e.g. depends on existing
`config`-values which would've lead to some ugly `builtins.tryExec`-heuristic.

This patch now simply checks if the option's prefix has any options
defined if a point in evaluation is reached where it's clear that the
option in question doesn't exist. This means that this patch doesn't
change the logic of the module system, it only provides a more detailed
error in certain cases:

  The option `[definition 1-entry 1].foo' defined in `<expr.nix>' does not exist.

  However it seems as there are no options defined in [definition 1-entry 1]. Are you sure you've
  declared your options properly? This happens if you e.g. declared your options in `types.submodule'
  under `config' rather than `options'.
@Ma27 Ma27 force-pushed the improve-opt-not-defined-error branch from de70b3b to fa30c9a Compare August 18, 2020 13:25
@infinisil infinisil merged commit f8962fd into NixOS:master Aug 18, 2020
@Ma27 Ma27 deleted the improve-opt-not-defined-error branch August 18, 2020 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants