Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update tree-sitter to 0.21.1 with napi bindings #404

Merged
merged 9 commits into from
Apr 22, 2024

Conversation

trevor-e
Copy link
Contributor

This addresses the issue I opened here: #400

I updated the tree-sitter and tree-sitter-cli dependencies to their latest versions. There are a few major features I wanted to take advantage of in this: 1) uses the napi interface for native code, and 2) has actual arm64 support.

After updating the dependencies I ran tree-sitter generate which produced the large majority of this diff. There was one issue I ran into after running this which was:

Error: dlopen(/Users/telkins/dev/tree-sitter-swift/build/Release/tree_sitter_swift_binding.node, 0x0001): symbol not found in flat namespace '_tree_sitter_swift_external_scanner_create'

I fixed this by adding "src/scanner.c", to the binding.gyp file where it mentions including custom scanners. After that, all of the tests seem to pass in my local repo that I'm using this library with! 🎉

"author": "Alex Pinkus <[email protected]>",
"license": "MIT",
"bugs": {
"url": "https://github.com/alex-pinkus/tree-sitter-swift/issues"
},
"homepage": "https://github.com/alex-pinkus/tree-sitter-swift#readme",
"dependencies": {
"nan": "^2.18.0",
"tree-sitter-cli": "=0.20.6",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously pinned to 0.20.6 which is why the latest arm64 updates weren't working. I took a look at the commit for why this was pinned in the first place but wasn't completely sure what it was referencing.

],
"sources": [
"bindings/node/binding.cc",
"src/parser.c",
"src/scanner.c"
# NOTE: if your language has an external scanner, add it here.
"src/scanner.c",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue I'm running into is that re-running tree-sitter generate removes this line, didn't realize this is part of the autogen. I haven't been able to figure out how to get the generate command to include the external scanner.

@@ -1,4 +1,4 @@
#include <tree_sitter/parser.h>
#include "tree_sitter/parser.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was causing the build to fail.

@trevor-e
Copy link
Contributor Author

@alex-pinkus bit stumped on these build failures if you have any ideas. It looks like the header files aren't being picked up properly in the Makefile. I tried copying the Makefile that both the javascript/python client libraries were using, with no luck.

@clason
Copy link

clason commented Apr 18, 2024

Note that the latest tree-sitter version is actually 0.22.5. Since there's a number of important changes in 0.22+ (in particular related to bindings), you absolutely should use that.

@clason
Copy link

clason commented Apr 18, 2024

tree-sitter generate also warns of a number of unused conflicts:

Warning: unnecessary conflicts
  `_expression`, `switch_statement`
  `simple_identifier`, `_modifierless_class_declaration`
  `willset_didset_block`
  `_no_expr_pattern_already_bound`, `_binding_pattern_with_expr`
  `_expression_with_willset_didset`, `_expression_without_willset_didset`
  `capture_list`, `_local_property_declaration`, `_local_typealias_declaration`, `_local_function_declaration`, `_local_class_declaration`
  `capture_list_item`, `self_expression`
  `simple_identifier`, `parameter_modifier`
  `simple_identifier`, `property_behavior_modifier`
  `_expression`, `_no_expr_pattern_already_bound`
  `_expression`, `if_statement`
  `_local_class_declaration`, `modifiers`

Since tree-sitter-swift is one of the slowest parsers in the world (right after nim), that might be worth looking into.

@alex-pinkus
Copy link
Owner

@clason Between “one of the slowest parsers in the world” and your comments on #398, I’m starting to feel somewhat like you have some problem with either me or this project. Remember that open-source work is often volunteer work, and relies on maintainers feeling independently motivated to contribute. If you do not intend to disparage my work, please be a little more careful with your tone. Your comments were probably intended with no ill will, but they certainly demotivate me.

As I’ve mentioned before, I have done extensive profiling and optimization to get this parser to where it is. If you find a silver bullet you are welcome to open a pull request.

@alex-pinkus
Copy link
Owner

@alex-pinkus bit stumped on these build failures if you have any ideas.

I will find some time this weekend to take a look.

@clason
Copy link

clason commented Apr 18, 2024

I apologize for my tone, then. I, too, am maintaining (multiple) projects in my spare time, which I only have in small chunks, and I often fail to put it in the required time to formulate my comments appropriately. This was merely meant as a hint for possible performance gains lying on the table. (It is a fact, however, that Swift -- after Nim -- leads the nvim-treesitter leaderboard by some margin. I consider this useful information; you may not care about it -- that's fine, there are other valid project goals -- but it certainly affects me through our CI times. In case it wasn't clear, I do appreciate the work you're putting in here and in nvim-treesitter.)

@trevor-e
Copy link
Contributor Author

@alex-pinkus bit stumped on these build failures if you have any ideas.

I will find some time this weekend to take a look.

Thanks, happy to help out getting this over the finish line if you find anything you want to delegate, just ping me. I'll continue trying a few ideas after some more reading and inspecting other projects.

The Makefile changes were just me trying out what some other languages were using, might want to revert that for now.

@alex-pinkus
Copy link
Owner

OK, so the problem is that at the time that node-gyp runs and examines the header files, tree-sitter generate has not yet run, so the header it's complaining about indeed does not exist. Once you generate the files once, everything works properly.

Let me dust off the cobwebs on this build logic... maybe there's an easy way to order it properly.

@alex-pinkus alex-pinkus mentioned this pull request Apr 22, 2024
@alex-pinkus
Copy link
Owner

I pushed some changes to #406 to address the remaining build failures, and now it seems to work properly. I'll leave that PR open for a week or so to give you the chance to chime in on anything that looks out of place, or, if you take a look sooner, I'll go ahead and merge.

@alex-pinkus alex-pinkus merged commit 316f2ce into alex-pinkus:main Apr 22, 2024
2 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants