lib/types.nix: listOf.check needs to check the elements #173568

ghost · 2022-05-19T03:21:34Z

Description of changes

Prior to this commit, the check method in the listOf type in lib/types.nix would accept any list, even if the list was not in fact a "list of" the specified type. This meant that for checking purposes, listOf bool and listOf int were the same thing.

This commit fixes that problem.

If the behavior above was in fact deliberate (which I would find surprising), please explain the reasoning to me and I will amend this commit to instead insert a comment explaining why this is the case.

This PR also makes a minor adjustment to the type declaration for meta.license. Previously this field could be a list of licenses or a list of str, but not a heterogeneous list with some licenses and some strs. Since many packages are in fact using heterogeneous mixed lists of license-and-str, and the old types.nix wasn't checking the old type declaration, I have updated the declaration to agree with the current usage.

Things done

Built on platform(s)
- x86_64-linux
- aarch64-linux
Fits CONTRIBUTING.md.

Prior to this commit, the `check` method in the `listOf` type in `lib/types.nix` would accept *any* list, even if the list was not in fact a "list of" the specified type. This meant that for checking purposes, `listOf bool` and `listOf int` were the same thing. This commit fixes that problem.

infinisil · 2022-05-19T09:04:51Z

This check indeed isn't done like this, because it's too strict. Here's an example of where it breaks:

((import ./lib).evalModules {
  modules = [
    ({ lib, ... }: {

      options.foo = lib.mkOption {
        type = lib.types.listOf lib.types.str;
      };

      config.foo = [
        (lib.mkIf true "test")
      ];
    })
  ];
}).config

Before this PR running nix-instantiate --eval --strict on this file gives

{ foo = [ "test" ]; }

But with this PR it fails:

error: A definition for option `foo' is not of type `list of strings'. Definition values:
       - In `<unknown-file>':
           [
             {
               _type = "if";
               condition = true;
               content = "test";
           ...

I'm a bit surprised that no test in lib/tests/modules.sh catches that. There should probably be one for this.

infinisil · 2022-05-19T09:05:33Z

Btw there's another PR that wanted to do the same, but with a different motivation and an alternate solution: #168295

ckiee · 2022-05-19T10:08:19Z

This check indeed isn't done like this, because it's too strict. Here's an example of where it breaks:

That doesn't seem that hard to work around:

      check = v: isList v && all (e: elemType.check e || e ? _type ) v;

Of course it's not perfect but it's better, though ideally it'd be done after we {pushDown,discharge}Properties. It looks like we can't easily move the checking around though and making it work would probably require two stages of checks which sounds overly complex, but just food for thought.

I'm a bit surprised that no test in lib/tests/modules.sh catches that. There should probably be one for this.

+1

infinisil · 2022-05-19T10:23:54Z

I need to point out that in the module system, the recursive check is already done using mergeDefinitions, and that's also usable outside of it, e.g.:

❯ nix repl
Welcome to Nix 2.8.1. Type :? for help.

nix-repl> :l <nixpkgs/lib>
Added 402 variables.

nix-repl> defs = [ { value = [ (mkIf true "test") ]; file = "default.nix"; } ]                                                        

nix-repl> merged = mergeDefinitions [] (types.listOf types.str) defs

nix-repl> :p merged.mergedValue
[ "test" ]

nix-repl>

This then also comes with good error messages.

ckiee · 2022-05-19T10:41:51Z

Sure, but if we typecheck after mergeDefinitions then that mergeDefinitions could break if for some reason the user supplies something that can't be merged? (maybe number instead of attrset)

infinisil · 2022-05-19T13:31:05Z

@ckiee Do you have an example? I think the merging mechanism used by the module system is very solid

ckiee · 2022-05-19T14:24:26Z

@infinisil Looking more closely it seems that won't be a problem so I like your approach more than mine now, though it looks like it'd be headache-inducing to implement and verify the API hasn't changed other than the original listOf thing, but maybe that's just me.

ghost · 2022-05-19T22:31:47Z

Btw there's another PR that wanted to do the same, but with a different motivation and an alternate solution: #168295

Thanks for linking to that; that PR is closed and wouldn't pass CI if reopened. It also makes checkMeta on-by-default which is a policy change that I would prefer not get mixed up with the issue I'm addressing here.

ghost · 2022-05-19T22:36:57Z

This check indeed isn't done like this, because it's too strict. Here's an example of where it breaks:

Is there a more minimal example? Perhaps that doesn't use NixOS modules?

I run an entirely nixpkgs-built userspace but do not use NixOS, so I've never dug into NixOS modules.

It seems weird that nixpkgs isn't allowed to check its meta sets properly because of something in a downstream user of nixpkgs (NixOS).

The error message produced by nix-instantiate --eval --strict seems totally justified to me. After all, { _type = "if"; ... } is an attrset, not a string.

That doesn't seem that hard to work around:

      check = v: isList v && all (e: elemType.check e || e ? _type ) v;

Thanks, I have included this in the latest push.

ckiee · 2022-05-19T23:08:06Z

The error message produced by nix-instantiate --eval --strict seems totally justified to me. After all, { _type = "if"; ... } is an attrset, not a string.

Or is it? As above, the result before this PR is a { foo = [ "test" ]; } and with these latest changes you can still plausibly mess up:

((import ./lib).evalModules {
  modules = [
    ({ lib, ... }: {

      options.foo = lib.mkOption {
        type = lib.types.listOf lib.types.str;
      };

      config.foo = [
        (lib.mkIf true "test")
        "this is okay"
        "but the next one won't be:"
        (lib.mkIf true { oopsie = "doopsie"; }) # <-- added this
      ];
    })
  ];
}).config

#ckie@cookiemonster ~/git/nixpkgs -> nix-instantiate --eval --strict types.nix
 error: A definition for option `foo.[definition 1-entry 4]' is not of type `string'. Definition values:
        - In `<unknown-file>':
            {
              oopsie = "doopsie";
            }
#(use '--show-trace' to show detailed location information)

I expected it to evaluate fine and show it in the list but it didn't. I am very confused at why it is now actually evaluating the mkIf. Very weird but I am too tired right now to debug this.

ghost · 2022-05-19T23:32:37Z

{ _type = "if"; ... } is an attrset, not a string.

Or is it?

Yes, according to the nix language specification it is an attrset, not a string.

ckiee · 2022-05-19T23:57:08Z

But it isn't by the end of the evaluation. Then it's either gone or mapped into the inner value.

ghost · 2022-05-20T00:30:45Z

Then it's either gone or mapped into the inner value.

I really have no idea what you're getting at here.

Could you please provide a simpler example that doesn't involve the entire NixOS module system? Or explain why the problem isn't in the NixOS module system?

As I mentioned in the comment at the top of this PR, if somebody can explain to me why types aren't types I'll change this PR to add just a comment explaining the situation. But at the moment I am totally failing to understand why nix types aren't nix types. So I would not know how to even begin writing that comment. Can you point me to some section of the nix language specification that I should re-read?

ghost · 2022-05-20T00:55:08Z

Or is it? As above, the result before this PR is a { foo = [ "test" ]; } and with these latest changes you can still plausibly mess up:

I tried that test case on master (i.e. without this PR) and got the same thing as with this PR.

ghost · 2022-05-20T00:57:02Z

But with this PR it fails:

error: A definition for option `foo' is not of type `list of strings'. Definition values:
       - In `<unknown-file>':
           [
             {
               _type = "if";
               condition = true;
               content = "test";
           ...

Please see the latest push, with which this test passes.

I have tried all the test cases mentioned so far in this PR and get the same results with or without this PR (making sure to include the most recent commit).

ckiee · 2022-06-03T13:55:43Z

I think this diff is reasonable now. Can you squash and fix the merge conflict?
(Oh and, maybe put the commits changing various invalid definitions in a separate treewide: ... commit)

infinisil

Strong 👎, this PR is not acceptable like this. As already explained in #173568 (comment), the module system does check listOf's fully, and that's the way in which listOf is intended to be used. So if you want meta to be fully checked you should make it use mergeDefinitions.

ckiee · 2022-06-03T16:23:12Z

the module system does check listOf's fully, and that's the way in which listOf is intended to be used. So if you want meta to be fully checked you should make it use mergeDefinitions.

Oh, for some reason I was convinced this doesn't error currently, but it does (on recent nixos-unstable and the commit this PR is based on)

((import ./lib).evalModules {
  modules = [
    ({ lib, ... }: {
 
      options.foo = lib.mkOption {
        type = lib.types.listOf lib.types.str;
      };
 
      config.foo = [
        (lib.mkIf true 123)
        # or: 123
      ];
    })
  ];
}).config

error: A definition for option `foo.[definition 1-entry 1]' is not of type `string'. Definition values:
       - In `<unknown-file>': 123

So unless I am missing something very obvious I don't see why this PR got this far at all?

roberth · 2022-06-03T17:14:19Z

This is all very unfortunate.

The non-obvious requirement here is that the module system needs to be close to as lazy as possible; otherwise some configuration logic becomes inexpressible. The types interface was written with this in mind. Values can only be checked at the last moment; otherwise they cause parts of their input to be evaluated too soon. In effect, this means that the checking must be driven by the merge functions.

I also would like to see a more useful check, but forking the types is not the way to achieve this. We should try to avoid burdening users with laziness trade-offs. Most will think "strict validation is good", despite the fact that the lazy choice will give the same error messages, while supporting more configuration logic than a solution which forces checks too early. Those checks would have been performed anyway.

I can't dismiss the possibility of improving the module-and-type system, but the requirements are be tight:

performance can't suffer
the module system remains as lazy as it is
no new types
check does something useful for non-module-system use

An avenue that's worth exploring is to make check return a value that validates itself as its thunks are evaluated. Something to the effect of { ok = isAttrs v; value = mapAttrs (k: v': ..... (elemType.check v')) v; }. I'm afraid that performance will suffer too much though.

Perhaps another possibility is adding a new attribute to the types, specifically for strict and deep checking. This will have no use in the module system, but could be a little bit faster than using the merge functions. We'll want to make mkOption provide default implementations, and/or this will be a maintenance burden.

Even if improving check is non-trivial, this is the direction that should be taken. Adding more variations on types is not worth the complexity, especially as a solution is already available in the form of merge.

roberth · 2022-06-03T17:19:38Z

lib/types.nix

+    # NixOS apparently has a different notion of "type" than nix(lang)
+    # does; see discussion in GH #173568


Is this the type of comment you would like to have in your code base?
It is accusatory and it lacks real meaning. Can't even paste a URL. A few extra words could have fixed that. (and a lazy Ctrl+L Ctrl+C to copy the whole url)

(I'll shut up now, but I figured this is useful feedback for your future PRs)

I would be happy to include complete URLs, and will do so in the future. It wasn't laziness. Some people get cranky about big long URLs messing up the line-wrapping in multi-line comments. Other people insist on prefixing issue numbers to identify the bugtracker (BZ for Bugzilla, etc) in case the project migrates from one to another in the future. Personally, I don't have strong feelings about any of these things, and just go along with the prevailing trend. Now that I know, my future PRs which reference github issues from a comment will use a complete URL.

Regarding the part of the comment before the question mark:

Is this the type of comment you would like to have in your code base?

It is highly non-obvious that types.nix uses a different definition of "list of <type>" than the Nix language definition does. I do think that non-obvious departures should be documented with comments. Is it documented somewhere else?

ghost · 2022-06-05T07:16:43Z

The non-obvious requirement here is that the module system needs to be close to as lazy as possible; otherwise some configuration logic becomes inexpressible. The types interface was written with this in mind.

Thank you, @roberth, for taking the time to explain your objection. I appreciate that.

ghost · 2022-06-05T07:43:35Z

So unless I am missing something very obvious I don't see why this PR got this far at all?

The main motivation for this PR was checking the meta blocks, where people are currently able to commit things like meta = { maintainers = [ 1 "doorknob" false ]; } without any complaints from OfBorg.

In any case, my takeaway from this whole discussion is approximately: "nixpkgs can't typecheck its meta blocks because of something in nixos", which seems like a layering violation to me... but I've learned to accept that some of those just aren't worth fixing.

Additional context, if helpful: I use "NixpkgsOS" rather than NixOS. Perhaps I lack perspective because of that.

roberth · 2022-06-05T10:49:20Z

"nixpkgs can't typecheck its meta blocks because of something in nixos"

This isn't true. You could use merge and deepSeq to construct a strict check with the module system. merge was mentioned before, so I didn't discuss it in my comment until the mere reference at the end.

which seems like a layering violation to me

I get the scare quotes, but the module system is actually an independent component, independent of NixOS. It's used in many places. The layering works out fine:

┌──────────────────────┐
│Configuration Managers│
└┬─┬─┬─────────────────┘
 │ │┌▽─────────────┐    
 │ ││Nixpkgs       │    
 │ │└┬───────────┬─┘    
 │┌▽─▽──────────┐│      
 ││Module System││      
 │└┬────────────┘│      
┌▽─▽─────────────▽┐     
│rest of lib      │     
└─────────────────┘

where examples of configuration managers are NixOS, nix-darwin, home-manager.

but I've learned to accept that some of those just aren't worth fixing.

Unless you like a challenge!

I use "NixpkgsOS" rather than NixOS. Perhaps I lack perspective because of that.

I don't expect NixOS users (or even contributors I guess) to fully understand the importance of laziness in the module system either.

ghost · 2022-06-06T07:36:11Z

like a layering violation to me... but I've learned to accept that some of those just aren't worth fixing.

Unless you like a challenge!

The costs are often paid in personal friction and generally pissing off of people. I don't rise to those sorts of challenges.

but the module system is actually an independent component, independent of NixOS ... where examples of configuration managers are NixOS, nix-darwin, home-manager.

Thanks for pointing out that home-manager can be used standalone, without NixOS. This gives me a reason to learn about it, and therefore to learn about modules (I don't have any Darwin-capable hardware, so nix-darwin isn't particularly motivating for me).

Somebody should probably update the wiki, which strongly implies that modules are something specific to NixOS:

Modules are files combined by NixOS to produce the full system configuration.

Maybe when I understand them better I will make that update.

The fact that modules is a subdirectory of nixos sends the same message, but it's probably much too late to do anything about that.

ghost requested review from edolstra, nbp and infinisil as code owners May 19, 2022 03:21

ghost mentioned this pull request May 19, 2022

add mechanism for handling meta.sourceProvenance attributes #161098

Merged

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels May 19, 2022

ghost requested review from Ericson2314 and matthewbauer as code owners May 19, 2022 05:15

github-actions bot added the 6.topic: stdenv Standard environment label May 19, 2022

fix meta.* type errors for PR #173568

ebab13d

Implement @ckiee's recommendation #173568 (comment)

61d0c9f

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation labels May 20, 2022

split types.listOf into the "nixos version" and the "nix version"

f979d74

regenerate manual docbook

0c47b6d

infinisil requested changes Jun 3, 2022

View reviewed changes

infinisil added the 6.topic: module system About "NixOS" module system internals label Jun 3, 2022

infinisil requested a review from roberth June 3, 2022 16:14

roberth closed this Jun 3, 2022

roberth reviewed Jun 3, 2022

View reviewed changes

ghost deleted the lib-types-listOf branch June 5, 2022 07:21

roberth mentioned this pull request Jun 5, 2022

lib.types.*.check: return a message #154178

Open

ghost mentioned this pull request Sep 15, 2022

Module system improvements #6794

Merged

Artturin mentioned this pull request Jan 29, 2024

List of @amjoseph-nixpkgs Pull requests before account removal #284796

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/types.nix: listOf.check needs to check the elements #173568

lib/types.nix: listOf.check needs to check the elements #173568

ghost commented May 19, 2022 •

edited by ghost

Loading

infinisil commented May 19, 2022

infinisil commented May 19, 2022

ckiee commented May 19, 2022

infinisil commented May 19, 2022

ckiee commented May 19, 2022

infinisil commented May 19, 2022

ckiee commented May 19, 2022

ghost commented May 19, 2022

ghost commented May 19, 2022 •

edited by ghost

Loading

ckiee commented May 19, 2022 •

edited

Loading

ghost commented May 19, 2022

ckiee commented May 19, 2022

ghost commented May 20, 2022 •

edited by ghost

Loading

ghost commented May 20, 2022

ghost commented May 20, 2022

ckiee commented Jun 3, 2022 •

edited

Loading

infinisil left a comment

ckiee commented Jun 3, 2022

roberth commented Jun 3, 2022

roberth Jun 3, 2022

ghost Jun 5, 2022 •

edited by ghost

Loading

ghost commented Jun 5, 2022 •

edited by ghost

Loading

ghost commented Jun 5, 2022 •

edited by ghost

Loading

roberth commented Jun 5, 2022

ghost commented Jun 6, 2022

		# NixOS apparently has a different notion of "type" than nix(lang)
		# does; see discussion in GH #173568

lib/types.nix: listOf.check needs to check the elements #173568

lib/types.nix: listOf.check needs to check the elements #173568

Conversation

ghost commented May 19, 2022 • edited by ghost Loading

Description of changes

Things done

infinisil commented May 19, 2022

infinisil commented May 19, 2022

ckiee commented May 19, 2022

infinisil commented May 19, 2022

ckiee commented May 19, 2022

infinisil commented May 19, 2022

ckiee commented May 19, 2022

ghost commented May 19, 2022

ghost commented May 19, 2022 • edited by ghost Loading

ckiee commented May 19, 2022 • edited Loading

ghost commented May 19, 2022

ckiee commented May 19, 2022

ghost commented May 20, 2022 • edited by ghost Loading

ghost commented May 20, 2022

ghost commented May 20, 2022

ckiee commented Jun 3, 2022 • edited Loading

infinisil left a comment

Choose a reason for hiding this comment

ckiee commented Jun 3, 2022

roberth commented Jun 3, 2022

roberth Jun 3, 2022

Choose a reason for hiding this comment

ghost Jun 5, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Jun 5, 2022 • edited by ghost Loading

ghost commented Jun 5, 2022 • edited by ghost Loading

roberth commented Jun 5, 2022

ghost commented Jun 6, 2022

ghost commented May 19, 2022 •

edited by ghost

Loading

ghost commented May 19, 2022 •

edited by ghost

Loading

ckiee commented May 19, 2022 •

edited

Loading

ghost commented May 20, 2022 •

edited by ghost

Loading

ckiee commented Jun 3, 2022 •

edited

Loading

ghost Jun 5, 2022 •

edited by ghost

Loading

ghost commented Jun 5, 2022 •

edited by ghost

Loading

ghost commented Jun 5, 2022 •

edited by ghost

Loading