WIP: Add MANY type annotations 🚀

[bew]

Hola @L3MON4D3, it's been a while o/

I've recently updated my neovim to 0.11 & reworking my config a bit, and since the big PR #1353 that started adding type annotations & DOC.md generation, I didn't touch my snippets at all...
The nvim upgrade was the nudge, and I started replacing the few type annotations I had manually crafted in my config, replacing them with the ones from the plugin!..
...
And then I kinda went down the rabbit hole 🤪
Porting documentation, reading the codebase while adding a few types for the things I understood, and adding MOAR types....

👉 So here is a first draft of ~1k LoC of doc additions & a few small reworks 🚀🚀

It took a few iterations of codebase exploration to kinda understand the various relationships between the different kind of snippet, node, and what 'layer'/'stage' of the snippets definition process I was on

I'm still far from understanding everything, and I might have made some mistakes, put some fields in the wrong class or failed a few inheritance relations, but I think it's pretty good for a start!

There are still many things I saw and could work on / refactor with your permissions, but I got a
good chunk going on and I'm quite happy with it!

Note

I left a few FIXME/IDEA in the PR with questions for you

Warning

I didn't test the DOC.md generation at all, I only focused on the Lua-side of things

Some things I typed/documented

• Node, InsertNode, FunctionNode, DynamicNode, ChoiceNode, with (most of) their internal fields ✨

• Snippet, SnippetNode & most of the internal snippet-initialization functions (their input opts & their normalized types)

  I managed to build a sort of hierarchy of classes that when combined build the final Snippet & SnippetNode types (with extra fields if needed), I'd need careful review of this part, I tried to follow the calls and build the structures but I might be wrong here and there..

  I know there are a few fields missing but they're kind of overwelming by their number, and the
  lack of internal documentation in such a big codebase 😅

• MultiSnippet, SnippetString

• some util functions here and there

• fmt's main functions

• AbsoluteIndexer, KeyIndexer, NodeRef

Other things

• moved copy3 impl to util, to avoid duplication between snippet.lua & fmt.lua
• rewrote the events.lua & types.lua for nicer typing without duplication

---

👉 In the end, most core files type-checks correctly, with only a few warnings that make sense and
that would need to be addressed (some of them challenge the class hierarchy in place, and I made a
few comments on ideas to improve the situation)

As for where to go with the types design, I think the codebase would benefit from splitting the Snippet class into multiple classes to reflect the various stages of use (barebone for internal use, snippetnode, snippet, expandedsnippet, (..?)). For the actual type annotations, maybe you'd like to have a public/private interface to avoid exposing the huge internals of the nodes to the users 🤔

I was also quite surprised that you used Node:new both to create new node types & new node instances, would you welcome some changes around this to segregate the types/functions more by use-cases?
(this can probably come later though)

Note

This PR targets the self-dependent-dNode branch, since this is the branch I'm on normally.
❔ I'm wondering, do you think we could merge it soon or do you still have planned work on it?
I'm asking because otherwise I'd need to rework some parts of this PR to be able to rebase on master 👀

Let me know what you think, and how you'd want to tackle this!
I'm available to do a day/night pair-programming/review session if you want 🙃

[L3MON4D3]

Hey bew, this sounds amazing, I'm happy you got nerd-sniped like that :P

It's pretty late for me right now, but I'll take a look at this first thing tomorrow and just answer the few things I can without looking deeper into your changes :)

As for where to go with the types design, I think the codebase would benefit from splitting the Snippet class into multiple classes to reflect the various stages of use (barebone for internal use, snippetnode, snippet, expandedsnippet, (..?)).

Oh, I totally agree here, the decision to make all of those the same thing was just because it was easy to incrementally cram more stuff into that one lua-table, but if I were to make a top-down decision right now, those'd have to be separated.

For the actual type annotations, maybe you'd like to have a public/private interface to avoid exposing the huge internals of the nodes to the users 🤔

Having a split there sounds pretty nice, is it possible to do so via LuaCATS?
If not, it'd be enough to just not put the internal stuff into DOC.md :)

I was also quite surprised that you used Node:new both to create new node types & new node instances, would you welcome some changes around this to segregate the types/functions more by use-cases?

