`bool_comparison` rule may report false positives

[montchr]

Consider the following fake module:

{
  config,
  lib,
  ...
}: let
  cfg = config.foo;
in {
  options.foo.start = lib.mkOption {
    type = with lib.types; either bool (enum ["graphical" "non-graphical"]);
    default = false;
    example = "graphical";
    description = ''
      Whether to launch the foo service with the systemd user session.
    '';
  };
  config = {
    systemd.user.services.foo = {
      Unit.After = lib.optional (cfg.start == "graphical") "graphical-session.target";
    } // lib.optionalAttrs (cfg.start != false) {
      Install.WantedBy = [(
        if (cfg.start == true || cfg.start == "non-graphical")
        then "default.target"
        else "graphical-session.target"
      )];
    };
  };
}

Note the type of foo.start can be either boolean or a string -- not my decision, this was simplified from a home-manager module [source], where changing the option types could potentially be a breaking change for users.

And here is the result of running statix check:

[W01] Warning: Unnecessary comparison with boolean
    ╭─[modules/homeModules/services/test-thing.nix:21:29]
    │
 21 │       // lib.optionalAttrs (cfg.start != false) {
    ·                             ─────────┬────────
    ·                                      ╰────────── Comparing cfg.start with boolean literal false
────╯
[W01] Warning: Unnecessary comparison with boolean
    ╭─[modules/homeModules/services/test-thing.nix:24:17]
    │
 24 │             if (cfg.start == true || cfg.start == "non-graphical")
    ·                 ────────┬────────
    ·                         ╰────────── Comparing cfg.start with boolean literal true

statix fix would change both of those conditions to omit the boolean comparison. But I'm pretty sure that "fixing" either of these would result in an error, at least without further implementing some type checking. Since the value might be a string, using cfg.start alone in the second case (L24) would result in a "error: value is a string while a Boolean was expected".

I also cited this example in #61, which would be a nice solution for the situation if I didn't want to disable the bool_comparison rule entirely.

[MattSturgeon]

I've also just ran into this.

In my example I have an attrset of (package-derivation, null, bool).

While mapping the attr values, I wanted to check (val != false), but I've had to do (isBool val -> val) instead to avoid the lint.

nnix-repl> vals = { null = null; true = true; false = false; str = "str"; num = 42; }

nix-repl> builtins.mapAttrs (n: v: builtins.isBool v -> v) vals
{ false = false; null = true; num = true; str = true; true = true; }