module system: Improve error messages around faulty imports

[roberth]

Description of changes

• Make explicit the module "class", which identifies the module system application, such as "nixos", "darwin" or "homeManager". This allows us to print an appropriate error when importing for example a home-manager module into NixOS:

  The module foo.nix#darwinModules.default was imported into nixos instead of home-manager.

  I do not expect users or module authors to set the class attribute in their modules (although they may). Instead I envision a library like flake-parts to set this for us.

• Use the new class information to automatically import the appropriate default module from a flake.

• Detect other mistakes that involve the importing _type-carrying values. For instance importing a nixosSystem result into NixOps network appears to be a somewhat common mistake, and for good reason. These changes detect this scenario and explain what to do instead.

Depends on NixOS/nix#7186

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[shlevy]

NACK from me. This is non-compositional and bloats the evalModules arguments, there is a much simpler solution #196584 (comment)

[roberth]

This is non-compositional

I'm not sure I follow. You may be referring to either commit.

• If you refer to the introduction and checking of class: The purpose of nominal typing is to prevent bad compositions without having to both the user with irrelevant details.
• If you refer to the treatment of flake attrsets in the module system: I've already proposed an alternative that did not couple with flakes, but that was shot down because allowing arbitrary manipulation of all modules would allow the possibility of behaviors that make modules harder to understand.

bloats the evalModules arguments

I disagree. It is optional and it will only be set in contexts where imports are expected to be used. For example, NixOS sets it only once per system, counting specialisations as systems of course. Submodules that provide the various foo.<name>.{x,y,z} don't need to set it.

class does have some use as a special arg, as it allows conditionals in modules that are otherwise identical between, say, nixos and darwin.

there is a much simpler solution #196584 (comment)

That's unnecessary logic. I'd rather read some extra attribute paths than some custom map call that doesn't transfer between classes.

[shlevy]

I was blurring things together, let me separate them.

Re class, I think this is a good idea, but it doesn't really make sense to pass it in specialArgs. It's not (just) an argument passed to each module, it's a change to how modules are interpreted, so it should be a top-level argument to evalModules (with null default), and if it is set it can be passed to each module as well if desired.

My real objection, and the lack of modularity, is in hard-coding the m.${classModules}.default lookup. This is setting opinionated policy rather than providing composable mechanism, ironically enough a policy that you just disagreed with in one particular instance, and becomes a piece of magic that cannot be overridden nor inferred from the usage site. I still think the right way to handle this is with a map in the end-user's invocation, but it certainly doesn't need to be in a core agnostic component like the module system; e.g. we could have the map I suggested inside flake-parts.lib.evalFlakeModule

[roberth]

Re class, I think this is a good idea

Cool. I don't really like the name I came up with. I've also considered kind, but I feel that's too tied to value : (type : kind), although we don't have that here. sort seemed ugly but I'd be easily convinced. Maybe there's a better word?

but it doesn't really make sense to pass it in specialArgs.

It is conveniently low overhead when it comes to most evalModules calls (submodules), but I have not made a measurement.

this is setting opinionated policy

That is exactly the intent. Some things shouldn't be left to opinion.
This seems to be where we disagree.

rather than providing composable mechanism

We already have this, and a map based solution can still be used as desired. As such it does not enforce a policy, but merely encourages it.

---

Final question

If we have

• nix run = nix run apps.default
• nix devshell = nix devshell devShells.default

then wouldn't it make sense to have

• imports = [ flake ]; = imports = [ flake.nixosModules.default ];

If you agree, and the module system can't know about flake attributes, we'd need a generic mechanism. I this case the disagreement would be with infinisil.
If you disagree, that choice is a small loss for users and a small win for the ivory tower. We'd have to agree to disagree, and I'll leave this pr open, so that others can form an opinion.

[shlevy]

It is conveniently low overhead when it comes to most evalModules calls (submodules), but I have not made a measurement.

What overhead do you mean? Syntactically, { specialArgs.class = ... } seems basically equivalent overhead to { class = ... }. In terms of runtime, either we allocate an extra attr on the top-level arg to evalModules or we evaluate an extra attr on specialArgs. What's the difference?

