alex-pinkus / tree-sitter-swift

A tree-sitter grammar for the Swift programming language.
MIT License
137 stars 35 forks source link

Update tree-sitter to 0.21.1 with `napi` bindings #404

Closed trevor-e closed 4 months ago

trevor-e commented 4 months ago

This addresses the issue I opened here: https://github.com/alex-pinkus/tree-sitter-swift/issues/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! 🎉

trevor-e commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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.