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: better error for invalid option declarations #222516

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

ncfavier
Copy link
Member

Make byName aware of whether it's processing options or config to give slightly more accurate error messages.

This presumably would have been useful in nix-community/home-manager#3793 (comment), cc @DamienCassou

with import ./lib;
(evalModules {
  modules = [ {
    options = {
      imports = [ {} ];
    };
  } ];
}).config
error: An option declaration for `imports' has type
       `list' rather than an attribute set.
       Did you mean to define this outside of `options'?

@ncfavier ncfavier requested a review from roberth March 22, 2023 10:29
@ncfavier ncfavier force-pushed the options-better-error branch from 9dcbf6c to 5e02ee8 Compare March 22, 2023 11:14
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find!

lib/modules.nix Outdated
@@ -563,32 +565,36 @@ rec {
qux = [ "module.hidden=baz,value=bar" "module.hidden=fli,value=gne" ];
}
*/
byName = attr: f: modules:
byName = isConfig: attr: f: modules:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me doubt that byName would be a good abstraction.
Inlining byName will likely make the code easier to read and remove a bunch of function calls from the hot path.

Alternatively, you could use attr == "config" instead of introducing another lambda. That way at least performance is unaffected by this PR; not introducing a lambda and corresponding calls.

Make `byName` aware of whether it's processing options or config to give
slightly more accurate error messages.
@ncfavier ncfavier force-pushed the options-better-error branch from 5e02ee8 to 8751764 Compare March 22, 2023 11:37
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 22, 2023
@ncfavier ncfavier merged commit 0d5c1ac into NixOS:master Apr 4, 2023
@ncfavier ncfavier deleted the options-better-error branch April 4, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants