Implementation: Given a disabled plugin, don't consider it's dependencies

[mehalter]

EDIT: This has been implementd in Lazy v10.4.0 with #1058

---

Some plugins might be configuring options in other plugins on a need to have basis through it's dependencies. If this plugin is then disabled it seems that it would make sense to not consider the plugins defined in it's dependencies as they aren't needed anymore. With the current implementation this is sorta how it works. Here are a few examples:

Case 1 (works as intended)

return {
  {
    "A",
    opts = {
      foo = true,
    },
  },
  {
    "B",
    dependencies = {
      {
        "A",
        opts = {
          bar = true,
        },
      },
    },
  },
}

In this example both plugins A and B are installed and A has the final options of { foo = true, bar = true } This makes sense and the result is showing that A should always have foo = true as an option and bar = true is required for plugin B to work.

Case 2 (works as intended)

return {
  {
    "B",
    enabled = false,
    dependencies = {
      {
        "A",
        opts = {
          bar = true,
        },
      },
    },
  },
}

Here in this example we are only defining A as a dependency of B and B is currently disabled. This means that A is not installed at all and is not available because it is no longer needed anywhere else and the plugin that depends on it isn't available

Case 3 (potentially incorrect behavior)

return {
  {
    "A",
    opts = {
      foo = true,
    },
  },
  {
    "B",
    enabled = false,
    dependencies = {
      {
        "A",
        opts = {
          bar = true,
        },
      },
    },
  },
}

This is similar to Case 1 but the difference here is B is disabled. The final result of this case is B is not installed and A is installed and setup with the options of { foo = true, bar = true }. It seems like this might not be the intended behavior. The plugin specification is saying that bar = true is needed as a dependency of B and is no longer necessary because B is not needed.

Proposed behavior: I think it would be better to not consider the dependency plugin specifications at all where A would only be setup in this case with the options of { foo = true }

It is a bit strange how committed/not committed the current approach is to caring/not caring about dependencies. where dependencies are fully considered and processed, options and all unless the dependencies are not available anywhere else in which case they are fully not considered and they are uninstalled. I think it makes sense to unifying this "caring" of dependencies and just not processing them at all. If the user wants bar = true in this case without plugin B added then it should not be specified as a dependencies options.

Let me know what you think! 😄 I am a huge fan of the semantics that lazy.nvim implements and adheres to. This is just one place I found that seems to have a bit of a disconnect. Thanks for your time and all of the effort you have put into this project and the Neovim community as a whole! ❤️

[abeldekat]

Hello @mehalter,

I read your comments on the original discussion.
Firstly, I would like to join you thanking Folke for his efforts!

I can follow your line of thinking and would like to add my own thoughts. Do note that they are merely thoughts, based on my experience in adding a tiny piece of implementation to the code base.

The user has a lot of flexibility provided by lazy.nvim in the manner his setup is configured.

Consider a normal plugin, at the top level. It can be defined and enhanced in multiple places, providing a powerful base for a setup like for example LazyVim.

Reading the code, plugins defined in "dependencies" can be configured as a full spec also.
Seen in this manner, your "case 3" can be understood differently. Plugin A is configured in multiple places, one location happens to be inside the dependencies field of plugin B. The definition of plugin B could be in a totally unrelated module.

Following these thoughts, the plugin manager does not need to care about the semantics of the configuration of plugin A. The options provided are treated technically, as data to be merged. The "user" knows that "bar=true" is relevant for the definition placed as a dependency of B.

The package manager, given the dependencies field, needs to make one thing sure: The relevant plugins need to be setup before the parent plugin (ie B) activates. It does provide an "extra" service: When plugin A can never be active, offer to uninstall. In the case of plugin A being defined in an optional context only: Do not install at all.

Best regards!

[mehalter]

Yeah that's true, sometimes these options are only able to be used when plugins are available. So in my configuration I just use the function notation and check if the plugin is available in lazy before setting things. This becomes pretty verbose especially because the idea is captured in the fact that it's a dependency so it would be nice to not have to include all this boiler plate. Since when the plugin isn't enabled I strictly need those options to not be set.

[abeldekat]

Hi @mehalter,

I have been thinking about your idea the last couple of days.

The explanation in my previous response outlines a possible reasoning given the current implementation, where plugin definitions are first "processed" into plugins. Inspecting them for disabled/optional happens thereafter. As a consequence, all individual definitions are already merged. Removing a dependency is only possible when not in active use elsewhere.

As a challenge I am trying my hand at reversing the process. Gathering raw plugin definitions, only entering the "merging" stage with active ones.
Would you be willing to test that code once it's ready? I am not quite there yet, there are some hurdles to take.

[mehalter]

Yeah definitely I would love to test it out! Let me know when it's ready and I can take it for a spin 🚀 Thanks for taking the time to think about this, I know it's definitely a bit different than the current approach but it seems that it could be pretty expressive.

[abeldekat]

Linking to a relevant discussion in a pull request for LazyVim

[abeldekat]

Hi @mehalter,

The code is ready to test.

I think the approach would be an improvement, but there are breaking changes, described as a top-level comment in "lazy/core/plugin.lua"

I tested it locally, with the code checked out in ~/projects/lazy.nvim
The command to start nvim would then be:

LAZY=~/projects/lazy.nvim nvim

In lazy.lua, the "dev" option needs to be activated.

Thanks!

[mehalter]

I just gave it a pretty intense check with a pretty complex configuration and I'm running into a bunch of errors that look like this:

Failed to load `astronvim.plugins._astrocore_autocmds`
/home/micah/projects/lazy.nvim/lua/lazy/core/plugin.lua:416: module 'astronvim.plugins._astrocore_autocmds' not found:
^Ino field package.preload['astronvim.plugins._astrocore_autocmds']
cache_loader: module astronvim.plugins._astrocore_autocmds not found
cache_loader_lib: module astronvim.plugins._astrocore_autocmds not found
^Ino file './astronvim/plugins/_astrocore_autocmds.lua'
^Ino file '/__w/neovim/neovim/.deps/usr/share/luajit-2.1.0-beta3/astronvim/plugins/_astrocore_autocmds.lua'
^Ino file '/usr/local/share/lua/5.1/astronvim/plugins/_astrocore_autocmds.lua'
^Ino file '/usr/local/share/lua/5.1/astronvim/plugins/_astrocore_autocmds/init.lua'
^Ino file '/__w/neovim/neovim/.deps/usr/share/lua/5.1/astronvim/plugins/_astrocore_autocmds.lua'
^Ino file '/__w/neovim/neovim/.deps/usr/share/lua/5.1/astronvim/plugins/_astrocore_autocmds/init.lua'
^Ino file './astronvim/plugins/_astrocore_autocmds.so'
^Ino file '/usr/local/lib/lua/5.1/astronvim/plugins/_astrocore_autocmds.so'
^Ino file '/__w/neovim/neovim/.deps/usr/lib/lua/5.1/astronvim/plugins/_astrocore_autocmds.so'
^Ino file '/usr/local/lib/lua/5.1/loadall.so'
^Ino file './astronvim.so'
^Ino file '/usr/local/lib/lua/5.1/astronvim.so'
^Ino file '/__w/neovim/neovim/.deps/usr/lib/lua/5.1/astronvim.so'
^Ino file '/usr/local/lib/lua/5.1/loadall.so'
# stacktrace:
  - ~/.config/nvim_astrov4/lua/config/lazy.lua:25
  - ~/.config/nvim_astrov4/init.lua:2

All I have in my lazy.nvim spec is

{
  { "folke/lazy.nvim", dev = true },
  { "AstroNvim/AstroNvim", dev = true, import = "astronvim.plugins" },
}

and it doesn't seem to be able to resolve any of the plugin files inside this astronvim module. Not sure if this is a helpful description of the problem but it seems like it's not properly resolving the modules in a plugin that is there for a collection of imports maybe.

Here is a link to the plugin specifications that I currently am using if that is helpful. I'm not sure if it is since the issue is happening before they are being read in anyway:

https://github.com/AstroNvim/AstroNvim/tree/v4/lua/astronvim/plugins

I am basically getting the above error for each file in this plugins/ folder that I am trying to import

[abeldekat]

I am not very knowledgeable regarding AstroNvim, so I had a try:

1. Create a zsh alias for astro: astro='NVIM_APPNAME=astro nvim'
2. Follow the steps from the site to install astronvim to ~/.config/astro
3. Type: "astro" and astronvim appears
4. Now, in ~/config/astro, edit lua/astronvim/lazy.lua

Add the following line below linenr 21:

vim.opt.rtp:prepend(vim.env.LAZY or lazypath)

Add the following line below the line containing spec=spec:

dev = { patterns = jit.os:find "Windows" and {} or { "abeldekat", "lazy.nvim" } },

Run:

LAZY=~/projects/lazy.nvim astro

AstroNvim now runs with lazy.nvim, found in ~/projects/lazy.nvim
At first glance, this works fine.
You can observe this in astronvim, by typing "Lazy".
The color in the "loaded" section of the lazy UI will differ next to lazy.nvim.

Best regards!

[mehalter]

Ah I see, actually I believe you may not have followed the link I sent. The link I sent is a link to the v4 branch which is a big refactor where AstroNvim is simply just a plugins/ folder. So there is no abstract or internal structure outside of plugin specifications. This removes all of the structure that you may not be familiar with :) That's why I was giving the link to the plugins/ folder that I was testing it with and the exact lines to add to lazy.nvim.

You can just take a base install of lazy.nvim and add this to your spec to try it (exactly as you would with LazyVim if you are familiar):

  { "AstroNvim/AstroNvim", branch = "v4", import = "astronvim.plugins" },

[mehalter]

@abeldekat here is to help test, this is a completely minimal init.lua file based on the lazy.nvim minimal.lua for reporting bugs to add your dev line and also the $LAZY env variable:

local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
	vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
	vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath })
end
vim.opt.rtp:prepend(vim.env.LAZY or lazypath)

-- install plugins
local plugins = {
	{ "AstroNvim/AstroNvim", branch = "v4", import = "astronvim.plugins" },
}
require("lazy").setup(plugins, {
	dev = { patterns = jit.os:find("Windows") and {} or { "abeldekat", "lazy.nvim" } },
	root = root .. "/plugins",
})

With this saved as minimal.lua and your lazy.nvim cloned to ~/projects/lazy.nvim, you can run

LAZY=~/projects/lazy.nvim nvim -u minimal.lua

(this will do a complete sandbox into a .repro folder in the current directory)

All this is adding is the AstroNvim repository at the v4 branch and imports the astronvim.plugins plugin spec which can be found here: https://github.com/AstroNvim/AstroNvim/tree/v4/lua/astronvim/plugins

It seems to be having trouble properly loading these modules and never gets to loading the specs in

[abeldekat]

Just to make sure, a quick response:
In ~/projects/lazy.nvim, could you switch to the main branch?
Is your setup working in that case? I can look into that tomorrow....

[mehalter]

Yup :) it is working perfectly on the main branch as well as the current tagged release. Thanks for looking into this and all your help!

[mehalter]

@abeldekat I put together a much simpler case that breaks:

here is the repo: https://github.com/mehalter/test_lazy/tree/main

Here is the minimal.lua that can be run with nvim -u minimal.lua:

local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
	vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
	vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath })
end
vim.opt.rtp:prepend(vim.env.LAZY or lazypath)

-- install plugins
local plugins = {
	{ "mehalter/test_lazy", import = "test_lazy.plugins" },
}
require("lazy").setup(plugins, {
	dev = { patterns = jit.os:find("Windows") and {} or { "abeldekat", "lazy.nvim" } },
	root = root .. "/plugins",
})

[mehalter]

it breaks the second I put { "test_lazy", lazy = false } in the spec in this case

[mehalter]

I was able to change AstroNvim to AstroNvim/AstroNvim in the plugin spec here: https://github.com/AstroNvim/AstroNvim/blob/v4/lua/astronvim/plugins/_astrocore.lua#L4

and it seems to be loading now!

[mehalter]

@abeldekat I have gone ahead and done some more testing now that I now how to get around the previous bug and I can't verify that it's actually not considering dependency specs of disabled plugins at all. I have made a new branch on this test_lazy repo that isn't working correctly:

local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
	vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
	vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath })
end
vim.opt.rtp:prepend(vim.env.LAZY or lazypath)

-- install plugins
local plugins = {
	{ "mehalter/test_lazy", branch = "dependency_check", import = "test_lazy.plugins" },
	-- { "telescope.nvim", enabled = false }, -- COMMENT TO DISABLE TELESCOPE
}
require("lazy").setup(plugins, {
	dev = { patterns = jit.os:find("Windows") and {} or { "abeldekat", "lazy.nvim" } },
	root = root .. "/plugins",
})

When you run this with nvim -u minimal you should see that you get an error of an improper config value for gitsigns, this is added as a dependency to Telescope. If you uncomment the line in minimal.lua to disable Telescope, upon running nvim -u minimal.lua you still get the error even though Telescope is disabled. This should not actually be loaded and passed to gitsigns since those options are set as dependencies.

Here is a link to the repo+branch for that plugin test: https://github.com/mehalter/test_lazy/tree/dependency_check

[abeldekat]

@mehalter,

Fixed! I handled the reference in top-level plugins (ie "AstroNvim" vs "AstroNvim/AstroNvim) incorrectly.
LazyVim in its plugins always fully qualifies repos. In my personal config I do this as well, but I might change that now that I know of its usefulness...)

Thanks! I appreciate your effort to provide me with reproducable configs.

[abeldekat]

Hi @mehalter,

I committed a new version with some refactoring and one change that will affect the "astrocommunity" plugin.
community.lua now contains:

"AstroCommunity/AstroCommunity"

When using this version, that should be changed into:

{ "AstroCommunity/AstroCommunity", import = ""}

[abeldekat]

edit... the text below is not applicable anymore

It's a performance gain that is not necessary...) I can easily revert it. In my mind, it marks the plugin as "to be used for import".
Let me explain...

Parsing the spec now has the following phases:

1. Gather all top level spec instances, including the imports configured in imports=, or as configured in any imports= that follow.
2. Mark any of those specs as optional, conditional or disabled
3. The "rest" is active, proceed as usual... Merge and parse dependencies.

Table Spec.plugins contains the results of step 3.

In step 1 I am collecting the specs in the order provided using a list. I do not need the table in Spec.plugins, at first glance.
However, this table is used when doing imports by lazy.nvim's loader, when collecting paths. Thus, in phase 1, plugins must be set on Spec.plugins as well. Before phase 3 starts, that table must be reset in order to enter the normal flow of operation.
I discovered that in phase 1 only the "distro" plugins really need to be set in Spec.plugins. The relevant code gains purpose.

The number of paths lazy.nvim encounters when doing an import is now much smaller.
The alternative I applied before this last change: Add all first occurences of plugins to Spec.plugins in phase 1.

[abeldekat]

Your comment triggered some doubts, so I reverted that change.
The performance gain is not worth it, and it breaks the specs for the user.
Most notably one I overlooked:

require("lazy").setup("plugins")

is the same as:

require("lazy").setup({{import = "plugins"}})

Now, the import key can be configured as usual.
I tested it with your examples.

Best regards!

[mehalter]

That sounds awesome! I just did a test with the dependency_check branch and it still seems to be erroring because the dependency is not being ignored 🤔 Did you make sure to try the code that I sent with and without the comment to see how it breaks?

local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
	vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
	vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath })
end
vim.opt.rtp:prepend(vim.env.LAZY or lazypath)

-- install plugins
local plugins = {
	{ "mehalter/test_lazy", branch = "dependency_check", import = "test_lazy.plugins" },
	{ "telescope.nvim", enabled = false }, -- UNCOMMENTED TO DISABLE TELESCOPE
}
require("lazy").setup(plugins, {
	dev = { patterns = jit.os:find("Windows") and {} or { "abeldekat", "lazy.nvim" } },
	root = root .. "/plugins",
})

This minimal.lua still doesn't seem to work with the same description as before (#949 (comment))

[abeldekat]

Strange! What is your latest commit in the local branch of lazy.nvim? Should be 312479c.
Also: Did you wipe the .repro folder?

[mehalter]

ah I am actually just a doofus.... sorry! It seems to be working well! :D I think this is a really great improvement!

[abeldekat]

@mehalter,

I think my approach limits lazy.nvim too much. The functions a user can write using keywords "cond, enabled" used to be called when lazy.nvim has a complete picture of its plugins. This should not be broken.

Today, I realized how to "repair" active plugins after lazy.nvim has done its normal parsing. The changes to the code are much simpler.

I expect no breaking changes, except for the effect your idea has anyhow. Users might already have expected this new behaviour to exist as the idea is very intuitive in my opinion.

This is the commit message:

Plugins that are unused currently can still contribute to
active plugins by the specs added to their "dependencies" field.

This commit adds functionality to the "parse" method,
repairing only the active plugins that could be affected by the
aforementioned specs.

This can be achieved by maintaining an extra administration table
containing a copy of each added plugin instance.

I created a new branch

I am hoping you can find the time to test the new code! I tested your scenarios.

[mehalter]

This sounds like some incredible improvements and I agree with all of the thoughts and conclusions regarding the approach and solution. I just did some tests with my test cases and it seems to work perfectly! I can dive a bit deeper into this over the next few days and do some more testing/verify some new cases. But this really seems like steps in the right direction! ❤️

[mehalter]

For reals, I cannot show you enough appreciation for the time you are spending on this and all of your help!

[abeldekat]

No thanks needed, I very much like doing this. Also, the topic is completely related to my previous pull-request. I do would like to thank you for the feedback and the testing, and look forward to any further results.

Let's hope Folke shares (some of) our enthusiasm once he returns from his vacation.

[abeldekat]

Hi @mehalter ,

FYI: I pushed new commits.
These are only about better naming and performance. A test has been added.

[mehalter]

Awesome! Sorry for being away a couple days. It'll take a look!

[abeldekat]

Just for reference: I searched the issues and found one expressing the idea.

[mehalter]

Oh man, that is a good issue. I will be curious to see what folke thinks of your proposed changes. I definitely think they are an improvement as I keep seeing this pop up in small ways in the issues people open. I didn't get the impression that folke disagrees with the approach from the response to the issue you linked even though he closed it as unplanned. It seems like he just said that's not currently supported.

I really think the changes you put together are some pretty stellar work for reals and I hope that folke agrees :)

[abeldekat]

If you have the time, would you "scan" the comments in this PR.
As it turns out, that PR might be a good illustration of the usefulness of this discussion's concept...

[abeldekat]

... I see you have already done so...)

[mehalter]

hehe I have 😂 thanks for @ ing me previously in the discussion. I've been following the discussion. It is a great illustration of the use case for sure! Also sounds like we have some more supporters :)

[abeldekat]

thanks for @ ing me previously in the discussion

Of course. As I am concerned, before knowing about the existing closed issue, you initiated, expressed and supported the idea! Thanks a ton.

[abeldekat]

Hi @mehalter

Your responses and the feedback from the review in this PR increase my confidence.
I think a formal PR can be submitted. Do you agree?

If so, the following questions come to mind:

1. For a plugin, regarding its individual spec fragments before they are merged, do you like the term: "contributions"? I also encountered "instances", which I find a little confusing, as the final spec of the plugin is the most important instance to reason about.
2. The title could be the topic of this discussion. I also considered: "Only active plugins contribute to the final spec."
   I think the title of this topic is better, more specific.
   However, maybe this fragment can be improved: Given a disabled plugin. The word "disabled" can be associated with lazy.nvim's enabled keyword. The PR discards the contributions of all unused plugins, either being conditional, enabled or completely optional.
3. I think the PR should contain some thoughts on the probability these changes break existing code. Your input would be very helpful.

[abeldekat]

Hi @mehalter,

I created the PR. I would appreciate your feedback!

[mehalter]

Awesome! I'll be back from a bit of travel at the end of August and I'll be able to check it out in detail :) a quick glance looks great tho!

[folke]

I've been working on a refactor of how plugin deps are calculated in lazy. You can find the PR here #1058

The biggest change is similar to @abeldekat's PR where plugin spec fragments from optional and disabled plugins will no longer be included in the final spec.

This actually simplifies dealing with disabled plugin deps quite a bit.

As far as I can see eveything works as expected, but if you have any feedback, let me know :)

[abeldekat]

That's great!
Calculating the handlers only for enabled plugins could be a performance benefit.
I also like the way you access the fragments: by number instead of string.

[folke]

handlers used to be calculated before disabling, so that would no longer be correct when removing fragments. Should all work now though :)

The fid ids made things a bit simpler for sure.

[abeldekat]

Indeed!
I discovered a new warning:
plugins.lua, line 328, undefined param is_dep:
---@param is_dep? boolean

[abeldekat]

It's a substantial improvement. Do you consider this a breaking change?

[folke]

Not really. For most people nothing will actually change. And for those where it does change, then the new way is probably what was intended anyway.