Binding Inconsistencies

[topi314]

Hi, first of all I appreciate the effort to provide updated & maintained go bindings for tree-sitter. Previously I was using https://github.com/smacker/go-tree-sitter which I had to fork to get rid of all the included tree-sitter grammars & make major breaking changes to be able to work with it.

My objective is to have good looking syntax highlighting and for this I ported the highlight crate of the official tree-sitter rust bindings to go. I had major pain points with missing features with the old module which are now gone.

While porting my highlighting code to this module I found 2 things I am wondering about:

1. github.com/smacker/go-tree-sitter used uint32 in most places which seem to have been replaced with uint in this module except for: https://github.com/tree-sitter/go-tree-sitter/blob/master/query.go#L113 Is this intended or just an oversight?

2. From what I can tell the Node struct should be passed around as a pointer. There are 3 places where they are handled as values instead which makes dealing with nodes a little bit annoying in those cases:

   1. https://github.com/tree-sitter/go-tree-sitter/blob/5432ade78ad6bde797bcdaa272a5953eaba83971/query.go#L112
   2. https://github.com/tree-sitter/go-tree-sitter/blob/5432ade78ad6bde797bcdaa272a5953eaba83971/tree_cursor.go#L143
   3. https://github.com/tree-sitter/go-tree-sitter/blob/5432ade78ad6bde797bcdaa272a5953eaba83971/query.go#L809 Is there also any specific reason this has been done?

Besides this I haven't ran into any issues, thanks for this awesome work!

[topi314]

I also want to add that tree_sitter is not exactly an ideal package name. Ideally it would be either treesitter, sitter or something else. Resource: https://go.dev/blog/package-names

[amaanq]

1. not sure, I mostly copied the types from C/Rust accordingly, is it actually a problem?
2. Nodes are stack allocated and are owned, so no they should not be passed around as pointers.
3. It might be better to change the package name in the next major version.

[topi314]

1. this is not really a problem, just an annoyance since you have to convert the type when you want to use it. I just saw there is a note on QueryCapture with that this is a C-compatible struct so ig it makes sense for it to be a uint32 then.

2. okay cool, in this case there are some functions/methods accepting or returning pointers where I wouldnt expect them to:

   1. https://github.com/tree-sitter/go-tree-sitter/blob/5432ade78ad6bde797bcdaa272a5953eaba83971/tree.go#L25
   2. https://github.com/tree-sitter/go-tree-sitter/blob/5432ade78ad6bde797bcdaa272a5953eaba83971/tree.go#L31

in general shouldn't the methods except Node.Edit all have a value receiver instead of pointer receiver?

[amaanq]

1. this is not really a problem, just an annoyance since you have to convert the type when you want to use it. I just saw there is a note on QueryCapture with that this is a C-compatible struct so ig it makes sense for it to be a uint32 then.

Ah that was it, I wanted the layout to remain the same as the C struct so casting from C pointers would be okay, like here

2. okay cool, in this case there are some functions/methods accepting or returning pointers where I wouldnt expect them to:

Yeah, I agree. PR welcome :) none of these methods besides Edit mutate the underlying node.

[topi314]

I was digging the C api, and if a node is not found they return an empty node, do you think it would make sense to do this in go too instead of returning pointers which could be nil? possibly with an added Valid() bool/Empty() bool method

returning a (Node, bool) would work too but is often annoying to deal with

[amaanq]

Ah right. I guess we could add an IsNull method to mirror the C api. Does that work better?

[topi314]

After thinking about it a bit more, I think I personally would prefer a (Node, bool) method signature, that would be the clostest to rusts Option<T> and force people to handle absent nodes. It should also make it more clear that nodes can be absent compared to Node.IsNull()

This does limit the ability to chain methods returning childs, but how would the current api with nil nodes handle that? probably just panicing?

[amaanq]

yeah it would just panic, I'm not really a fan of returning a (Node, bool) tbh