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. go-tree-sitter/query.go

      Line 112 in 5432ade

      Node Node

   2. go-tree-sitter/tree_cursor.go

      Line 143 in 5432ade

      func (tc *TreeCursor) Reset(node Node) {

   3. go-tree-sitter/query.go

      Line 809 in 5432ade

      func (qm *QueryMatch) NodesForCaptureIndex(captureIndex uint) []Node {

      
      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. go-tree-sitter/tree.go

      Line 25 in 5432ade

      func (t *Tree) RootNode() *Node {

   2. go-tree-sitter/tree.go

      Line 31 in 5432ade

      func (t *Tree) RootNodeWithOffset(offsetBytes int, offsetExtent Point) *Node {

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

[topi314]

while I would prefer (Node, bool) I am fine with it being a pointer then.

After I spent some time thinking about Node.IsNull(), I think it might be a confusing API.

[topi314]

I ran into another issue with the QueryMatches.Next & QueryCaptures.Next methods.
While changing how the QueryCaptures get converted into go structs I also changed the type of Index to uint which I mentioned earlier.

See: #24

[ObserverOfTime]

I also want to add that tree_sitter is not exactly an ideal package name.

What about parsers? e.g. treesittersshconfig is not exactly an ideal package name either.

[topi314]

Maybe something like tssshconfig?
I don't think it matters too much for parsers since you only import them once while you import this package a lot.

[n-peugnet]

IMO even sshconfig is enough as the import path will leave no doubt that it is related to tree-sitter. But anyways, it is still possible to override it on the consumer side.