Merge master into napi branch #129
Conversation
A Range contains: - startIndex - endIndex - startPosition - endPosition
…tree-sitter#63) * Fix the TypeScript definition of a Range A Range contains: - startIndex - endIndex - startPosition - endPosition * Add `bufferSize` and `includedRanges` to TS def The function signature of `Parser.prototype.parse` defined in index.js `Parser.prototype.parse = function(input, oldTree, {bufferSize, includedRanges}={})` accepts a third optional parameter. This commit reflects the signature in TypeScript definition.
* queries: initial implementation * queries: add predicates * test: setup query tests * package: add build script * query: display TSQueryError name instead of value * tree-sitter: update to latest * query_cursor: remove & move .exec to query.cc * query: continue implementation of .exec * test: adjust query_test.js to match current API * lint: remove unused module * node_methods: expose GetMarshalNodes * lint: add section comments * query: implement Query.matches * query: implement Query.captures * query: implement predicates filtering * query: make Query::GetPredicates warmer * query: fix predicates initialization * tests: unskip query.captures * query: implement range searching * query: remove Query.exec * query: use arrays for marshalling * node: fix tree-sitter#66 free transfer_buffer * tests: fix query_test labels * query: flatten intermediate array * query: dont transfer capture_count * unmarshalNodes: pass nodes as array * lint * query: remove unused values * query: avoid out-of-bound access
* Fix typo * tree-sitter.d.ts - define missing Parser class fields * tree-sitter.d.ts - define missing SyntaxNode typeId field * tree-sitter.d.ts - define missed TreeCursor currentFieldName field * tree-sitter.d.ts - define missed Tree printDotGraph method * tree-sitter.d.ts - define missed Query class and related types * tree-sitter.d.ts - fix Query.captures return type * tree-sitter.d.ts - sync query parameter names with wasm binding * fix types checking for node types
fix: prevent Nan::Utf8String destructor call due to out of scope
| const TSLanguage *language = static_cast<const TSLanguage *>( | ||
| GetInternalFieldPointer(value) | ||
| ); | ||
| const TSLanguage *language |
There was a problem hiding this comment.
@maxbrunsfeld this is the only thing I'm unsure about. I remember having issues loading my language when I originally made this branch, so I added to that language (somewhere, I forget) a special property _language which is a Napi::External with a pointer to the TSLanguage. I can probably remove this change, tests should still pass, but I do think there is probably some untested issue that I need to recover, unrelated to merging with master. Thoughts on me removing this change in this PR and deferring finding what the issue was and solving it to another PR?
There was a problem hiding this comment.
I need to refresh on how much of this should be generated, but apparently in my fork of tree-sitter-sql I modified the node binding module export to export the TSLanguage as a Napi::External for loading here. I did that here
There was a problem hiding this comment.
Is this what @maxbrunsfeld was talking about in #52 (comment)?
Unfortunately, we can't achieve true ABI-stability all in one fell swoop. This module has two binary interfaces that currently depend on Nan / V8 directly:
- Retrieving the
TSLanguage *pointers out ofLanguageobjects from language modules. Currently, the Node.js binding code that thetree-sitterCLI generates for individual languages are reliant onnanand the V8 ABI 😞 .
There was a problem hiding this comment.
Almost definitely that's it. Well I had a workaround clearly but it's worth discussing.
There was a problem hiding this comment.
What about the changes suggested in this comment: #52 (comment) ?
|
@MichaelBelousov node-tree-sitter depends on the superstring library which is also a NAN package. superstring is abandoned, but there's some people that are working on a fork: https://github.com/pulsar-edit/superstring and they're also working on porting it to Node-API https://github.com/pulsar-edit/superstring/tree/napi @mauricioszabo said on Discord that "We actually are moving to a WASM version of superstring because the move no N-API is going slower than expected". I guess I'm telling you this in case(/in hopes that) you're interested in helping out with that as well. |
I will at least take a look at what they're doing and figure out what the time commitment would look like. |
|
I tried using your code but it didn't work: Usually I've ran into |
I've only tried it on Linux. I'm on vacation without a computer until the 17th. I can fix it when I get back, I have a Mac mini to test on |
|
I was able to get it to start building after some minor changes diff --git a/package.json b/package.json
index da61a08..a1e11b0 100644
--- a/package.json
+++ b/package.json
@@ -17,16 +17,22 @@
"dependencies": {
"nan": "^2.14.0",
"node-addon-api": "git+https://github.com/nodejs/node-addon-api.git",
- "prebuild-install": "^6.0.1"
+ "prebuild-install": "^7.0.1",
+ "node-gyp": "9.3.1"
},
"devDependencies": {
"@types/node": "^14.14.31",
"chai": "^4.3.3",
"mocha": "^8.3.1",
"prebuild": "^10.0.1",
- "superstring": "^2.4.2",
+ "@curlconverter/superstring": "^0.0.3",
"tree-sitter-javascript": "git://github.com/tree-sitter/tree-sitter-javascript.git#master"
},
+ "overrides": {
+ "prebuild": {
+ "node-gyp": "$node-gyp"
+ }
+ },
"scripts": {
"install": "prebuild-install || node-gyp rebuild",
"build": "node-gyp build",
diff --git a/test/node_test.js b/test/node_test.js
index 8dadc3d..e66f6ce 100644
--- a/test/node_test.js
+++ b/test/node_test.js
@@ -1,7 +1,7 @@
const Parser = require("..");
const JavaScript = require('tree-sitter-javascript');
const { assert } = require("chai");
-const { TextBuffer } = require("superstring");
+const { TextBuffer } = require("@curlconverter/superstring");
describe("Node", () => {
let parser;
diff --git a/test/parser_test.js b/test/parser_test.js
index b45f1df..19ed1c7 100644
--- a/test/parser_test.js
+++ b/test/parser_test.js
@@ -1,7 +1,7 @@
const Parser = require("..");
const JavaScript = require('tree-sitter-javascript');
const { assert } = require("chai");
-const {TextBuffer} = require('superstring');
+const {TextBuffer} = require('@curlconverter/superstring');
describe("Parser", () => {
let parser;
diff --git a/vendor/tree-sitter b/vendor/tree-sitter
index c51896d..a62bac5 160000
--- a/vendor/tree-sitter
+++ b/vendor/tree-sitter
@@ -1 +1 @@
-Subproject commit c51896d32dcc11a38e41f36e3deb1a6a9c4f4b14
+Subproject commit a62bac5370dc5c76c75935834ef083457a6dd0e1but I get a new error: enjoy the vacation |
|
Thanks so much for working on this @MichaelBelousov. It seems like it's very close. Thoughts on two of the design questions:
|
|
I'm not sure if it helps, but I was working on this migration in parallel, before I found out that all grammars would suffer from the same problem and will not load on newer Electron versions anyway: pulsar-edit#1 |
|
Why won't they load on Electron? |
|
@verhovsky After Electron 13, all modules need to be "context-aware", meaning that they need to offer some guarantees of not keeping global "node context" objects. The problem is that none of the grammars are context-aware, and they all need to be. So even if I could finish the migration to N-API, all grammars would need to be converted... |
But the bindings for languages are both generated and versioned... so can't we just move up a major version and have the newer version generate incompatible bindings? The version check will still be ABI compatible ofc |
|
We use tree-sitter a ton in production and i would love to help get this merged so that tree-sitter can transition to the NAPI. @MichaelBelousov @maxbrunsfeld wondering if you two have some sense of the work that is required to get this PR merged. I really appreciate the work that both of you have put in to get this PR so close. |
I can do the work and provide a schedule if @maxbrunsfeld has a plan for getting it merged/reviewed |
|
I can help out too! It just so happens that @maxbrunsfeld has the best context on getting it merged |
|
Another issue is that unless this module is made fully context aware, as in, using I'm now stuck with deciding what to do... Whether I want to re-implement stuff in my own native node addon module, maintain a fork and complete the work on this there, etc... which also includes having to port the grammers binding, and regenerate and rebuild any that I want to use... Or maybe using the slower wasm version... But I'm not sure if @maxbrunsfeld, or anyone else, is still actively maintaining this to get this merged and done all the way through... |
|
Oh great. superstring even fails to build in newer macOS: |
|
superstring is unmaintained and should be removed. |
mastersince creation of thenapibranch to use NapiNapi::ReferenceNotes:
TSLanguage*from the module, which is to export a_languageproperty which is aNapi::External. This allows loading both new and old, but I have not added (except in a manual test) the code to generate such a property in new auto generated bindings, and this new contract warrants some discussion