That is exactly the intent. Some things shouldn't be left to opinion.

We shouldn't layer flake-specific knowledge into the module system.

that choice is a small loss for users and a small win for the ivory tower

I understand your disagreement with my stance, but I object to this characterization. The abstract principles I'm applying here, of composable interfaces without layering violations, are principles I hold because I think they result in better software and I'm applying them because I think this case is within the context that those principles are sound. The "ivory tower" comment suggests that there is some necessary conflict between theory and practice and I'm choosing theory here, but there is no such conflict.

In this particular case, I only think this is a "loss for users" in a short term sense. The ultimate cost of this decision, and more importantly making decisions like this down the road, is that the code becomes less maintainable, more tightly coupled, more brittle, less usable in new contexts. Especially since the exact same functionality can be moved to components that are actually flake-specific, I see no reason to add this here that wouldn't also result in more and more cruft over time.

[roberth]

It is conveniently low overhead when it comes to most evalModules calls (submodules), but I have not made a measurement.

What overhead do you mean? Syntactically, { specialArgs.class = ... } seems basically equivalent overhead to { class = ... }. In terms of runtime, either we allocate an extra attr on the top-level arg to evalModules or we evaluate an extra attr on specialArgs. What's the difference?

evalModules would have to handle the argument on every invocation and create an entry for it in an Env, not just when it is set, but in each submodule.

That is exactly the intent. Some things shouldn't be left to opinion.

We shouldn't layer flake-specific knowledge into the module system.

... and lib shouldn't have knowledge of systems, derivations, packages shouldn't know about their NixOS tests.
Yet, these violations of the implicit architectural guidelines are very helpful every day. Are those bothering you?

that choice is a small loss for users and a small win for the ivory tower

I understand your disagreement with my stance, but I object to this characterization. The abstract principles I'm applying here, of composable interfaces without layering violations, are principles I hold because I think they result in better software and I'm applying them because I think this case is within the context that those principles are sound. The "ivory tower" comment suggests that there is some necessary conflict between theory and practice and I'm choosing theory here, but there is no such conflict.

We disagree.

In this particular case, I only think this is a "loss for users" in a short term sense. The ultimate cost of this decision, and more importantly making decisions like this down the road,

Each decision should be judged by its merit.

is that the code becomes less maintainable, more tightly coupled, more brittle, less usable in new contexts. Especially since the exact same functionality can be moved to components that are actually flake-specific

It can not. It's supposed to be a convenience, not an eyesore. We've already discussed this.

, I see no reason to add this here that wouldn't also result in more and more cruft over time.

I find this disrespectful. Infinisil and I have been maintaining and improving the module system for the last couple of years, with diligence and care. Why wouldn't we continue to do so?

[shlevy]

evalModules would have to handle the argument on every invocation and create an entry for it in an Env, not just when it is set, but in each submodule.

Ah, right. Would be nice to see measurements here but makes sense.

... and lib shouldn't have knowledge of systems, derivations, packages shouldn't know about their NixOS tests.
Yet, these violations of the implicit architectural guidelines are very helpful every day. Are those bothering you?

Nix is a language for package management. Why shouldn't lib have knowledge of systems and derivations?

As for the package issue, this does actually bother me a bit since it encourages testing only on NixOS and discourages building a testing interface that is decoupled from the distro. But there is also an important difference between a layering violation in leaf applications of a system and layering violations in a core library component, one which is actually used in many different contexts.

Each decision should be judged by its merit.

I agree, my point is that a) on this particular case, this is a small step of additional tight coupling and magic surprise and b) the reasoning applied in this case, if applied in future decisions, would equally justify further tight coupling and magic surprise.

I find this disrespectful. Infinisil and I have been maintaining and improving the module system for the last couple of years, with diligence and care. Why wouldn't we continue to do so?

I wasn't making a comment on your diligence or care, and I appreciate the ongoing work on the module system. I was making a claim about the logical implication of the reasoning being applied here, not about the quality of your efforts. If you disagree, OK, you disagree, I think that you're wrong and that the overall quality of the work will suffer as a result but that doesn't mean I think you're not overall doing an important job well.

