Prevent `mkIf`'s from being `//`'d, and co. #72949

infinisil · 2019-11-06T23:02:02Z

Motivation for this change

mkIf can be very deceiving, since the following doesn't do what you might expect:

{
  some.option = {
    foo = "foo";
  } // mkIf cond {
    bar = "bar";
  };
}

Because in fact, this results in exactly the same behavior as

{
  some.option = mkIf cond {
    bar = "bar";
  };
}

The assigment to foo gets completely ignored! This is because of the implementation of mkIf:

nixpkgs/lib/modules.nix

Lines 500 to 503 in 9221668

    
           mkIf = condition: content: 
        
             { _type = "if"; 
        
               inherit condition content; 
        
             };

which makes the resulting value in the above example be of the form

{ _type = "if"; condition = <cond>; content = { bar = "bar"; }; foo = "foo"; }

mkIf creates an attribute set with some special keys the module system uses to figure out whether it's a mkIf and allow lazy evaluation of the condition and contents. So using // on these attribute sets makes no sense because the keys don't represent the actual values, but some meta attributes!

This has led to at least one annoying bug in NixOS, see #72916. In addition I've seen this a couple times when reviewing NixOS module PRs too.

With this change, doing such mistakes gives the error

error: value is a function while a set was expected

which admittedly is pretty bad, but any error is better than incorrect behavior that potentially never gets noticed, and this is the only way I could come up with an error being produced at all that doesn't require changing everything.

Now when the user sees this error they have to fix their conditionals by using mkMerge which correctly handles this:

{
  some.option = mkMerge [
    { foo = "foo"; }
    (mkIf cond { bar = "bar"; })
  ];
}

This problem can also occur with other such functions like mkMerge, mkOverride and mkOrder combined with // or attribute access. This PR will make all of those throw an error when used incorrectly.

See the commits for an explanation of the implementation.

Ping @jtojnar @JohnAZoidberg @worldofpeace @rycee

Things done

Tested that this indeed produces an error for such incorrect assignments
Made sure that the minimal graphical installer still evaluates
Found and eliminated all uses of mkIf + // (and similar) in nixpkgs

grahamc · 2019-11-06T23:03:45Z

Is there a way the errors could be nicer?

infinisil · 2019-11-06T23:16:32Z

@grahamc I considered different things, but none really work out:

`builtins.addErrorContext`

I tried using builtins.addErrorContext such that potentially with --show-trace it could have more information, but it turns out that this doesn't work with functions, which is what I'd need here:

$ nix-instantiate --eval -E '(builtins.addErrorContext "context" (throw "error!"))' --show-trace
error: context
error!
$ nix-instantiate --eval -E '(builtins.addErrorContext "context" (x: throw "error!")) 0' --show-trace
error: while evaluating anonymous function at (string):1:38, called from (string):1:1:
error!

Detecting `//`

I also considered somehow detecting // being applied to mkIf's, but that would necessitate all mkIf's having some different set of attribute names (because if attrNames a == attrNames b then a // b == b, making the // undetectable). But this kind of implies that mkIf's would have to be impure, potentially using builtins.unsafeGetAttrPos to distinguish different ones, which might work but it would be very unreliable and could make things strict which mkIf tries to prevent in the first place.

`__cantIntersectThis`

A questionable solution by changing Nix would be to add some __cantIntersectThis attribute you can use such that

nix-repl> { __cantIntersectThis = true; } // { ... }
error: Can't intersect those attribute sets

infinisil · 2019-11-07T00:45:15Z

lib/modules.nix

+  applyIfFunction = key: f: args@{ config, options, lib, ... }:
+    # We only want to apply module arguments if we're actually dealing with a
+    # module function, an opaque wrapper isn't one
+    if isFunction f && ! isOpaqueWrapper f then


This right here is a bit hacky. It's needed because the module system assumes that if you have a function as a module, it must be a NixOS module function. This worked fine with imports = [ (mkMerge [ ... ]) ]; previously, because mkMerge didn't return a function but an attrset. But with this change it does. So we need to prevent it trying to call the opaque wrapper function (which is really just an implementation detail) with NixOS arguments.

I'll argue that the module system allowing both attribute sets and functions as modules on a guessing basis is hacky already, so this is just a counter-hack!

edolstra · 2019-11-07T09:56:01Z

What's the impact on evaluation time / memory?

infinisil · 2019-11-07T11:21:33Z

@edolstra See https://github.com/NixOS/nixpkgs/pull/72949/checks?check_run_id=291926489

I'm not entirely sure what this eval run includes, it might be nix-instantiate nixos/release-combined.nix -A tested. @grahamc Can you confirm this?

nbp

Thanks for noticing this issue and fixing it.

lib/modules.nix

infinisil · 2019-11-14T02:23:56Z

ToDo: Make sure this isn't bad for performance (it might be that ofborg's eval only measures nixpkgs time)

infinisil · 2019-11-14T02:49:31Z

Doing a measurement on my system with 4 samples:

Without this change the rebuild eval time is 5.99 (stddev 0.06)
With this change it's 6.37 (stddev 0.04)

So that's about a 6% increase in evaluation time. Not sure if worth. It sure can prevent bugs, but the error message is really not that great, and 6% is kind of a lot. Wouldn't have to worry about that if #57477 was resolved

stale · 2020-06-01T04:24:17Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

infinisil requested review from edolstra and nbp as code owners November 6, 2019 23:02

infinisil force-pushed the opaque-wrappers branch from a2047ac to 7192b4e Compare November 7, 2019 00:24

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 7, 2019

infinisil commented Nov 7, 2019

View reviewed changes

nbp approved these changes Nov 7, 2019

View reviewed changes

lib/modules.nix Outdated Show resolved Hide resolved

infinisil added 2 commits November 7, 2019 19:43

lib/modules: Add opaque wrapper functions

8a954e6

lib/modules: Wrap mkIf and co. in an opaque wrapper

7a71057

infinisil force-pushed the opaque-wrappers branch from 7192b4e to 7a71057 Compare November 7, 2019 18:43

worldofpeace mentioned this pull request Jan 9, 2020

nixos/buildbot: fix eval, and some twin issues #77324

Merged

10 tasks

jtojnar mentioned this pull request Jan 30, 2020

[wip] nixos/lsb.nix: init #78798

Closed

10 tasks

infinisil added the 6.topic: module system About NixOS module system internals label Mar 19, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020

infinisil closed this Jun 1, 2020

infinisil mentioned this pull request Jul 15, 2020

Missing submodules in Nixos nixcloud/nixcloud-webservices#64

Closed

infinisil deleted the opaque-wrappers branch July 1, 2021 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent `mkIf`'s from being `//`'d, and co. #72949

Prevent `mkIf`'s from being `//`'d, and co. #72949

infinisil commented Nov 6, 2019

grahamc commented Nov 6, 2019

infinisil commented Nov 6, 2019

infinisil Nov 7, 2019

edolstra commented Nov 7, 2019

infinisil commented Nov 7, 2019

nbp left a comment

infinisil commented Nov 14, 2019

infinisil commented Nov 14, 2019

stale bot commented Jun 1, 2020

	mkIf = condition: content:
	{ _type = "if";
	inherit condition content;
	};

Prevent mkIf's from being //'d, and co. #72949

Prevent mkIf's from being //'d, and co. #72949

Conversation

infinisil commented Nov 6, 2019

Motivation for this change

Things done

grahamc commented Nov 6, 2019

infinisil commented Nov 6, 2019

builtins.addErrorContext

Detecting //

__cantIntersectThis

infinisil Nov 7, 2019

Choose a reason for hiding this comment

edolstra commented Nov 7, 2019

infinisil commented Nov 7, 2019

nbp left a comment

Choose a reason for hiding this comment

infinisil commented Nov 14, 2019

infinisil commented Nov 14, 2019

stale bot commented Jun 1, 2020

Prevent `mkIf`'s from being `//`'d, and co. #72949

Prevent `mkIf`'s from being `//`'d, and co. #72949

`builtins.addErrorContext`

Detecting `//`

`__cantIntersectThis`