Lazy evaluation of types of `listOf oneOf` causes option missing errors when inner types are submodules

OmnipotentEntity commented 2 years ago

Describe the bug

When attempting to write a module, I had need of a tagged type to disambiguate between a list of strings and a list of strings that contain paths, but when I created this type I ran into an issue where the module's check for stray and useless option after configuration flagged one of my inner tagged types as having an stray and undefined option.

# Example type:
with lib;

foo = types.submodule {
  options = {
    bar = mkOption {
      type = types.str;
    };
  };
};

baz = types.submodule {
  options = {
    words = mkOption {
      quux = (types.nonEmptyListOf types.str);
    };
  };
};

exampleType = types.listOf (types.oneOf [ foo baz ]);

Note if foo is found first in the type definition list of exampleType, then using [ {bar = "/my/path" } { quux = [ "asdf" ] } ] will trigger the error "The option path.to.quux does not exist." On the other hand if baz is found first in the list, then using the same data will trigger the error "The option path.to.bar does not exist."

Steps To Reproduce

Steps to reproduce the behavior:

Create a type that is similar to the code above
Use the type in a mkOption definition
Attempt to use that option

Expected behavior

It would be expected that the type would compose such that both submodules are valid and the option missing errors do not occur.

Additional context

You can find a full example of code that triggers this problem here (It is, however, awful, and I apologize for making you read it.)

Use the following additional module to trigger it:

{config, pkgs, ...} : {
  config.environment.wordlist = {
    enable = true;
    lists = [
      {
        envVar = "WORDLIST";
        source = "${pkgs.scowl}/share/dict/words.txt";
      }
      {
        envVar = "AUGMENTED_WORDLIST";
        source = [
          { file = "${pkgs.scowl}/share/dict/words.txt"; }
          { words = [ "foo" "bar" "baz" ]; }
        ];
      }
    ];
  };
}

Notify maintainers

@Infinisil @Ma27 @edolstra

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 5.15.13, NixOS, 21.11 (Porcupine)`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.5.0pre20211206_d1aaa7e`
 - channels(root): `"nixos-21.05.3957.66d6ec6ed2d, nixos-unstable-21.11pre325810.2deb07f3ac4"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`

GrafBlutwurst commented 2 years ago

I actually stumbled across the same thing yesterday and I think I figured out what the underlying issue is see:

https://discourse.nixos.org/t/problems-with-types-oneof-and-submodules/15197/2?u=grafblutwurst

TL;DR The merge function in oneOf/either checks if all values in the passed defs are of the left type parameter or the right one. But check on submodule only uses builtins.isAttr (and a couple othere ones). This causes eithers merge to always pick the left type parameter if you have two submodules in the type parameters and thus breaking.

Note that my solution at the end is extremely hacky and probably a bad idea, but it was the easiest way i could think off to fix it for what i'm working on right now.

infinisil commented 2 years ago

I think we should close this as wont-fix, because indeed as @GrafBlutwurst already discovered, either/oneOf relies on types .check function to make decisions, which doesn't work great because .check in the current module system is really only a very basic and shallow check meant mainly for triggering early "A definition of option

However, while the module system could use some redesign, and either/oneOf doesn't work great, I believe there's nothing stopping us from having proper ADT's similar to @GrafBlutwurst's proof-of-concept. Previous work is https://github.com/shlevy/nix-adt by @shlevy, but I believe for proper module system integration it needs a bunch of work.

I think to close this issue we should at least document the current limitations of either/oneOf.

GrafBlutwurst commented 2 years ago

I haven't tried it yet, but wouldn't this be fixed to a degree by having submodules check actually trigger checking of the wrapped types? the same caveat would still apply albeit to a smaller degree. that is if your first entry in the oneOf is a structural subtype of a subsequent one it would be falsely picked up.

oneOf could ensure that for a given term exactly one type clause typechecks though that would change its semantics to be disjoint and i that might heavily mess with preexisting usage. It also may get computationally expensive in large definitions

NixOS / nixpkgs