I also have an optimization that I'm not totally sure about. In the case that nulls.right is nonempty (meaning that some of the values are null), you do:

  valueMeta = optionalAttrs (nulls.wrong != [ ]) checkedAndMerged.valueMeta;

But if nulls.wrong != [], then we're going to get an error. [...]
So I think we can set valueMeta = { } here, and behavior will be unchanged.

I'm also not 100% on that. valueMeta should (typically) only be evaluated when introspecting options, or extending them in non-standard ways.

IIUC, it's supposed to work even if there are (unrelated) errors.

Originally, checkedAndMerged was checking & merging nulls.wrong, so nulls were already filtered out. Meaning it was possible for code that didn't care about our mixed-null-non-null assertion to still read what the elemType would've returned for valueMeta.

If that has a large performance cost, it may not be worth it; so you're optimisation may be acceptable. Ideally we'd get @hsjobeki or @roberth's input on idiomatic valueMeta in a partial/unrelated error scenario.

Looking at how it simplifies the overall diff, I personally think it's fine.

lib.types: don't take loc parameter in checkDefsForError

I'd prefer we didn't increase the scope of my PR to include this. Do you mind moving that to a dedicated PR against nixpkgs?

- throwIf (headError.causedByMixedNulls or false) headError.message value;
+ if (headError.causedByMixedNulls or false) then throw headError.message else value;

Does this actually have a significant performance impact? I strongly prefer idiomatic usage of helpers like throwIf to keep the code more readable. Seems like we'd be saving at most O(n) function applications? This codepath is also only evaluated when nullOr is wrapped by a non-v2-aware wrapper type.

also note that PR is giving me an error for evaluating my personal config, because of this bit of code in display-managers/default.nix:

      defaultSession = lib.mkOption {
        type = lib.types.nullOr lib.types.str // {
          description = "session name";
          check =
            d:
            lib.assertMsg (d != null -> (lib.types.str.check d && lib.elem d cfg.sessionData.sessionNames)) ''
              Default graphical session, '${d}', not found.
              Valid names for 'services.displayManager.defaultSession' are:
                ${lib.concatStringsSep "\n  " cfg.sessionData.sessionNames}
            '';
        };
        default = null;
        example = "gnome";
        description = ''
          Graphical session to pre-select in the session chooser (only effective for GDM, LightDM and SDDM).

          On GDM, LightDM and SDDM, it will also be used as a session for auto-login.

          Set this option to empty string to get an error with a list of currently available sessions.
        '';
      };

so we may want to fix instances of nullOr being merged before merging this.

also was just about to do a real-time test on my own config, but found that this PR is giving me an error, because of this bit of code in display-managers/default.nix:

      defaultSession = lib.mkOption {
        type = lib.types.nullOr lib.types.str // {
          description = "session name";
          check =
            d:
            lib.assertMsg (d != null -> (lib.types.str.check d && lib.elem d cfg.sessionData.sessionNames)) ''
              Default graphical session, '${d}', not found.
              Valid names for 'services.displayManager.defaultSession' are:
                ${lib.concatStringsSep "\n  " cfg.sessionData.sessionNames}
            '';
        };
        default = null;
        example = "gnome";
        description = ''
          Graphical session to pre-select in the session chooser (only effective for GDM, LightDM and SDDM).

          On GDM, LightDM and SDDM, it will also be used as a session for auto-login.

          Set this option to empty string to get an error with a list of currently available sessions.
        '';
      };

so we may want to fix instances of nullOr being merged before merging this.

so we may want to fix instances of nullOr being merged before merging this.

Interesting. Is there a test I can run that covers that?

By "merged" do you mean the // { description, check } update? Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge. IIRC there is a check somewhere that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example? (Report error for unsupported t // { check = ...; })

so we may want to fix instances of nullOr being merged before merging this.

Interesting. Is there a test I can run that covers that? By "merged" do you mean the // { description, check } update?

Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge. IIRC there is a check somewhere that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example? (Report error for unsupported t // { check = ...; })

so we may want to fix instances of nullOr being merged before merging this.

Interesting. Is there a test I can run that covers that? By "merged" do you mean the // { description, check } update?

Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge.
There is (Report error for unsupported t // { check = ...; }) that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example?

so we may want to fix instances of nullOr being merged before merging this.

Interesting. Is there a test I can run that covers that? By "merged" do you mean the // { description, check } update?

Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge.
There is report error for unsupported t // { check = ...; } that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example?

Yeah, I agree on hot-code. But on "normal" code, I tend to favor legibility over mirco-optimisation.

Based on roberth's comment on your linked issue I'll keep your version, though:

I support removing these calls from lib and other hot paths.

oh, i should've sent the error - yeah that's the error that triggered

 error: The option `services.displayManager.defaultSession' has a type `session name' that uses
       an ad-hoc `type // { check = ...; }' override, which is incompatible with
       the v2 merge mechanism.

       Please use `lib.types.addCheck` instead of `type // { check }' to add
       custom validation. For example:

         lib.types.addCheck baseType (value: /* your check */)

       instead of:

         baseType // { check = value: /* your check */; }

       Alternatively, this message may also occur as false positive when mixing Nixpkgs
       versions, if one Nixpkgs is between 83fed2e6..58696117 (Aug 28 - Oct 28 2025)

is there any reason why this check would pass?

nix-repl> (lib.types.listOf (lib.types.enum [])).check [ "hello" ]
true

(this is a minimal example but the original type is also just an enum, and the merging doesn't throw any errors at all)

i found this issue but i don't think that's related since it seems to be submodule- and oneOf-specific?

is there any reason why this check would pass?

nix-repl> (lib.types.listOf (lib.types.enum [])).check [ "hello" ]
true

(this is a minimal example but the original type is also just an enum, and the merging doesn't throw any errors at all)

(also sorry if this isn't the place to ask noobish question like this, it seems like mostly technical discussion ;-;)
i found this issue but i don't think that's related since it seems to be submodule- and oneOf-specific?

is there any reason why this check would pass?

nix-repl> (lib.types.listOf (lib.types.enum [])).check [ "hello" ]
true

(this is a minimal example but the original type is also just an enum, and the merging doesn't throw any errors at all)

i found this issue but i don't think that's related since it seems to be submodule- and oneOf-specific?

(also sorry if this isn't the place to ask noobish question like this, it seems like mostly technical discussion ;-;)

types.enum's check is literally x: elem x values, so (lib.types.enum []).check "hello" should fail.

types.listOf's check just uses isList, without also checking all elemType.check. This laziness was introduced in https://github.com/NixOS/nixpkgs/pull/6794