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....

:point_right: 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. :grey_question: 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 :upside_down_face:

[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 1048cdfec1a3cc406f1c5bcb5616e4953fe7238f (or even earlier :eyes:), more than four years ago, and not really questioned since :sweat_smile:

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 :crossed_fingers: :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! :pray:

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

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 :eyes: 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 :rocket:

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 :thinking:

---

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

:+1:

Taking a look now :eyes:

[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 :thinking:

[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]

Whoo boy, what a patchset :O

I think I've given everything a first pass, I really think you're spot on with most of the annotations, so that's already a great job :ok_hand:

I think pulling apart snippet into a few separate classes is a really good idea, that will also make what multisnippet/snippetProxy do more clear.

There are a few places where perfect annotations are not really possible, but I think as long as we cover a superset of valid states and are precise in the doc it should be fine.

Especially finding a perfect solution for the callback-function documentation would be great, but I don't think that's possible :/

Regardless, Thank You so much for this effort, it's a really great step :heart:

[bew]

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

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 :thinking:

[L3MON4D3]

I rebased on master since you merged https://github.com/L3MON4D3/LuaSnip/pull/1137 (wooo 🚀)

Perfect, let's hope all goes well :crossed_fingers: :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 :ok_hand:

[bew]

I also kinda went overboard in 68b7376c0883772cf6d3588ada2c880118fdda7d & d2b494a6b2af64b19d3ae46760502009a70d164e 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 :grimacing:), 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 :thinking:

[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 :)