Oh, I think that's a good idea, this is also something I've first written in 1048cdf (or even earlier 👀), more than four years ago, and not really questioned since 😅

This PR targets the self-dependent-dNode branch, since this is the branch I'm on normally.
❔ I'm wondering, do you think we could merge it soon or do you still have planned work on it?
I'm asking because otherwise I'd need to rework some parts of this PR to be able to rebase on master 👀

Nah, I'll look into merging it, that band-aid will have to come off anyway, this is a good reason to finally do that.
I'll cut a release before the merge because I'm still a bit unsure everything will work smoothly, but here's hoping 🤞 :D

Speaking a bit more on changes to luasnips' internals: while self-dependent-dNode already contains a few larger changes, I plan on making more drastic ones, up to a point where we'd (internally) only have textNode, insertNode, dynamicNode and snippetNode, which should be enough to represent any tree of nodes and emulate the behaviour of choiceNode, functionNode and restoreNode.
My point with this tangent is that I just wanted to make you aware that I'm eventually planning to rip out/rework large paths of luasnips internals, so best focus on the user-facing side of things, that's unlikely to change much :D (OTOH, you know how long self-dependent-dNode has taken to be in a good state, and anything larger in scope will likely be on a similar time-frame xD)

I'm available to do a day/night pair-programming/review session if you want 🙃

I'd be happy to handle this similarly to the last PR we did, that was very pleasant :D

[bew]

Take your time! 🙏

Really happy that you'd be okay with making some structural changes! ✨

For the actual type annotations, maybe you'd like to have a public/private interface to avoid exposing the huge internals of the nodes to the users 🤔

Having a split there sounds pretty nice, is it possible to do so via LuaCATS? If not, it'd be enough to just not put the internal stuff into DOC.md :)

Hmm actually maybe not with LuaCATS alone 👀
But it could be possible with a ~hidden _ or _private attr containing all private state for example.
It's true that we can selectively generate stuff in DOC.md but I'm mostly thinking of in-editor luals documentation & eventual completion suggestions..

Nah, I'll look into merging it, that band-aid will have to come off anyway, this is a good reason to finally do that. I'll cut a release before the merge because I'm still a bit unsure everything will work smoothly, but here's hoping 🤞 :D

Oh yeah 🚀

I'm eventually planning to rework large paths of luasnips internals, so best focus on the user-facing side of things, that's unlikely to change much :D

I personally like having types when refactoring, because the LSP is smarter and can immediately show you where things are used and how without having to hold everything in your head, and can show the impact of changes with diagnostics..
And it makes the codebase more approachable so to me it's always a net positive even when changes are planned 🤔

---

For the review, I think the most important ones are in node.lua & snippet.lua
And I'll probably split out small snippets of refactors / docs that don't depend on everything else in separate PRs

[L3MON4D3]

Take your time! 🙏

Always :P

But it could be possible with a ~hidden _ or _private attr containing all private state for example.

Puuuh, that does not sound appealing, nodes have like >20 fields/functions that are not supposed to be used by users.
Oh, maybe we could annotate a different class at places where the nodes are passed to users?
e.g. (from the dynamicNode update-function):

tmp = self.fn(
    effective_args,
    self.parent ---[[@as LuaSnip.PublicAPISnippetNode]],
    old_state,
    unpack(self.user_args)
)

and then LuaSnip.PublicAPISnippetNode only has the public subset of fields the real snippetNode has. We'd have a fair bit of duplication, but users aren't exposed to the internals :)

I personally like having types when refactoring, because the LSP is smarter and can immediately show you where things are used and how without having to hold everything in your head, and can show the impact of changes with diagnostics..

Ah, that's true, I was thinking more of reimplementing the internals from scratch than an incremental change. There are lots and lots of things we don't really need anymore/have better ways of doing (for example absolute_insert_position was my first attempt to get dynamicNodes to reference nodes outside its snippetNode, but key is just way better for that).

For the review, I think the most important ones are in node.lua & snippet.lua

👍

Taking a look now 👀

[L3MON4D3]

Ah, you can put the relative path to the DOC.md in here, so the link also works when viewing the file ;)
([Node Reference](../../../DOC.md#node-reference), +- a .. :D)

[bew]

Done in 02cabf1

[L3MON4D3]

Ah, snippetstring_args is a recent addition in self-dependent-dNode: it's a boolean which, when set, makes the dynamicNode pass snippetStrings instead of string[] as the first argument (snippetStrings are text copied from the buffer, with snippets preserved, so you can do stuff like :gsub on a nested snippet)

A bit of a problem with that the signature of LuaSnip.DynamicNode.Fn changes based on that boolean.. Can we properly represent that or can we only do something like args:(string[]|LuaSnip.SnippetString)?

[bew]

yeah it should be possible using @overload I think

[L3MON4D3]

Oh that sounds like a good substitute 👍
I don't know if it'd even be possible for luals to track which dynamicNodes have snippetstring_args set and which don't, so overload may be as precise as we can get here

[bew]

yeah, the @overload would only help user code, to have the immediately passed callback function have the correct type, but the internal code would have zero knowledge of which node has what (like at runtime).
And the types can still break (show the (opt1|opt2) in callback) if their code is a bit generic functions calling the node functions, and don't want to deal with overloads in their config...

[bew]

But it could be possible with a ~hidden _ or _private attr containing all private state for example.

Puuuh, that does not sound appealing, nodes have like >20 fields/functions that are not supposed to be used by users.

Actually I noticed there is a @field protected thing in luals, would be worth trying!
As for function we can simply make them start with an underscore, to do like in Python with its convention of _foo is protected and isn't part of the public interface 🤔

[L3MON4D3]

Yeah that's a good question 👍
I think it's not possible to document free-standing functions like that with LuaCATS :/
For reference:
EmmyLuaLs/emmylua-analyzer-rust#779
LuaLS/lua-language-server#1456

I think the current way of documenting these callbacks in the dynamicNode/functionNode signature is... fine, but not very elegant :/

[L3MON4D3]

Settings hover.expandAlias in lua-language-server to false makes this work pretty well

Honestly, setting that does not seem like a bad idea at all 👀 The only annoying bit is that it doesn't seem like one can jump to definition from the documentation window :/
(and obviously that not everyone will want to set this only to make this annotation work :D)

[bew]

Thanks I added it to my config 👍 (and removed this comment)

👉 Could you maybe add this tip into a kind of editor support section of the DOC ?
(near the other one about completion 🤔 )

[L3MON4D3]

Mhmm, sounds reasonable 👍

[L3MON4D3]

Actually I noticed there is a @field protected thing in luals, would be worth trying!

Oh, yeah that sounds good!

As for function we can simply make them start with an underscore, to do like in Python with its convention of _foo is protected and isn't part of the public interface 🤔

True, that would work.. I guess what makes me hesitate is that almost everything is internal xD
The public API of nodes is essentially only :get_jump_index and :get_buf_position.

[L3MON4D3]

I think we can omit the gifs, they can be added via code in DOC.md (or just by putting them into a separate section that gives more of an overview of insertNode, compared to this detailed documentation)

[bew]

or just by putting them into a separate section that gives more of an overview of insertNode, compared to this detailed documentation

I like this idea, to split the reference-level docs from the guide-level docs.
In the meantime I'll do a sort of hybrid but'll add a note for this somewhere

[L3MON4D3]

Yes, well put 👍

[bew]

I added the rest of the insert node docs in 1641838
The DOC.md generator will need to be updated but I 'included' the gifs with a line like <demo-gif:SomeID> 🤔

[L3MON4D3]

I think including this in luals-mdgen should be straightforward, and the syntax looks fine (it's even being highlighted for me, what is it for in regular markdown?)

[bew]

(it's even being highlighted for me, what is it for in regular markdown?)

FYI: In markdown it's an explicit link

[L3MON4D3]

Ahh that's pretty cool 👍
Check out L3MON4D3/luals-mdgen@f4f134b, it implements almost exactly what you suggested (only <media:SomeID> to make it a bit more generic).

These media-links are ignored if their target is not specified, so we no longer need the whole <panvimdoc-ignore> thing, we can just not set media_mapping when generating the markdown for the vimdoc :)

[L3MON4D3]

I don't quite get at what point a snippet is a BareInternalSnippet.. In my mind there is Addable for things that can be put into add_snippets, Expandable for stuff that has :matches and condition and the like, and then there is ExpandedSnippet for when the snippet is expanded.

As far as I remember snippet, dependents_dict, child_snippets, static_text, and indentstr are only used by snippets once they are expanded, maybe ExpandedSnippet is a better place for them? (And maybe it'd be a good idea to initialize them in trigger_expand?)

[bew]

I had to make something for the return type of _S, which is used in different places for defining different types.
And it's made to have the common fields for all other classes that are derived from it.

It's used as the common base for Snippet & SnippetNode:

• ---@class LuaSnip.SnippetNode: LuaSnip.BareInternalSnippet, LuaSnip.NormalizedSnippetNodeOpts
• ---@class LuaSnip.Snippet: LuaSnip.BareInternalSnippet, LuaSnip.NormalizedSnippetContext, LuaSnip.NormalizedSnippetOpts, LuaSnip.Addable

Those two don't share all the same fields nor the same parent classes, so we need a kind of base class for all their common fields 🤔

---

As far as I remember snippet, dependents_dict, child_snippets, static_text, and indentstr are only used by snippets once they are expanded, maybe ExpandedSnippet is a better place for them? (And maybe it'd be a good idea to initialize them in trigger_expand?)

Interesting, I'll have to explore that!

[L3MON4D3]

I had to make something for the return type of _S, which is used in different places for defining different types.
And it's made to have the common fields for all other classes that are derived from it.

It's used as the common base for Snippet & SnippetNode

Ahh I see.. this seems fine 👍
So far I'd thought of snippet as a specialization of snippetNode, but it's possible that, rigourously, there are some conflicts there 😅

[L3MON4D3]

👍
Splitting Snippet's responsibilities is a really good idea, it definitely has the weirdest lifecycle out of all nodes.

[L3MON4D3]

You mean this should only be defined on the types used at runtime? I agree :D

[bew]

I rebased on master since you merged #1137 (wooo 🚀)

How do you want to handle me fixing things incrementally?
I usually like to leave the commenter resolve each of his comment (so you can check they were fixed properly), and I can just put a done comment to notify that this one can be checked.
But I can work differently 🤔

[L3MON4D3]

I rebased on master since you merged #1137 (wooo 🚀)

Perfect, let's hope all goes well 🤞 :D

How do you want to handle me fixing things incrementally?
I usually like to leave the commenter resolve each of his comment (so you can check they were fixed properly), and I can just put a done comment to notify that this one can be checked.

This sounds great, that way we both know what's been written/verified 👌

[L3MON4D3]

Funny diff :D
It's used in put_intial, it's the text that will actually appear in the buffer when the snippet is expanded, and it is updated to represent the last text that the user typed into this node (via :store())

[bew]

I also kinda went overboard in 68b7376 & d2b494a as I rewrote the condition objects..
First to fully type & document them, second because I started doing some custom conditions in my config, composed together and I REALLY wanted a much more explicit & readable way to compose condition objects..

At the moment it's all in this PR (so my config doesn't break 😬), but I'll definitely move those out in a separate PR later on!
.. And maybe another PR to suggest adding my custom conditions, which could really be useful for everyone 🤔

[bew]

not sure about why these are possible to specify in the snippet opts, is it for backward compat?

[L3MON4D3]

Yeah exactly, they used to be in that table, and until now I don't think there's been a big reason to actually remove them. Best put something about them being deprecated in there? Unfortunately @deprecated only works for functions 😢

[L3MON4D3]

First to fully type & document them, second because I started doing some custom conditions in my config based, composed together and I REALLY wanted a much more explicit & readable way to compose condition objects..

Ah, as opposed to the slightly nebulous and ad-hoc operator-overrides? That sounds good :D

At the moment it's all in this PR (so my config doesn't break 😬), but I'll definitely move those out in a separate PR later on!
.. And maybe another PR to suggest adding my custom conditions, which could really be useful for everyone 🤔

This also sounds great, I guess I'll ignore that file for now, and then do a proper review of it in that separate PR? Definitely chuck your conditions in there, it really can't hurt to have more :)

[L3MON4D3]

I actually don't know if it is even used anywhere.. it was the first idea for referencing a node that is not a sibling (so no relative jump-index possible).
Checking the code, I can see no references to it, best ignore it for now/rip it out in a separate commit :)