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

Sources/Completion item: Store non-copyable data. #851

Open
L3MON4D3 opened this issue Mar 20, 2022 · 4 comments
Open

Sources/Completion item: Store non-copyable data. #851

L3MON4D3 opened this issue Mar 20, 2022 · 4 comments

Comments

@L3MON4D3
Copy link

Hi!

It looks like the data-fiels of completion-items is copied at some place, which can cause issues if data contains complicated structures eg. recursive tables.

The specific issue is with cmp_luasnip, where we currently (because storing a reference doesn't work due to copying) have to store what amounts to an iterator, which might be invalidated (see here), making it necessary to rebuild the entire cache, even though no items were changed.

I can see thre ways to solve this:

  1. preserve references and recursive tables while copying
  2. Don't copy/provide a way to store non-copied values in data
  3. Do some more work on luasnips side to provide keys that can be resolved to snippets

IMO 2 would be pretty nice, because there's no need to copy the snippets, at least in cmp_luasnip case.
I wouldn't be opposed to 3, but it would only solve this problem for cmp_luasnip, not for other sources (which might be enough, I'm not sure if any other source has a similar problem).
1 doesn't sound good, I don't think there's any cases where big, complicated structures should be copied, and it would probably worsen copy for simple tables.

I'm not sure if it's possible to do already (didn't find anything about it if it is 😅)

Thank you for your work on nvim-cmp ❤️

@hrsh7th
Copy link
Owner

hrsh7th commented Mar 20, 2022

I think the data field is expected as serializable and copied data (because the original protocol is LSP that protocol is communicate the server and client via TCP socket or studio.

Sorry. I might not understand the problem. Could you explain more easily?

@L3MON4D3
Copy link
Author

Sorry. I might not understand the problem. Could you explain more easily?

Yeah, of course.
cmp_luasnip is the source for luasnip, so snippets have to be expanded in source:execute, which means we need some way to find out which snippet a completion-item refers to.
The snippets are not serializable, so we currently pass an index into some table in the data-field of the completion item, but this is going to become more cumbersome soon (due to some changes in luasnip) + wasn't that nice to start with.

We basically need some way to reference a non-serializable snippet in the completion-item, which proves to be kind of hard because all of the completion-item seems to be copied.

I assumed that data is the only place in the completion-item where completion-source specific values can be stored, is that even correct?

@L3MON4D3 L3MON4D3 changed the title Sources: Don't copy data-table of completion-items Sources/Completion-item: Store non-copyable data. Mar 20, 2022
@L3MON4D3 L3MON4D3 changed the title Sources/Completion-item: Store non-copyable data. Sources/Completion item: Store non-copyable data. Mar 20, 2022
@smjonas
Copy link
Contributor

smjonas commented Mar 20, 2022

I assumed that data is the only place in the completion-item where completion-source specific values can be stored, is that even correct?

I think this is not correct, e.g. for cmp-nvim-ultisnips we use a custom snippet attribute for the completion item and it's working fine. So should be able to use arbitrary keys. I am not sure if that helps you for actual problem though ^^ No idea if everything in the item table is copied or only parts of it..

@L3MON4D3
Copy link
Author

I just tried it (which I could've done like 5 hours ago xD), seems like the entire completion-item is copied :/

I'm not certain if this really is something useful to completion-sources other than cmp_luasnip. If it isn't, and if it isn't trivial to add to cmp, we'll just implement some snippet -> key, key -> snippet-mechanism in luasnip and store keys.

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

No branches or pull requests

3 participants