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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/options.nix
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ rec {
then true
else opt.visible or true;
readOnly = opt.readOnly or false;
type = opt.type.description or null;
type = opt.type.description or "unspecified";
}
// optionalAttrs (opt ? example) { example = scrubOptionValue opt.example; }
// optionalAttrs (opt ? default) { default = scrubOptionValue opt.default; }
Expand Down
7 changes: 4 additions & 3 deletions nixos/doc/manual/development/option-declarations.section.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ The function `mkOption` accepts the following arguments.

`type`

: The type of the option (see [](#sec-option-types)). It may be
omitted, but that's not advisable since it may lead to errors that
are hard to diagnose.
: 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.


`default`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ options = {
<listitem>
<para>
The type of the option (see
<xref linkend="sec-option-types" />). It may be omitted, but
that’s not advisable since it may lead to errors that are hard
to diagnose.
<xref linkend="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.
</para>
</listitem>
</varlistentry>
Expand Down
9 changes: 8 additions & 1 deletion nixos/lib/make-options-doc/mergeJSON.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,21 @@ def unpivot(options: Dict[Key, Option]) -> Dict[str, JSON]:
elif ov is not None or cur.get(ok, None) is None:
cur[ok] = ov

severity = "error" if warningsAreErrors else "warning"

# check that every option has a description
hasWarnings = False
for (k, v) in options.items():
if v.value.get('description', None) is None:
severity = "error" if warningsAreErrors else "warning"
hasWarnings = True
print(f"\x1b[1;31m{severity}: option {v.name} has no description\x1b[0m", file=sys.stderr)
v.value['description'] = "This option has no description."
if v.value.get('type', "unspecified") == "unspecified":
hasWarnings = True
print(
f"\x1b[1;31m{severity}: option {v.name} has no type. Please specify a valid type, see " +
"https://nixos.org/manual/nixos/stable/index.html#sec-option-types\x1b[0m", file=sys.stderr)

if hasWarnings and warningsAreErrors:
print(
"\x1b[1;31m" +
Expand Down
20 changes: 8 additions & 12 deletions nixos/modules/services/networking/nsd.nix
Original file line number Diff line number Diff line change
Expand Up @@ -194,19 +194,8 @@ let
zone.children
);

# fighting infinite recursion
zoneOptions = zoneOptionsRaw // childConfig zoneOptions1 true;
zoneOptions1 = zoneOptionsRaw // childConfig zoneOptions2 false;
zoneOptions2 = zoneOptionsRaw // childConfig zoneOptions3 false;
zoneOptions3 = zoneOptionsRaw // childConfig zoneOptions4 false;
zoneOptions4 = zoneOptionsRaw // childConfig zoneOptions5 false;
zoneOptions5 = zoneOptionsRaw // childConfig zoneOptions6 false;
zoneOptions6 = zoneOptionsRaw // childConfig null false;

childConfig = x: v: { options.children = { type = types.attrsOf x; visible = v; }; };

# options are ordered alphanumerically
zoneOptionsRaw = types.submodule {
zoneOptions = types.submodule {
options = {

allowAXFRFallback = mkOption {
Expand Down Expand Up @@ -246,6 +235,13 @@ let
};

children = mkOption {
# TODO: This relies on the fact that `types.anything` doesn't set any
# values of its own to any defaults, because in the above zoneConfigs',
# values from children override ones from parents, but only if the
# attributes are defined. Because of this, we can't replace the element
# type here with `zoneConfigs`, since that would set all the attributes
# to default values, breaking the parent inheriting function.
type = types.attrsOf types.anything;
default = {};
description = ''
Children zones inherit all options of their parents. Attributes
Expand Down
1 change: 1 addition & 0 deletions nixos/modules/services/networking/unbound.nix
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ in {
};

stateDir = mkOption {
type = types.path;
default = "/var/lib/unbound";
description = "Directory holding all state for unbound to run.";
};
Expand Down
1 change: 1 addition & 0 deletions nixos/modules/services/networking/vsftpd.nix
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ in

userlist = mkOption {
default = [];
type = types.listOf types.str;
description = "See <option>userlistFile</option>.";
};

Expand Down
18 changes: 18 additions & 0 deletions nixos/modules/services/x11/display-managers/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,24 @@ in

session = mkOption {
default = [];
type = with types; listOf (submodule ({ ... }: {
options = {
manage = mkOption {
description = "Whether this is a desktop or a window manager";
type = enum [ "desktop" "window" ];
};

name = mkOption {
description = "Name of this session";
type = str;
};

start = mkOption {
description = "Commands to run to start this session";
type = lines;
};
};
}));
example = literalExpression
''
[ { manage = "desktop";
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/system/boot/kernel.nix
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ in

boot.kernelPackages = mkOption {
default = pkgs.linuxPackages;
type = types.unspecified // { merge = mergeEqualOption; };
type = types.raw;
apply = kernelPackages: kernelPackages.extend (self: super: {
kernel = super.kernel.override (originalArgs: {
inherit randstructSeed;
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/system/boot/stage-1.nix
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ in
else "gzip"
);
defaultText = literalDocBook "<literal>zstd</literal> if the kernel supports it (5.9+), <literal>gzip</literal> if not";
type = types.unspecified; # We don't have a function type...
type = types.either types.str (types.functionTo types.str);
description = ''
The compressor to use on the initrd image. May be any of:

Expand Down