fmt and multiple key node

[uyha]

When the snippet below is expanded, only the last node is being replaced with actual value when typing, the first {answer} stays 42, is it possible that fmt automatically recognizes that the key is being reused and replaces it with rp? fmt("{answer} {answer}", {answer= i(1, "42)}

[L3MON4D3]

I think we're inserting a copy.

[uyha]

I'm not sure why a copy is used instead of a repeat, wouldn't a repeat be better in this case?

[L3MON4D3]

In this case yes, we could probably implement that as well, but I'm not sure it's the right thing to do: fmt is very simple and easy to reason about (although inserting a copy already doesn't 100% fit with that, reprospectively I would've preferred an error for duplicated keys), tacking more functionality on it does not seem right.

[uyha]

I see, my thinking was it is just kind of expected that it would allow reusing keys since (I think) that is the main use case for that, otherwise it provides very little benefit compare to just using indices. In case you don't think it should be implemented, then maybe it's reasonable to mention in DOC.md that this is not supported.

[L3MON4D3]

Reusing keys is allowed (okay they're copied which doesn't do much, I agree), what fmt does not do is generate new nodes. But that being your assumption is good to know, I'll mention it in the doc. Thank you :)

[uyha]

My wording was bad there, I mean they are technically allowed, but they do something wildly unexpected. I think saying that fmt doesn't generate new nodes in this case is not true, since it does generate new nodes, it just doesn't generate unmentioned types of nodes (I am being nitpicky here). I still think it's better to insert repeat nodes, since it helps alot in practice.

[L3MON4D3]

Okay, also bad wording on my part, "new nodes" -> "different nodes".

We could probably introduce duplicate->repeat-node as a setting for fmt (though it'll default to off since I'm not confident that there aren't configs which rely on the current behaviour).

[uyha]

The current behaviour is a bit nonsensical to rely upon in my opion (at least I don't see how you can do something useful with it but you'll never know), but it is awesome that we can opt in into the new behaviour.

[danielo515]

I wasted a couple of minutes until I decided this was maybe a bug in LuaSnip, thankfully I didn't invested a couple of hours 😝 ! IMO the expected behavior when people uses this is to generate mirrored insert places, so the current behavior of just inserting the initial value and not doing anything else is extremely confusing. Specially if you use as default value the key name itself (which I was doing as an example). This makes the named keys (which is my preferred method) very weak in comparison to just using indexes.

Also, something not mentioned in the docs is numbering. How are repeated nodes numbered? They are skipped from the count? Take a look at this example:

    fmt([[ fmt( {key} , {{ {key} = i({value}) }}) ]], { key = i(1, "key"), value = i(2, "value") })

It works, despite the node in the value key is pointing to the second position, but it appears in the third place. Does this mean that one of the {key} values is not being taken into account? I also find a bit confusing that I have to account for key positions when using named placeholders, it also defeats the purpose of using named insert points.

[L3MON4D3]

I wasted a couple of minutes until I decided this was maybe a bug in LuaSnip, thankfully I didn't invested a couple of hours :stuck_out_tongue_closed_eyes:!

:sweat_smile:

IMO the expected behavior when people uses this is to generate mirrored insert places, so the current behavior of just inserting the initial value and not doing anything else is extremely confusing.

Gotta say, I've come around a bit on this. It really isn't useful to just insert the same node, it'll likely just not work/lead to errors in the worst case, so I'm open to just do a breaking change here and make copy the default-operation, maybe the only one.

How are repeated nodes numbered?

Mhmm, we just insert a copy, so the actual snippet from that fmt-call would look something like this:

sn(nil, {
	t"fmt( ", i(1, "key"), t" , { ", i(1, "key"), t" = i(", i(2, "value"), ") })"
})

It sounds like this is also the snippet you're getting, what would you expect instead?

I also find a bit confusing that I have to account for key positions when using named placeholders, it also defeats the purpose of using named insert points

You mean the i(position), right? I can see how that is a bit annoying.. a better version that works more like a lsp-snippet (though at that point you could also just use ls.parser.parse_snippet) can be built by using __index to create insertNodes:

ls.setup_snip_env()

-- function so each metatable gets it's unique insert_indx, sharing it would be pretty bad.
local new_mt = function()
	local insert_indx = 1
	return setmetatable({}, {
		__index = function(t,k)
			-- used internally for some check, a bit annoying tbh.
			-- maybe we can change that check to a rawget.
			if k == "type" then
				return nil
			end
			-- k should be string here.
			local node = i(insert_indx, k)
			-- increase so we don't reuse positions.
			insert_indx = insert_indx + 1

			rawset(t, k, node)
			return node
		end
})
end

-- the formatstring may not contain numbered keys now, so no {} or {1}.
-- (insert_indx would be incorrect if the metatable is not the only source of positions)
ls.snip_expand(s("", fmt([[{key} = {value}]], new_mt())))

Not sure if we wan't to include this in fmt, if at all via explicit opt-in I think, I like the current behaviour of warning about missing args, which would be destroyed by this.

[atticus-sullivan]

In my opinion this metatable fiddling is pretty nice. Though if we want this to be used, we should hide this somehow as users inexperienced with lua will be unable to write this on their own. It should be possible to merge this with that function somehow (so numbered index -> i(num), string index -> i(insert_idx, key) and {} would work just like the numbered index I think). Maybe we could provide this as luasnip.extras.fmt.auto_gen_nodes (extras seams to be a good place, and I see no point in moving this into some fmt_utils file).

[L3MON4D3]

Hmm yeah, that would work :+1: It might be best to provide these behaviours separately though, so

• one option for number-key -> i(num) ({} are also numbered indices behind the scenes, so that also works)
• another for repeating the second occurence of any key (can be true by default, but we need a good error for nodes that can't be repeated, like functionNode)

I don't think we should include the key-to-insertNode-preset-text (even though metatables are cool :D) since it wildly modifies the behaviour of fmt (can only use nodes generated by it) and doesn't offer any value over using parse (the restriction could be worked around by inspecting the passed nodes, determining used jump-indices, sorting them and finding the free indices inbetween, but that sounds disgusting, and I'd like for extras to not rely on any internals on luasnip (my intention for them at least))

[atticus-sullivan]

doesn't offer any value over using parse

Fair enough ^^

another for repeating the second occurence of any key (can be true by default, but we need a good error for nodes that can't be repeated, like functionNode)

Maybe I missed this in the previous discussion, but how are we gonna get the node_index which shall be repeated (as node_index != index in the fmt table) without inspecting the corresponding node? (just can't think of any way without inspecting the node which shall be repeated right now. Then if we want to exclude e.g. functionNodes we need to look inside the node anyways)

Refers to:

I'd like for extras to not rely on any internals on luasnip (my intention for them at least)

[L3MON4D3]

Maybe I missed this in the previous discussion, but how are we gonna get the node_index which shall be repeated (as node_index != index in the fmt table) without inspecting the corresponding node?

Right, that's true.. Yeah so we'd need to at least add API to get that information. Well okay, with that added, we could also tackle the preset-text

[atticus-sullivan]

Well okay, with that added, we could also tackle the preset-text

:+1: The advantege I see over parse is that this way you can mix different kinds e.g. ("{user}: hello {world}", {user=functionNode(...)}, where user could be a function node resulting in the user name and world being a simple insertNode with world as default text (not that deep into parse (is there something in DOC.md?), but I think more complex function/dynamic nodes aren't possible with parse).

So basically fmt becomes a simpler (no direct way of adding e.g. choices with just the snippet string) parse but being more flexible (just being able to manually insert nodes). This development has advantages (fmt stays mighty, but becomes better usable/shorte snippet declarations), but also disadvantages (the usecases of fmt and parse become quite similar and its always weird having multiple things doing almost the same but only almost).

[L3MON4D3]

+1 The advantege I see over parse is that this way you can mix different kinds e.g. ("{user}: hello {world}", {user=functionNode(...)}, where user could be a function node resulting in the user name and world being a simple insertNode with world as default text

Yeah true, that's pretty comfortable, and in the end that's more important than some purity in our implementation.

(not that deep into parse (is there something in DOC.md?), but I think more complex function/dynamic nodes aren't possible with parse).

Yup, exactly, parse only handles lsp/textmate-syntax.

So basically fmt becomes a simpler (no direct way of adding e.g. choices with just the snippet string) parse but being more flexible (just being able to manually insert nodes). This development has advantages (fmt stays mighty, but becomes better usable/shorte snippet declarations), but also disadvantages (the usecases of fmt and parse become quite similar and its always weird having multiple things doing almost the same but only almost).

Mhmm, I agree with advantages+disadvantages, but in the end it's probably worth it. Also, parse is always justified as it's necessary for parsing snippets from lsp and loading vscode/snipmate-collections.

[uyha]

so, any update about this? I have some free time and would like try implementing this hoping it is wanted. Anything I should know before jumping in?

[L3MON4D3]

Oh awesome, as far as I'm aware nobody's working on it. Go for it :)

I think this is the most complicated part:

Maybe I missed this in the previous discussion, but how are we gonna get the node_index which shall be repeated (as node_index != index in the fmt table) without inspecting the corresponding node? (just can't think of any way without inspecting the node which shall be repeated right now. Then if we want to exclude e.g. functionNodes we need to look inside the node anyways)

For properly implementing this, we'd need a function to return the jump_index (formerly also insert-position/node_index) for any node. This is stored as pos for all nodes that have it, and is nil for those that don't, so it should be fine if jump_index(node) just returns node.pos, with nil <=> "node is not a jumpable node". That function can be defined in lua/luasnip/nodes/node.lua, source for fmt is in lua/luasnip/extras/fmt.lua, apart from that feel free to open a draft-pr and ask questions there (better than here I'd say)

[uyha]

Kinda over my head right now, but hopefully it makes more sense when i dive in the code

[uyha]

implemented by #665