refactor(wasm)!: make current*, is*, and has* methods properties

[verhovsky]

This is a breaking change to make the Wasm and Node bindings more interchangeable

Closes #2195

[maxbrunsfeld]

@verhovsky Just curious - do you have thoughts on which interface makes more sense? We should definitely make the two interfaces consistent, but which interface should we change, the Node one, or this one? From an idiomatic JS perspective, when should we use getters and when not?

[maxbrunsfeld]

I'm fine with making breaking changes, but I want to be open to making breaking changes in the other direction (to the Node binding) since we're already making breaking changes there due to the NAPI stuff. I just want to check in about which methods make sense to be getters.

One of the principles I was going for with the interface was to make the API feel somewhat like the most standard JS API for accessing trees, which is the web DOM APIs. I remember the DOM uses a lot of getters, but I don't remember the detailed shape of the APIs. Curious if you (or other folks who use this web binding) have thoughts.

[verhovsky]

The way this PR is, we'll need breaking changes in Node as well, to isMissing() and hasChanges() and hasError(). Right now in the Node bindings we have

      isNamed: boolean; // <- not a function
      hasChanges(): boolean;
      hasError(): boolean;

      isMissing(): boolean;
      readonly currentNode: SyntaxNode;

      readonly currentFieldName: string;

and in Wasm

      isNamed(): boolean;
      hasChanges(): boolean;
      hasError(): boolean;
      isError(): boolean;
      isMissing(): boolean;
      currentNode(): SyntaxNode;
      currentFieldId(): number;
      currentFieldName(): string;

isNamed, currentNode and currentFieldName are incompatible and if we change hasChanges, etc. as in this PR we'll need to change it in Node too.

I think it makes sense for these to be properties because they don't change state or iterate the tree or perform any action really, just copy bytes from the given object from C into a JavaScript object and return it.

The DOM seems to consistently follow this too, things like Node.nextSibling or Document.hidden are properties. I don't see many functions with no arguments.

It doesn't make sense to me to make things that don't modify state in any meaningful way be functions. I could understand things that require traversing the tree being functions though. It's a minor annoyance to have to type () after currentNode, especially when it doesn't change any state. If tree-sitter was a pure JavaScript library then .currentNode would definitely just be the property it uses internally for tracking the current node and isError would probably just be a property of a node.

[ObserverOfTime]

I think has* (maybe is* as well?) make more sense as functions.

I could understand things that require traversing the tree being functions though.

This too.

[maxbrunsfeld]

Yeah, I agree with currentNode, currentFieldName, and currentFieldId being properties. For the rest, I could go either way. I'm ok with either methods or properties. It'll be good to have it consistent between Node and Wasm.

[verhovsky]

People seemed opposed to changing the other methods to properties on Discord so I undid that change so this PR can be merged. We will still have to change isNamed in the Node bindings to be a function to be consistent with the Wasm bindings.

But I'm still convinced that these things should be properties. For the most part they're just reading a value that already exists in memory, wrapping it in a JavaScript object and returning it.

tree-sitter/lib/src/node.c

Lines 464 to 466 in 7b96bd8

	bool ts_node_is_missing(TSNode self) {
	return ts_subtree_missing(ts_node__subtree(self));
	}

tree-sitter/lib/src/subtree.h

Line 222 in 2d652c9

static inline bool ts_subtree_missing(Subtree self) { return SUBTREE_GET(self, is_missing); }

tree-sitter/lib/src/subtree.h

Line 215 in 2d652c9

#define SUBTREE_GET(self, name) ((self).data.is_inline ? (self).data.name : (self).ptr->name)

None of them change state. The ones that do any sort of computation, like hasError barely do any (an if statement and checks if a number exceeds a threshold). That feels like a property to me. These are already properties in the Python bindings and everything else around these things in the JavaScript code is also already get properties, which kind of suggests this was just an oversight. We even have cursor.nodeIsMissing and .nodeIsNamed as properties already

tree-sitter/lib/binding_web/binding.js

Lines 577 to 580 in 7b96bd8

	get nodeIsMissing() {
	marshalTreeCursor(this);
	return C._ts_tree_cursor_current_node_is_missing_wasm(this.tree[0]) === 1;
	}

So we have cursor.nodeIsMissing and cursor.currentNode.isMissing(), which are equivalent... Making people type () to read a piece of data on an object is weird.

That being said, more breaking changes will be really annoying, and since it's not as necessary as changing .currentNode and friends, for the sake of backwards compatibility I'm okay with not doing it if people are against it.

[maxbrunsfeld]

Ok, I'm ok with either direction of change, to make the two bindings consistent.