[roberth]

I've pivoted the PR to type checking instead of type matching, and it now covers an extra set of errors where we can be quite helpful with our error messages.
No more layer violation.

I've addressed the comments and pivoted the change towards checking

[infinisil]

I took another close look at this and I think I'm onboard with this idea, though I still have some things to point out.

And in addition to the points below, this should also have documentation.

[roberth]

Thanks for the many reviews ❤️

[roberth]

Here's a PR for the postponed types.configuration

• lib.types: Add configuration type #230377

[uninsane]

External use of lib.modules.pushDownProperties is deprecated. If your use case isn't covered by non-deprecated functions, we'd like to know more and perhaps support your use case well, instead of providing access to these low level functions.

@roberth as far as i can tell, my use case isn't covered by non-deprecated functions. as you expected, external uses of these are a bit gnarly, but i'll try here to condense my justification for its use.

config = mkMerge <list> at module scope often causes infinite recursion, e.g. when the items in <list> are computed attrsets: a well-known issue but with AFAIK no widely agreed upon solution. code like this barfs:

{ config, ... }:
{
  options.my.option = mkOption {
    type = types.listOf types.string;
  };
  
  config = mkMerge (map generateMoreConfig config.my.option);
  # ^ ERROR: `config.*` depends on `config.my.option` => infinite recursion
}

assume generateMoreConfig produces an attrset with assertions and users. the simple fix is:

  config = let
    genAssertions = conf: (generateMoreConfig conf).assertions;
    genUsers = conf: (generateMoreConfig conf).users;
  in {
    assertions = mkMerge (map genAssertions config.my.option);
    users = mkMerge (map genUsers config.my.option);
  };

however, this fails if generateMoreConfig includes any lib.mkIf at the toplevel, e.g.

generateMoreConfig = value: lib.mkIf value != "root" {
  assertions = ...; users = ...;
};

hence, any more robust equivalent to module-scope mkMerge has to "push down" that outer lib.mkIf:

generateMoreConfig = value: {
  assertions = lib.mkIf value != "root" ...;
  users = lib.mkIf value != "root" ...;
};

but you don't want to manually push these down. ideally one could take the original, clean-but-unsupported code:

config = mkMerge (map generateMoreConfig config.my.option);

and defeat the infinite recursion by constraining the attrnames at the outer layer -- without contorting the logic everywhere below that:

config = let
  mergeEachAttr (map generateMoreConfig config.my.option);
in {
  assertions = mergeEachAttr.assertions;
  users = mergeEachAttr.users;
};

hence, this mergeEachAttr function has to use lib.pushDownProperties. in case anyone wants to dive deeper, my concrete use:

• helper definition: mkTypedMerge
• where it's used

[roberth]

@uninsane That seems like a valid use case. Could you create an issue for it?

[uninsane]

@uninsane That seems like a valid use case. Could you create an issue for it?

the mkMerge limitations are i think well-known (but with no foreseeable direct "fix"). unless i'm not thinking in the right scope, the only new/actionable issue i can see is "pushDownProperties was marked private even though a single external user still depends on it". but i don't know that that's even an issue: i only felt compelled to say anything about it because you signaled in the code change that you'd like to hear from anyone who's using these. i assumed the code committer would then be in a position to make any relevant judgements.

if you prefer i formalize that feedback through some issue, i'm happy to do that, but i'd need guidance because i have so much less context here than you that i don't know what the substance of any issue here actually is.

[roberth]

My goal is just to have a separate thread for it, not to put you through the bureaucratic wringer or something. This thread already has over a hundred messages, and changing the topic of a long thread makes things harder to find. GitHub hiding the middle also doesn't help.

limitations are i think well-known (but with no foreseeable direct "fix")

While there's an element of there being a fundamental problem that prevents the simplest fix imaginable, that doesn't stop is from finding practical solutions, like you did!

but i'd need guidance because i have so much less context

Most issues are conversations. They don't have to be full-on research presentations. You might fill in the template and paste your existing prose in the appropriate header(s). Markdown source is under the comment edit button ;)