Report module import antipattern

[roberth]

Is your feature request related to a problem? Please describe.

The module system can only track file names it gets to see. This means the items in imports and modules should ideally be file names, as well as flake.nixosModules.<name> and similar attributes.

Unfortunately, nix flake check used to check the opposite, mistakenly suggesting that nixosModules.<name> should not be a path value, with the wrong assumption that it's equivalent and easier to validate.

Describe the solution you'd like

Before evaluating anything, find the flake attributes that should be modules, but do not yet evaluate them.
If they are thunks, check that they aren't calls to import, by inspecting the thunk's expr.
Calls to other functions are ok (probably necessary). Functions (not thunks) and attribute sets are also ok; they'd probably be anonymous modules, but that's allowed.
If the module is represented by a thunk that immediately performs a call to import, report a warning that import can be removed to improve the module system's error reporting and module deduplication.

Examples:

No warning:

 outputs = { ... }: {
    nixosModules.foo = ./modules/nixos;
 };

Warning:

 outputs = { ... }: {
    nixosModules.foo = import ./modules/nixos;
#                      ^^^^^^
# unnecessary import hides file path from module system error reporting and deduplication
 };

Unknown: this expression needs to be evaluated to find what's inside modules/nixos/default.nix

 outputs = { ... }: {
    # No actual module in sight. This `import` is ok.
    nixosModules = import ./modules/nixos;
 };

Describe alternatives you've considered

Do not perform evaluation? This would make it hard to know which attributes are actually turned into which flake attributes, needing a heuristic. That's harder to implement and not necessarily better.

Additional context

• showOption giving incorrect location for modules brought in via Flakes NixOS/nix#11210

[fzakaria]

I documented it in https://fzakaria.com/2024/07/29/import-but-don-t-import-your-nixos-modules.html as well.

[inclyc]

Hi @roberth, @fzakaria,

So as per my understanding, this issue actually does not relate to flake, but the module system, right?

i.e. maybe we can just match AST to see if the file looks like a module, then see if it imports some file, if so, the linter may suggest

- import $xxfile;
+ $xxfile

to make sure the location is better preserved, is that right?

is there a way to somehow test for a NixOS module use and check if it's brought in via import?

There are no obvious ways to check how random nix file is imported, but it is obvious to check the importer. i.e. just file linting warning for each import xxx in module imports.

Before evaluating anything, find the flake attributes that should be modules, but do not yet evaluate them.
If they are thunks, check that they aren't calls to import, by inspecting the thunk's expr.
Calls to other functions are ok (probably necessary). Functions (not thunks) and attribute sets are also ok; they'd probably be anonymous modules, but that's allowed.

If the module is represented by a thunk that immediately performs a call to import, report a warning that import can be removed to improve the module system's error reporting and module deduplication.

nixf-tidy linter is a pure static linter, thus there are no thunks actually, so maybe the final implementation will be matching some idioms.

Do not perform evaluation? This would make it hard to know which attributes are actually turned into which flake attributes, needing a heuristic. That's harder to implement and not necessarily better.

As commented above, nixf-tidy does not perform evaluation.

[inclyc]

The reason why there is no evaluation is

it is hard to make a reasonable evaluation target facing a large nix project.

And manually configuring it for each project leads to bad user experience.

[roberth]

There's pretty much only one way to evaluate a flake, so it'd be a safe bet, but I understand that this may not be desirable. For instance, it'd be difficult to run the linter in the Nix sandbox if it did.

Heuristics probably work quite well.

I had proposed to evaluate because any attribute set can be a module. It'd be nice to be able to detect modules reliably, but it's not absolutely necessary.

Let's break down the problem:

Without any other knowledge, import $path can be a valid module expression. Matching these alone would obviously produce too many false positives.

So the heuristic needs to determine whether the expression import $path occurs in a context where a module is expected.

I'd look for the following patterns to find places where modules are expected. Each line can match any subexpression.

# These two catch usage with `evalModules`, inside other modules, submodules, `nixosSystem` and many more
modules = [ $match ... ];
imports = [ $match ... ];

# These are flakes-inspired, but don't necessarily have to occur in a `flake.nix` `outputs`,
# considering use of flake-utils, flake-parts and other libraries obscures the otherwise
# trivial "data flow", but their use still tends to show attributes like this.
nixosModules.$name = $match;
nixosModules = { $name = $match; ... };
nixosModules = rec { $name = $match; ... };

# I'll omit equivalent syntaxes in the rule that follow.

homeManagerModules.$name = $match;
darwinModules.$name = $match;
# perhaps:
${anyName}Modules.$name = $match;

# https://github.com/NixOS/nix/issues/6257
modules.$name.$name2 = $match;


# I don't know if this occurs, but it seems like a safe bet anyway
modules.$name = $match;
# Perhaps also:
module = $match;

So that is a set of pretty "eager" rules to find possible modules. I think substituting $match = import $path makes it specific enough not to produce many false warnings.

[Aleksanaa]

We have no flake detection atm, and would be good to check some common patterns in flakes (Maybe also let nixd understand flake urls?) Maybe we can borrow some checks from https://github.com/NixOS/nix/tree/master/src/libflake?

{
  description = 1;

  inputs = {
    nixpkgs.url = 2;
  };
}

[inclyc]

https://github.com/NixOS/nix/tree/master/src/libflake

Please submit a new issue :)