Skip to content
Closed
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
6 changes: 6 additions & 0 deletions doc/release-notes/rl-2511.section.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,13 @@
### Deprecations {#sec-nixpkgs-release-25.11-lib-deprecations}

- Create the first release note entry in this section!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Create the first release note entry in this section!

- `types.either` silently accepted mismatching types when used in `freeformType`. Module maintainers should fix the used type
In most cases wrapping `either` with `attrsOf` should be sufficient.

Since types.either was used to bootstrap other types. This also affects the following types:
- `oneOf`
- `number`
- `numbers.*`

### Additions and Improvements {#sec-nixpkgs-release-25.11-lib-additions-improvements}

Expand Down
4 changes: 4 additions & 0 deletions lib/tests/modules.sh
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,10 @@ checkConfigError 'A definition for option .* is not of type.*between.*-21 and 43
# types.either
checkConfigOutput '^42$' config.value ./declare-either.nix ./define-value-int-positive.nix
checkConfigOutput '^"24"$' config.value ./declare-either.nix ./define-value-string.nix

# Check regression detected in https://github.com/NixOS/nixpkgs/issues/438459
# Can be removed after 26.05 release.
checkConfigOutput '^"bar"$' config.foo ./freeform-either.nix
# types.oneOf
checkConfigOutput '^42$' config.value ./declare-oneOf.nix ./define-value-int-positive.nix
checkConfigOutput '^\[\]$' config.value ./declare-oneOf.nix ./define-value-list.nix
Expand Down
7 changes: 7 additions & 0 deletions lib/tests/modules/freeform-either.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{ lib, ... }:
{
# Proper type would be attrsOf (either int str)
# This checks backwards compatibility for the incorrect usage of either
freeformType = lib.types.either lib.types.int lib.types.str;
foo = "bar";
}
32 changes: 26 additions & 6 deletions lib/types.nix
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ let
in
if invalidDefs != [ ] then { message = "Definition values: ${showDefs invalidDefs}"; } else null;

# Compat helper for merge to safely access the .value of merge.v2
Copy link
Member

Choose a reason for hiding this comment

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

The old type interface also allowed the merged value to be accessed without checking the definitions. It was the module system's responsibility to throw the definition errors, so the caller's responsibility.
Haven't we preserved that pattern for checkAndMerge?
Calling this a compat helper suggests that v1 didn't behave that way, but it did.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, did some more analysis and I think we should consider this to be two problems at once, the combination of which allowed this to go undetected for a while.
Both should be solved.

  • either had a magical last case that just returned an unchecked value. This seems very wrong to me, and we don't do this in v2. We should probably dig into its history but it seems like a fair assumption that it should throw an error there, and that error should include something about wrong use of the type interface.

  • freeformType didn't check or check headError

I've started an alternate fix in #439189 with fewer workarounds. Ideally all workarounds are easy to clean up and we don't add workarounds that other code may accidentally start to rely on.
Both PRs need more tests at the moment :)

checkHeadError =
loc: descr: checkedAndMerged:
if checkedAndMerged.headError or null != null then
throw "A definition for option `${showOption loc}' is not of type `${descr}'. TypeError: ${checkedAndMerged.headError.message}"
Copy link
Member

Choose a reason for hiding this comment

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

This has a fictional identifier "TypeError", which could be confusing.
Given the current way we write the message string in checkDefsForError, I think this looks best.

Suggested change
throw "A definition for option `${showOption loc}' is not of type `${descr}'. TypeError: ${checkedAndMerged.headError.message}"
throw "A definition for option `${showOption loc}' is not of type `${descr}'. ${checkedAndMerged.headError.message}"

else
checkedAndMerged.value;

outer_types = rec {
isType = type: x: (x._type or "") == type;

Expand Down Expand Up @@ -715,7 +723,7 @@ let
merge = {
__functor =
self: loc: defs:
(self.v2 { inherit loc defs; }).value;
checkHeadError loc description (self.v2 { inherit loc defs; });
Copy link
Member

Choose a reason for hiding this comment

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

These could turn out to be a problem, as they don't follow the original interface.

v2 =
{ loc, defs }:
let
Expand Down Expand Up @@ -828,7 +836,7 @@ let
merge = {
__functor =
self: loc: defs:
(self.v2 { inherit loc defs; }).value;
checkHeadError loc description (self.v2 { inherit loc defs; });
v2 =
{ loc, defs }:
let
Expand Down Expand Up @@ -1252,7 +1260,7 @@ let
merge = {
__functor =
self: loc: defs:
(self.v2 { inherit loc defs; }).value;
checkHeadError loc description (self.v2 { inherit loc defs; });
v2 =
{ loc, defs }:
let
Expand Down Expand Up @@ -1417,7 +1425,16 @@ let
merge = {
__functor =
self: loc: defs:
(self.v2 { inherit loc defs; }).value;
let
cm = (self.v2 { inherit loc defs; });
in
if cm.headError ? fallback then
lib.warn (
cm.headError.message
+ "\nIf you are the module maintainer, please fix the type. This will be turned into an error after 26.05"
) cm.headError.fallback
else
checkHeadError loc description cm;
v2 =
{ loc, defs }:
let
Expand Down Expand Up @@ -1452,6 +1469,9 @@ let
};
headError = {
message = "The option `${showOption loc}` is neither a value of type `${t1.description}` nor `${t2.description}`, Definition values: ${showDefs defs}";
# Specific for either, to ease migrations
# until 26.05 this is a warning, afterwards it will be removed
fallback = mergeOneOption loc defs;
};
value = abort "(t.merge.v2 defs).value must only be accessed when `.headError == null`. This is a bug in code that consumes a module system type.";
};
Expand Down Expand Up @@ -1501,7 +1521,7 @@ let
merge = {
__functor =
self: loc: defs:
(self.v2 { inherit loc defs; }).value;
checkHeadError loc description (self.v2 { inherit loc defs; });
v2 =
{ loc, defs }:
let
Expand Down Expand Up @@ -1568,7 +1588,7 @@ let
merge = {
__functor =
self: loc: defs:
(self.v2 { inherit loc defs; }).value;
checkHeadError loc elemType.description (self.v2 { inherit loc defs; });
v2 =
{ loc, defs }:
let
Expand Down
Loading