fix(from_snipmate): extends

[uga-rosa]

This PR is incomplete.
If you use load to load all the snippets at once, this is fine, but if you use lazy_load, there is a bug where the snippets are loaded repeatedly when the filetype changes, resulting in duplicate candidates.
If you prepare sh.snippets as follows and open hoge.sh, then run :set ft=zsh, you should be able to reproduce the problem.

extends zsh
snippet test
    hi!

Furthermore, with lazy_load, if you open a file with ft=zsh without going through ft=sh, the snippet will not be added.

[uga-rosa]

I think I've solved the problem, but the WiFi in my room is not working, so the update will have to wait (this post is from my iPhone).

[L3MON4D3]

xD
Okay, I'll be looking forward to that 👍

[uga-rosa]

In the case of lazy, only look at extends first, otherwise, examine extends at the timing of parsing the snippet.
Since filetype_extend is supported by util.get_snippet_filetypes() in M._lazyload(), this should not be taken into account in the filter (loader_util.filetypelist_to_set()). This was causing duplication.

[L3MON4D3]

Ah, I understand.
What do you think of keeping the extends snipmate/snippet-library-local (so if snippets/cpp.snippets has extends c it would only include snippets from snippets/c.snippets)? I'd treat the extends more like a #include, eg. "use the snippet listed in that file here as well", not a global "extend the current filetype with this other filetype". That way the extends would also do something different than ls.filetype_extend, which IMO is a bonus.

[uga-rosa]

Oh I see, #include. That's easier to understand and use.
I'll try it.

[L3MON4D3]

TY, I'm glad we're on the same page 👍
lazy_load probably needs to be a bit more complicated as there might be both extends somefiletype (in filetype.snippets) and ls.fileteype_extend(filetype, {"somefiletype"}), which could lead to duplicate snippets if we don't keep track of which files were already loaded for a given filetype.
(The root problem, of course, is that sources have their own ways of extending filetypes, which we have to respect. Everything would be easier if those wouldn't exist :| )

[uga-rosa]

It took some design changes, but I was able to implement it. Reading files asynchronously was complicated and confusing, so I borrowed Path.read_file from that PR and used it. It takes about 700ms to load all the honza/vim-snippets without lazy_load, so we need to tell users.
I was also able to support nested extends. Example, typescriptreact -> typescript -> javascript.
WiFi is broken again, so I'll push tomorrow :'(

[leiserfg]

Time to start coding in your iPhone bro 😄

[uga-rosa]

It works fine for my own testing. (and WiFi is probably back on)

[leiserfg]

@uga-rosa could you profile the loading? Maybe when can find out what's what makes it "slow", you just have to be using luajit 2.1 (it comes with the neovim from the releases, is the default in debian, for arch it's in AUR) and do something like:

require("jit.p").start("10,i1,s,m0,G", "/tmp/output.log")
-- your function call here
require'jit.p'.stop()

And share your output.log .

[uga-rosa]

It's like this, isn't it?

[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;fix_node_indices 39
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;init_nodes 4
[string]:0;load;_load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;fix_node_indices 4
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet 3
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;parse_text 2
[string]:0;load;_load;load_snippet_file;parse_snipmate;startswith 2
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;parse_text;gsplit 2
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;to_line_table;split;gsplit;validate; 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;brackets_offset 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;I 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;to_line_table;split;gsplit 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;new 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;fn;parse_snippet;parse_text;gsplit;validate; 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;fn;I 1
[string]:0;load;_load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;fn;split;gsplit 1
[string]:0;load;_load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;init_nodes 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;fn;text_only 1
[string]:0;load;_load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet 1
[string]:0;load;_load;load_snippet_file;parse_snipmate;_parse;parse_snippet;fn;parse_snippet;parse_text;gsplit 1

[leiserfg]

[leiserfg]

@L3MON4D3 check this, maybe if you try it in your branch (the one for sync vscode loading) we can get more precise results (as we have more files there) but seems like fix_node_indices is a bottleneck.

[uga-rosa]

I'm loading my own snippets and honza/vim-snippets.

[L3MON4D3]

Oh holy.. fix_node_indices shouldn't be taking that long xD

[L3MON4D3]

@uga-rosa You also included the previously unsupported (iirc :D ) snippets/filetype/*.snippets, right?

[uga-rosa]

Hmmm? I think it supported {filetype}.snippets and {filetype}/*.snippets before.

[leiserfg]

BTW the flame graph has extra data, if you click it you can see the percent of each bar while hovering it.

[L3MON4D3]

Hmmm? I think it supported {filetype}.snippets and {filetype}/*.snippets before.

Oh, nvm then :D

[L3MON4D3]

BTW the flame graph has extra data, if you click it you can see the percent of each bar while hovering it.

It's only a picture for me 😅 But it's clear that fix_node_indices kind of drags everything down, which it definitely shouldn't.
I can investigate+fix what's going on there in my PR (when I get to it, in a bit), together with the on-expand-parsing, then we can first merge that and then this one (I think that'd be best, but not sure).

[uga-rosa]

It's only a picture for me 😅 But it's clear that fix_node_indices kind of drags everything down, which it definitely shouldn't.
I can investigate+fix what's going on there in my PR (when I get to it, in a bit), together with the on-expand-parsing, then we can first merge that and then this one (I think that'd be best, but not sure).

It was 57.35%. I'm curious...

[L3MON4D3]

Should we warn users that both a (in filetype.snippet) extends otherfiletype and ls.filetype_extend("filetype", {"otherfiletype"}) leads to duplicate snippets and the only fix is to change the .snippets (which might be non-trivial in cases of snippet-libraries, eg. vim-snippet)?

[L3MON4D3]

It's only a picture for me sweat_smile But it's clear that fix_node_indices kind of drags everything down, which it definitely shouldn't.
I can investigate+fix what's going on there in my PR (when I get to it, in a bit), together with the on-expand-parsing, then we can first merge that and then this one (I think that'd be best, but not sure).

It was 57.35%. I'm curious...

Damn, that's quite a bit...

[uga-rosa]

Should we warn users that both a (in filetype.snippet) extends otherfiletype and ls.filetype_extend("filetype", {"otherfiletype"}) leads to duplicate snippets and the only fix is to change the .snippets (which might be non-trivial in cases of snippet-libraries, eg. vim-snippet)?

Hmm. It would certainly be better to write it in the help.

[L3MON4D3]

I fixed the horrendous performance of fix_node_indices (and hopefully didn't break anything :D (update: did break something :( )), which makes parsing snippets much faster.

before:

after:

(with require("jit.p").start("10,i1,s,m0,G", "pre"/"post") and on master, but it should look similar here)
Would it be correct to assume that parse_snipmate took ~289ms and now takes ~142ms (eg samples * sample interval roughly equals runtime)?

[L3MON4D3]

I think the synchronous_load-PR can be merged tomorrow or so, @uga-rosa would you extend (haha get it :P) the existing snipmate-test suite with one for extends? (maybe just add extends somefile to the existing all.snippets, and make sure that snippets from somefile.snippets are present)

I'll be happy to help if you have any questions concerning the tests :)

[uga-rosa]

Is it something like this? Please let me know if something is wrong.

[L3MON4D3]

Jup, looks good 👍
Not at home rn, I'll look into merging everything when I get back

[L3MON4D3]

I accidentially merged an older version of this PR (didn't update my local branch before merging 🤦), but now all should be in order.

[L3MON4D3]

I'm sorry, nvm, I'm getting this error when calling the regular load(), but not with lazy_load():

E5108: Error executing lua ...cker/start/luasnip/lua/luasnip/loaders/from_snipmate.lua:151: bad argument #1 to 'ipairs' (table expected, got nil)
stack traceback:
        [C]: in function 'ipairs'
        ...cker/start/luasnip/lua/luasnip/loaders/from_snipmate.lua:151: in function '_load'
        ...cker/start/luasnip/lua/luasnip/loaders/from_snipmate.lua:155: in function '_load'
        ...cker/start/luasnip/lua/luasnip/loaders/from_snipmate.lua:172: in function 'load'
        [string ":lua"]:1: in main chunk

Somehow it doesn't happen in the tests.

I've got to go rn, could you look into it @uga-rosa?

[uga-rosa]

Ah, let add or {} for now.
I will look into it. I wonder why.

[L3MON4D3]

Thank you ❤️

[L3MON4D3]

Okay, I found the error, fixed with 1a0fac4

[uga-rosa]

Ah, I see, because scandir now returns the full path.
Ty.

[L3MON4D3]

Jup, that's it exactly.
Also, thank you for writing this addition 👍 👍