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

Throw an error for options without a type #162271

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Conversation

infinisil
Copy link
Member

Motivation for this change

This is a revised version of #127066, which is a follow-up to #76184. This PR improves the types, error message, documentation and updates it to work with #149532.

Warnings are thrown when building the manual. Only options included in the manual are required to have a type specified. E.g. with this configuration.nix:

{ lib, ... }: {
  options.foo = lib.mkOption {};
  config.documentation.nixos.includeAllModules = true;
}

building it gives you

$ nix-build nixos --arg configuration ./configuration.nix -A config.system.build.manual.manualHTML
building '/nix/store/19s7jd4jxym22pl997070h58h62jawwb-options.json.drv'...
error: option foo has no description
error: option foo has no type. Please specify a valid type,
  see https://nixos.org/manual/nixos/stable/index.html#sec-option-types
Treating warnings as errors. Set documentation.nixos.options.warningsAreErrors to false to ignore these warnings.

Closes #76184
Closes #141730
Closes #141760

Ping @dasJ @domenkozar @roberth @primeos

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:
  • 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 and others added 2 commits February 28, 2022 22:50
Co-Authored-By: Janne Heß <janne@hess.ooo>
Makes all options rendered in the manual throw an error if they don't
have a type specified.

This is a follow-up to NixOS#76184

Co-Authored-By: Silvan Mosberger <contact@infinisil.com>
: The type of the option (see [](#sec-option-types)). This
argument is mandatory for nixpkgs modules. Setting this is highly
recommended for the sake of documentation and type checking. In case it is
not set, a fallback type with unspecified behavior is used.
Copy link
Member

Choose a reason for hiding this comment

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

unspecified behavior

Wait what? Surely we can specify what the type is, or will be?

Copy link
Member Author

@infinisil infinisil Mar 1, 2022

Choose a reason for hiding this comment

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

I chose this wording because the current default type, types.unspecified, uses a merge function that's really not safe and fairly arbitrary:

  • Booleans are merged using or: true + false -> true
  • Lists and strings are just concatenated together, which can be unsafe and makes the result depend on module order. Also relates to the deprecation of types.string
  • Attribute sets are merged using lib.mergeAttrs, which overrides properties depending on module order
  • Nested mkIf's, mkMerge's and co. are not taken into account.

Not only does this align with the type name, but this behavior really wasn't ever specified to behave in this way, thus unspecified. This is also reflected in how types.unspecified is undocumented. I believe originally it was only meant to be used internally to signify an unspecified type.

In the future I believe we should make types.anything be the default type instead, which is a similarly generic type but without any of the above flaws. By saying the type is unspecified we leave the possibility open for changing it in the future without breaking people's expectations. If we do the switch to types.anything we can update this documentation to be more specific and strict.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I can get on board with that.
Instead of anything I'd go with raw though, as it does not sneak extra arbitrary decisions into underspecified code. anything may be better than unspecified, but it is still arbitrary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yeah, maybe raw would be better, I can see a case to be made for either, though that's a discussion for the future.

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.

lgtm, but can't vouch for the changed types in the NixOS modules

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-25/18003/1

@roberth roberth merged commit 0395086 into NixOS:master Mar 16, 2022
@infinisil infinisil deleted the warn-no-type branch March 18, 2022 20:37
vcunat added a commit to vcunat/nixpkgs that referenced this pull request Mar 19, 2022
For now at least.  I expect someone will find a working type later.
It's incorrect and was causing bad issues.  Example test case:
nix-instantiate nixos/release.nix -A tests.xfce.x86_64-linux --dry-run

This is a partial revert of commit b2d803c from PR NixOS#162271.
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.

All options should have types
6 participants