nix-community / nixvim

Configure Neovim with Nix! [maintainer=@GaetanLepage, @traxys, @mattsturgeon, @khaneliman]
https://nix-community.github.io/nixvim
MIT License
1.59k stars 250 forks source link

plugins/todo-comments: assertions read `cfg.keymaps.todoTelescope.key`, which may be an undefined option #2181

Closed trueNAHO closed 3 days ago

trueNAHO commented 1 week ago

Why are the programs.nixvim.plugins.todo-comments.keymaps.todoTelescope and programs.nixvim.plugins.todo-comments.keymaps.todoTrouble options not allowed to be null:

    assertions = [
      {
        assertion = cfg.keymaps.todoTelescope.key or null != null -> config.plugins.telescope.enable;
        [...]
      }
      {
        assertion = cfg.keymaps.todoTrouble.key or null != null -> config.plugins.trouble.enable;
        [...]
      }
    ];

These assertions were introduced in commit https://github.com/nix-community/nixvim/commit/379ae77a76ba2818b0c7876e97636f7064b84db6 ("plugins/todo-comments: migrate to mkNeovimPlugin") and require me to use the

programs.nixvim.plugins.todo-comments.keymaps.todoTrouble =
  lib.mkIf <CONDITION> { key = <VALUE>; };

workaround instead of the simpler

programs.nixvim.plugins.todo-comments.keymaps.todoTrouble.key =
  lib.mkIf <CONDITION> <VALUE>;

pattern in my standalone Home Manager setup.

Would it be possible to lift this non-null restriction?


Note: I have been inactive in NixVim discussions for more than half a year.

MattSturgeon commented 1 week ago

These assertions don't say "cfg.keymaps.todoTelescope.key can't be null", they say "if cfg.keymaps.todoTelescope.key isn't null, then config.plugins.telescope.enable should be true".

khaneliman commented 1 week ago

Yeah, those assertions won't prevent that. I tested with adding todoTrouble that way and it works for me? https://github.com/khaneliman/astronvim/commit/8893af3eddb8dd6b9946e8ba8fbfc7c0a89ca1ed

EDIT: Oh, I see... when I disable trouble it throws the error because we're accessing the key option....

 error: The option `plugins.todo-comments.keymaps.todoTrouble.key' was accessed but has no value defined. Try setting the option.
trueNAHO commented 1 week ago

These assertions don't say "cfg.keymaps.todoTelescope.key can't be null", they say "if cfg.keymaps.todoTelescope.key isn't null, then config.plugins.telescope.enable should be true".

Thanks for the clarification.

EDIT: Oh, I see... when I disable trouble it throws the error because we're accessing the key option....

 error: The option `plugins.todo-comments.keymaps.todoTrouble.key' was accessed but has no value defined. Try setting the option.

Yes, I was essentially referring to the case when programs.nixvim.plugins.todo-comments.keymaps.todoTrouble.key is set based on programs.nixvim.plugins.trouble.enable:

Specifically, I want to set todo-comments.keymaps.todoTrouble.key regardless of programs.nixvim.plugins.trouble.enable being set to true or false. Maybe the assertion should be removed and a lib.mkIf should be added where the keymap action is instantiated?

trueNAHO commented 1 week ago

Maybe the assertion should be removed and a lib.mkIf should be added where the keymap action is instantiated?

Unless there are objections, I will submit a patch for this. While I am at it, I will search for assertion and coherently apply this behaviour tree wide.

khaneliman commented 1 week ago

Maybe the assertion should be removed and a lib.mkIf should be added where the keymap action is instantiated?

Unless there are objections, I will submit a patch for this. While I am at it, I will search for assertion and coherently apply this behaviour tree wide.

The assertions were an intentional addition and something we are trying to migrate towards. I would rather invest time into fixing the actual bug that is throwing the error for the used not defined message on accessing the key property due to the change in the option trying to adhere to https://github.com/nix-community/nixvim/issues/603

MattSturgeon commented 6 days ago

Oh, I see... when I disable trouble it throws the error because we're accessing the key option....

 error: The option `plugins.todo-comments.keymaps.todoTrouble.key' was accessed but has no value defined. Try setting the option.

Just to confirm, this issue doesn't relate to the assertions and instead relates to an option "used but not defined" error (i.e. there's no default value for an option we're reading)?

khaneliman commented 6 days ago

Oh, I see... when I disable trouble it throws the error because we're accessing the key option....

 error: The option `plugins.todo-comments.keymaps.todoTrouble.key' was accessed but has no value defined. Try setting the option.

Just to confirm, this issue doesn't relate to the assertions and instead relates to an option "used but not defined" error (i.e. there's no default value for an option we're reading)?

Yeah, in the MRE you get the "used but not defined error" when the key is accessed and the right hand side evaluates to false since we use it in the assertions and the keymaps config.

I created a local test case like this:

  conditional-mappings =
    { config, ... }:
    {
      plugins.todo-comments = {
        enable = true;

        keymaps = {
          todoTrouble.key = lib.mkIf config.plugins.trouble.enable "<leader>xq";
          todoTelescope = lib.mkIf config.plugins.telescope.enable {
            key = "<leader>ft";
            keywords = [
              "TODO"
              "FIX"
              "FIX"
            ];
          };
        };
      };
    };
trueNAHO commented 6 days ago

How should this be resolved:

Or is the problem caused by something else entirely?

khaneliman commented 6 days ago

We were talking on matrix a bit about this and it basically has to do with the non nullable key option within https://github.com/nix-community/nixvim/blob/d0c082124535acd6f0f786be42b38c32abe89d64/lib/keymap-helpers.nix#L108-L115 I think we have to adjust the definition here to support defaults on the options so they can be accessed. I was working on a fix locally, but had to take a break for a bit.

MattSturgeon commented 6 days ago

Check if config.plugins.telescope.enable exists using ?

The issue isn't that the option or module don't exist.

Instead, options that are declared always exist. But attempting to access the value of an option that exists but is not defined anywhere will throw an error.

It is not a nix language error. It is an error thrown by the nixpkgs module system when it detects someone reading the value of an option that is undefined.

Example

{ config, lib, ... }:
{
  # Declare foo as a boolean option, that converts its final value to a stupid string...
  options = {
    # this option defaults to false
    hasDefault = lib.mkOption {
      type = types.bool;
      default = false;
    };
    # this option has no `default`
    noDefault = lib.mkOption {
      type = types.bool;
    };
    # NOTE: `missing` is not a declared option
  };

  config = {
    assertions = [
      {
        assertion = config.hasDefault == false;
        message = ''
          `hasDefault` exists _and_ is defined.
          Its value is `false`.
        '';
      }
      {
        assertion = config ? noDefault;
        message = ''
          `noDefault` is present in `config`.
          NOTE: we haven't checked its _value_ here, only its existance.
        '';
      }
      {
        assertion = config.noDefault or false;
        message = ''
          This will throw "used but not defined".
          `or` has no effect, because the attr _is_ present.
          The attr't _value_ is `throw "option `noDefault' used but not defined"`
        '';
      }
      {
        assertion = config ? missing;
        message = ''
          `missing` is _not_ present in `config`, because no option named `missing` is declared.
        '';
      }
    ];
  };
}