Skip to content

fix include-less parametric in provides#208

Merged
vic merged 3 commits intodenful:mainfrom
musjj:provides-parameter
Mar 1, 2026
Merged

fix include-less parametric in provides#208
vic merged 3 commits intodenful:mainfrom
musjj:provides-parameter

Conversation

@musjj
Copy link
Copy Markdown
Contributor

@musjj musjj commented Mar 1, 2026

Fix: #207

Also added a test to catch this.

I hope this isn't just peppering over a deeper underlying issue.

@musjj
Copy link
Copy Markdown
Contributor Author

musjj commented Mar 1, 2026

Never mind, this fails with:

ns.foo = den.lib.parametric { };
ns.bar.provides.baz = den.lib.parametric { };
ns.a = den.lib.parametric {
  provides.b = den.lib.parametric { };
  provides.c = den.lib.parametric { nixos.networking.hostName = "pinguino"; };
  provides.d = den.lib.parametric {
    provides.e = den.lib.parametric { };
  };
};

den.aspects.igloo.includes = [
  ns.foo
  ns.bar._.baz
  ns.a
  ns.a._.b
  ns.a._.c
  ns.a._.d._.e
];

This errors with:

error: attribute '_' missing
at /nix/store/zs759xrvfvkk0aki4mpbdprshnjcmfc1-source/templates/ci/modules/features/provides-parametric.nix:34:11:
    33|           ns.a._.c
    34|           ns.a._.d._.e
      |           ^
    35|         ];

Looks like this is relevant: #113

@vic
Copy link
Copy Markdown
Member

vic commented Mar 1, 2026

top level aspects always have an initial includes = [] because they are coerced by the Nix module type defined at flake-aspect's types.nix. see https://github.com/vic/flake-aspects/blob/main/nix/types.nix#L81 , however doing parametric { } the argument to parametric here is a plain empty attr-set that was not coerced by the type-system.

@musjj
Copy link
Copy Markdown
Contributor Author

musjj commented Mar 1, 2026

Hmm, interesting. Do you have any idea why the provides is yeeted out of the doubly-nested aspect?

ns.a = den.lib.parametric {
  provides.d = den.lib.parametric {
    provides.e = den.lib.parametric { };
  };
};

ns.a._.d works here, but ns.a._.d._.e fails.

Inspecting ns.a._.d reveals that it only has __functionArgs and __functor.

provides/_ is entirely gone. Any idea why?

@vic
Copy link
Copy Markdown
Member

vic commented Mar 1, 2026

Inspecting ns.a._.d reveals that it only has __functionArgs and __functor.

hm, the parametric.* function family should only change __functor and nothing more, maybe we need tests that ensure this, if you look at the implementation the reuse the aspect given to them and just change __functor. There's lib.owned that does remove __functor and includes. Maybe we need unit tests for them at ~/checkmate (as opposed to integration tests at templates/ci)

@vic
Copy link
Copy Markdown
Member

vic commented Mar 1, 2026

them having __functionArgs looks like already touched by Nix in some way. because we never add that.

@musjj
Copy link
Copy Markdown
Contributor Author

musjj commented Mar 1, 2026

It's definitely not caused by the parametric functions. I tried debugging it like this:

parametric.__functor =
  _: aspect:
  let
    result = parametric.withOwn parametric.atLeast aspect;
  in
  (builtins.trace (builtins.attrNames result) result);

It prints:

trace: [ "__functor" "provides" ]

So the provides is still intact.

I suspect that there was something wrong with the provides type: https://github.com/vic/flake-aspects/blob/88bac0fd89f3d9ce8f157debc57875ca470535e8/nix/types.nix#L84-L94, but it looks ok to me.

@musjj
Copy link
Copy Markdown
Contributor Author

musjj commented Mar 1, 2026

Ok, I narrowed down the problem to the nested provide type being incorrectly coerced to curriedProviderFn. This is a very incomplete fix, but changing like this:

-curriedProviderFn = lib.types.functionTo providerType;
+curriedProviderFn = lib.types.addCheck (lib.types.functionTo providerType) (f: !(f ? provides));

Fixes the issue. Not sure what a more robust check should look like.

@vic
Copy link
Copy Markdown
Member

vic commented Mar 1, 2026

Right, looks like flake-aspects PR patch then, let's have tests there first.

@musjj
Copy link
Copy Markdown
Contributor Author

musjj commented Mar 1, 2026

I tried to fix this here: denful/flake-aspects#36

@vic
Copy link
Copy Markdown
Member

vic commented Mar 1, 2026

Merged your flake-aspects PR, do you think we need anything on Den itself ?

@musjj
Copy link
Copy Markdown
Contributor Author

musjj commented Mar 1, 2026

Yeah the self.includes or [ ] is still needed. Also the parametric test is probably still useful to have here.

@vic vic added the allow-ci allow all CI integration tests label Mar 1, 2026
@vic
Copy link
Copy Markdown
Member

vic commented Mar 1, 2026

You are awesome, thanks for fixing all of this, and getting your feet wet on Den and flake-aspects internals :)

@vic vic merged commit 3d9be07 into denful:main Mar 1, 2026
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

allow-ci allow all CI integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Nested provides must have includes

2 participants