modules/lua-loader: allow null value, do not configure by default

[stasjok]

This changes luaLoader.enable option's type to allow null and sets its default to null.

Why? First, most important: it's not expected to have vim.loader configuration in files submodules. Changing default to null fixes that. Second, as I understand, the goal of this project is to not configure anything by default. I don't particularly like this extra line in init.lua.

My goal is to make files submodules to produce empty configs by default. When I do

files."ftplugin/nix.lua".localOpts.shiftwidth = 2;

I expect ftplugin/nix.lua will have only this option, and nothing else.

If somehow it's not desired, maybe a compromise can be made. Set it to false somewhere in the top-level modules, so that the default in files stays null.

Current behavior is completely undesired. Consider config:

luaLoader.enable = true;
files."plugin/test.lua" = {};

With this config init.lua has vim.loader.enable() and plugin/test.lua has vim.loader.disable()...

[MattSturgeon]

Thanks! This looks good, I've commented on a few minor points we can discuss briefly, but I don't see any blockers that would prevent merging this as-is.

Thanks for the detailed description and justification, this is very helpful.

I completely agree that (where possible) we should try to produce empty output when the config is empty.

[MattSturgeon]

Your commit message could be improved by shortening the summary line and adding a "body" with some of your justification text?

E.g.

modules/lua-loader: make nullable

This avoids having the option always "defined".

Ideally, it should only be included in the output file when explicitly defined by user config.

It was also possible to have vim.loader.enable() in init.lua and vim.loader.disable() in an extra file by simply doing:

```nix
luaLoader.enable = true;
files."plugin/test.lua" = {};
```

(Obviously that's just an example, word it as you please. Should probably try and wrap lines at 72 as per standard commit messages too.)

[MattSturgeon]

Thanks! I can't see any issues.

Could you confirm you've checked the tests fail as expected when this isn't fixed? I'm not aware of any other tests that use the assertions option to do negative checks, so worth checking our tests implementation handles that correctly

[stasjok]

Could you confirm you've checked the tests fail as expected when this isn't fixed? I'm not aware of any other tests that use the assertions option to do negative checks, so worth checking our tests implementation handles that correctly

Under the hood it completely fails evaluation. So this assertions fails even before the build starts. And yes, most of the time I test my test.

It looks like this:

warning: Git tree '/home/stas/upstream/nixvim' is dirty
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'modules-lua-loader'
         whose name attribute is located at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/pkgs/stdenv/generic/make
-derivation.nix:334:7

       … while evaluating attribute 'buildPhase' of derivation 'modules-lua-loader'

         at /nix/store/n59sdgvpn7d93s5f42w6hzmrbywypay5-source/tests/test-derivation.nix:38:7:

           37|       # errors on stderr
           38|       buildPhase = lib.optionalString (!dontRun) (
             |       ^
           39|         ''

       (stack trace truncated; use '--show-trace' to show the full trace)

       error:
       Failed assertions:
       - luaLoader is expected to be explicitly enabled.

At the end will be the full list of failed assertions (for one test only, not all).

[MattSturgeon]

Under the hood it completely fails evaluation. So this assertions fails even before the build starts.

I think this is fine. I thought it'd be something along those lines.

I guess I was worried about the eval failure causing other tests to not be run. Even if so, that's not the end of the world and could be improved separately.

And yes, most of the time I test my test.

Excellent 😁

[MattSturgeon]

LGTM!

Are you able to rebase on main? If not I can do a rebase merge on GitHub.

[stasjok]

I guess I was worried about the eval failure causing other tests to not be run. Even if so, that's not the end of the world and could be improved separately.

Ah, if that's the issue, I think it would be exactly the case, because current test framework runs all the tests in one evaluation. So this test will fail early, nothing else will be ran. And it's hard to tell what test is failed. Only assertion message is useful, even --show-trace shows the wrong places.

[stasjok]

No, I was wrong. With full trace the test could be pin-pointed. Full trace, shown with --show-trace:

stas@server2 ~/u/nixvim (lua-loader-default-null) [1]> nix build --no-link .#checks.x86_64-linux.tests --show-trace
warning: Git tree '/home/stas/upstream/nixvim' is dirty
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'nixvim-tests'
         whose name attribute is located at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/pkgs/stdenv/generic/make
-derivation.nix:334:7

       … while evaluating attribute 'buildCommand' of derivation 'nixvim-tests'

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/pkgs/build-support/trivial-builders/default.nix:68:16:

           67|         enableParallelBuilding = true;
           68|         inherit buildCommand name;
             |                ^
           69|         passAsFile = [ "buildCommand" ]

       … while calling anonymous lambda

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/attrsets.nix:1096:10:

         1095|     attrs:
         1096|     map (name: f name attrs.${name}) (attrNames attrs);
             |          ^
         1097|

       … from call site

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/attrsets.nix:1096:16:

         1095|     attrs:
         1096|     map (name: f name attrs.${name}) (attrNames attrs);
             |                ^
         1097|

       … while calling anonymous lambda

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/pkgs/build-support/trivial-builders/default.nix:531:16:

          530|       linkCommands = lib.mapAttrsToList
          531|         (name: path: ''
             |                ^
          532|           mkdir -p "$(dirname ${lib.escapeShellArg "${name}"})"

       … from call site

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/pkgs/build-support/trivial-builders/default.nix:533:19:

          532|           mkdir -p "$(dirname ${lib.escapeShellArg "${name}"})"
          533|           ln -s ${lib.escapeShellArg "${path}"} ${lib.escapeShellArg "${name}"}
             |                   ^
          534|         '')

       … while calling 'escapeShellArg'

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/strings.nix:454:20:

          453|   */
          454|   escapeShellArg = arg: "'${replaceStrings ["'"] ["'\\''"] (toString arg)}'";
             |                    ^
          455|

       … while evaluating derivation 'modules-lua-loader'
         whose name attribute is located at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/pkgs/stdenv/generic/make
-derivation.nix:334:7

       … while evaluating attribute 'buildPhase' of derivation 'modules-lua-loader'

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/tests/test-derivation.nix:38:7:

           37|       # errors on stderr
           38|       buildPhase = lib.optionalString (!dontRun) (
             |       ^
           39|         ''

       … from call site

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/tests/test-derivation.nix:38:20:

           37|       # errors on stderr
           38|       buildPhase = lib.optionalString (!dontRun) (
             |                    ^
           39|         ''

       … while calling 'optionalString'

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/strings.nix:268:5:

          267|     # String to return if condition is true
          268|     string: if cond then string else "";
             |     ^
          269|

       … while calling anonymous lambda

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/tests/test-derivation.nix:44:13:

           43|           builtins.map (
           44|             {
             |             ^
           45|               derivation,

       … from call site

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/tests/test-derivation.nix:49:13:

           48|             }:
           49|             lib.optionalString (!dontRun) ''
             |             ^
           50|               echo "Running test for ${name}"

       … while calling 'optionalString'

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/strings.nix:268:5:

          267|     # String to return if condition is true
          268|     string: if cond then string else "";
             |     ^
          269|

       … from call site

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/tests/test-derivation.nix:52:45:

           51|
           52|               output=$(HOME=$(realpath .) ${lib.getExe derivation} -mn --headless "+q" 2>&1 >/dev/null)
             |                                             ^
           53|               if [[ -n $output ]]; then

       … while calling 'getExe'

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/meta.nix:356:12:

          355|   */
          356|   getExe = x: getExe' x (x.meta.mainProgram or (
             |            ^
          357|     # This could be turned into an error when 23.05 is at end of life

       … from call site

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/meta.nix:356:15:

          355|   */
          356|   getExe = x: getExe' x (x.meta.mainProgram or (
             |               ^
          357|     # This could be turned into an error when 23.05 is at end of life

       … while calling 'getExe''

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/meta.nix:396:16:

          395|   */
          396|   getExe' = x: y:
             |                ^
          397|     assert assertMsg (isDerivation x)

       … while evaluating derivation 'nixvim'
         whose name attribute is located at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/pkgs/stdenv/generic/make
-derivation.nix:334:7

       … while evaluating attribute 'paths' of derivation 'nixvim'

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/wrappers/standalone.nix:41:7:

           40|       name = "nixvim";
           41|       paths = [
             |       ^
           42|         config.finalPackage

       … from call site

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/wrappers/standalone.nix:44:12:

           43|         config.printInitPackage
           44|       ] ++ pkgs.lib.optional config.enableMan self.packages.${pkgs.stdenv.hostPlatform.system}.man-docs;
             |            ^
           45|       meta.mainProgram = "nvim";

       … while calling 'optional'

         at /nix/store/qmh8bas1qni03drm0lnjas2azh7h87cn-source/lib/lists.nix:784:20:

          783|   */
          784|   optional = cond: elem: if cond then [elem] else [];
             |                    ^
          785|

       … from call site

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/wrappers/standalone.nix:37:16:

           36|       };
           37|       config = handleAssertions evaledModule.config;
             |                ^
           38|     in

       … while calling 'handleAssertions'

         at /nix/store/nwnrh4s538ki3nznyhbhrnn8a465rl1r-source/wrappers/standalone.nix:14:5:

           13|   handleAssertions =
           14|     config:
             |     ^
           15|     let

       error:
       Failed assertions:
       - luaLoader is expected to be explicitly disabled.

Line while evaluating derivation 'modules-lua-loader' can be found in the middle.

[stasjok]

Are you able to rebase on main? If not I can do a rebase merge on GitHub.

Done.

[MattSturgeon]

I guess I was worried about the eval failure causing other tests to not be run. Even if so, that's not the end of the world and could be improved separately.

Ah, if that's the issue, I think it would be exactly the case, because current test framework runs all the tests in one evaluation. So this test will fail early, nothing else will be ran. And it's hard to tell what test is failed.

I think #1898 will mitigate this slightly, but we can resolve it more fully in some future work.

Thanks for your efforts! I'll try and look at your other PRs again soon, just currently a little busy.