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

Extension seems to be getting clobbered by other Telescope extensions #22

Open
prurph opened this issue Nov 24, 2023 · 8 comments
Open

Comments

@prurph
Copy link

prurph commented Nov 24, 2023

I'm trying to use telescope-luasnip.nvim with LazyVim, which bundles a few other telescope extensions. The issue I'm having is that :Telescope luasnip works exactly once, then subsequent invocations give the following error:

Error  21:39:19 msg_show.lua_error   Telescope luasnip Error executing Lua callback: vim/shared.lua:0: after the second argument: expected table, got nil
stack traceback:
	[C]: in function 'error'
	vim/shared.lua: in function 'validate'
	vim/shared.lua: in function 'tbl_extend'
	...scope-luasnip.nvim/lua/telescope/_extensions/luasnip.lua:74: in function <...scope-luasnip.nvim/lua/telescope/_extensions/luasnip.lua:73>
	...re/LazyVim/lazy/telescope.nvim/lua/telescope/command.lua:193: in function 'run_command'
	...re/LazyVim/lazy/telescope.nvim/lua/telescope/command.lua:259: in function 'load_command'
	...l/share/LazyVim/lazy/telescope.nvim/plugin/telescope.lua:108: in function <...l/share/LazyVim/lazy/telescope.nvim/plugin/telescope.lua:107>

My config is:

"benfowler/telescope-luasnip.nvim",
  config = function()
    require("lazyvim.util").on_load("telescope.nvim", function()
      require("telescope").load_extension("luasnip")
    end)

where on_load is used for other telescope etensions in LazyVim that work correctly. I've also tried a lot of different orders to ensure the extension isn't being loaded until after Telescope is setup, as well as not adding any explicit config at all and just letting the extension autoload but no dice. The fact it works properly once makes me think that's not an issue with it loading the first time, rather something corrupting/clobbering/overwriting it.

it seems like this extension sets a global and my guess is that somehow gets clobbered when other extensions are invoked. This is supported by the fact that :Telescope luasnip will give that error on the first invocation if I call any other Telescope picker first.

I'm certainly far from an nvim plugin or Lua expert, but I'm confused why M appears to be global here. That said, I tried renaming it and the issue persisted, so it doesn't seem to be a name collision. I haven't tried disabling other extensions yet because this one is less critical to my workflow and all of the others seem to work alongside along side each other without any issues. Any thoughts on how I might debug this? Thanks!

@strcat
Copy link

strcat commented Dec 4, 2023

I've the same problem and no solution.

@xorander00
Copy link

Not sure why M.opts is nil in luasnip_fn, even though it's set at the top-level of the module, but anyway, here are the changes I made to workaround it...

luasnip.lua @ line 68:
Changed:

M.opts = {
  preview = {
    check_mime_type = true,
  },
}

To:

local _opts = {
  preview = {
    check_mime_type = true,
  },
}
M.opts = _opts

luasnip.lua @ line 74:
Changed:

opts = vim.tbl_extend('keep', opts or {}, M.opts)

To:

local opts = vim.tbl_extend('keep', opts or {}, M.opts or _opts)

luasnip.lua @ line 177:
Changed:

M.opts = vim.tbl_extend('keep', optsExt, opts)

To:

M.opts = vim.tbl_extend('keep', optsExt or {}, opts or {}, M.opts or _opts)

@benfowler
Copy link
Owner

benfowler commented Dec 6, 2023

Hi; sorry I'm late to the party. I've been flat out with real life.

I'm not a plugin expert myself, I'm afraid. This was originally meant as a straightforward port of the Ultisnips Telescope extension to LuaSnip to scratch a personal itch, and has sprouted a few legs since. Issues may have crept in as we've found and fixed edge cases.

I looked OP's stack trace, and then noticed independently that M isn't local. The new configuration scheme came in with a recent PR to fix an unrelated issue, and nobody caught it in review. Sorry!

I myself had thought that the proper way to define modules was to declare M as a local and then return it to avoid polluting the global namespace -- that's certainly how I do it in my own personal config.

I tried out simply declaring M as local in that module, and it seemed to work fine for me.

Would somebody else like to give that a go too and then test it a bit, just to double-check this solution works?

EDIT: Spoke too soon! It broke snippet loading for me, and I have a report due. I'll test @xorander00 's solution now.

@benfowler
Copy link
Owner

benfowler commented Dec 6, 2023

I put @xorander00's changes into a branch - debugExtensionLoading - to ease testing, and opened a PR - #23 - to get some more review on it. It's holding up well for me so far.

Thanks again, @prurph and @xorander00 !

@xorander00
Copy link

I myself had thought that the proper way to define modules was to declare M as a local and then return it to avoid polluting the global namespace -- that's certainly how I do it in my own personal config.

This is what I do too and is a very common pattern in Lua. You've got it right, IMO.

I tried out simply declaring M as local in that module, and it seemed to work fine for me.

I completely missed that M wasn't local lol. That could have something to do with opts being nil in luasnip_fn, though I don't remember lifetime/scoping rules in Lua off the top of my head.

My workaround is pretty lame and just defensively falling back to empty tables + the locally declared _opts table. It would be better to figure out why M.opts is nil in luasnip_fn.

@benfowler
Copy link
Owner

Agreed -- I think this needs a bit more investigation.

I'll try and get to this shortly, as I've just gotten past crunch time, but if you've got time to dig in a little further, I won't stop you ;-)

@xorander00
Copy link

I reset my changes back to the latest commit and took a look again really quick. I changed M to be local and merged the M.opts {} assignment into the M initialization line. Seems to be working for me, but I've only used it for a couple of minutes and I have a bunch of stuff left on to todo-list for today.

If anyone else wants to try it and see how it goes for them, the patch is below. If a PR is needed, let me know.

diff --git a/lua/telescope/_extensions/luasnip.lua b/lua/telescope/_extensions/luasnip.lua
index 7eee771..05beaff 100644
--- a/lua/telescope/_extensions/luasnip.lua
+++ b/lua/telescope/_extensions/luasnip.lua
@@ -14,7 +14,11 @@ local conf          = require("telescope.config").values
 local ext_conf      = require("telescope._extensions")
 -- stylua: ignore end
 
-M = {}
+local M = {
+  preview = {
+    check_mime_type = true,
+  },
+}
 
 local filter_null = function(str, default)
   return str and str or (default and default or '')
@@ -65,11 +69,6 @@ local default_search_text = function(entry)
     .. filter_description(entry.context.name, entry.context.description)
 end
 
-M.opts = {
-  preview = {
-    check_mime_type = true,
-  },
-}
 M.luasnip_fn = function(opts)
   opts = vim.tbl_extend('keep', opts or {}, M.opts)

@zivarah
Copy link

zivarah commented Jan 6, 2024

It would be better to figure out why M.opts is nil in luasnip_fn.

Late to the party here, but I think the reason is that if any other extension also sets M as a global, and loads after this extension, it wipes out the global created here. I ran into this yesterday and tracked it down to orgmode using the same M = {} as a global, so if orgmode loaded after telescope-luasnip, I'd get this stack trace.

orgmode addressed this quickly once I reported it, and their fix was also to just do a local M.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants