Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

@Infinisil
Copy link
Member

Infinisil commented Nov 6, 2019

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
@Infinisil Infinisil requested review from edolstra and nbp as code owners Nov 6, 2019
@grahamc
Copy link
Member

grahamc commented Nov 6, 2019

Is there a way the errors could be nicer?

@Infinisil
Copy link
Member Author

Infinisil commented Nov 6, 2019

@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 Infinisil force-pushed the Infinisil:opaque-wrappers branch from a2047ac to 7192b4e Nov 7, 2019
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 comment has been minimized.

Copy link
@Infinisil

Infinisil Nov 7, 2019

Author Member

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
Copy link
Member

edolstra commented Nov 7, 2019

What's the impact on evaluation time / memory?

@Infinisil
Copy link
Member Author

Infinisil commented Nov 7, 2019

@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
nbp approved these changes Nov 7, 2019
Copy link
Member

nbp left a comment

Thanks for noticing this issue and fixing it.

lib/modules.nix Outdated Show resolved Hide resolved
Infinisil added 2 commits Nov 6, 2019
@Infinisil Infinisil force-pushed the Infinisil:opaque-wrappers branch from 7192b4e to 7a71057 Nov 7, 2019
@Infinisil
Copy link
Member Author

Infinisil commented Nov 14, 2019

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

@Infinisil
Copy link
Member Author

Infinisil commented Nov 14, 2019

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

@worldofpeace worldofpeace mentioned this pull request Jan 9, 2020
0 of 10 tasks complete
@jtojnar jtojnar mentioned this pull request Jan 30, 2020
0 of 10 tasks complete
@stale
Copy link

stale bot commented Jun 1, 2020

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:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.