alex-pinkus / tree-sitter-swift

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

`make install` builds an inconsistent `dylib` when `grammar.js` changes #229

Closed konrad1977 closed 2 years ago

konrad1977 commented 2 years ago

I'm on the current main branch 58deb71df91bcee6d650774dbd136a7493ca20f

Here is the output with debug. Hope it helps

Skärmavbild 2022-09-09 kl  11 10 39

I didn't have them yesterday.

alex-pinkus commented 2 years ago

This is happening because the compiled version of your external scanner is out of date. It must not have gotten updated when you switched to the latest. The (generated) parser.c and (manually written) scanner.c files refer to the same set of constants that must remain in sync, so updating one requires you to update to the other.

The error will go away if you uninstall the grammar and then reinstall it. You may want to dig into the upgrade logic from the emacs tree sitter plugin to see how you got into this state.

(How do I know this is the issue? The source code in the provided screenshot parses just fine for me using the latest grammar. However, I see the exact same parse errors you report if I manually revert the most recent changes to scanner.c, which simulates what would happen if you updated just the parser file and not the scanner.)

[Edited to remove some content about tree-sitter version that may mislead others into thinking that was the issue. This issue is not caused by, or related to, the use of any one version of tree-sitter vs another]

konrad1977 commented 2 years ago

Ok thanks for the explanation. I think they are checking the possibilities in using a new compiler (20.x).

alex-pinkus commented 2 years ago

just to make sure we’re on the same page: the grammar still works with 0.19, you just need to fully uninstall and reinstall it to fix this issue. I don’t think that’s a versioning problem, it sounds like a bug in upgrade logic in general that will affect any grammar that has an external scanner.

can you try that out and let me know if it works? It may even be helpful to list out the steps you follow so that others who come across this later know what to do. If it works after doing that, please close the issue.

konrad1977 commented 2 years ago

In Emacs I just rename your dylib file to "swift.dylib" and place in a folder. Same thing with queries. So I dont really see how this would change anything. It's dynamically loaded by Emacs when needed.

alex-pinkus commented 2 years ago

Thank you for clarifying that! I'm not quite sure what "your dylib file" refers to -- I don't vend a dylib artifact. Are there steps you ran locally to create the dylib? I think that's where the bug lies -- that the dylib was created using old artifacts for the scanner (e.g. maybe it used a cached scanner.o) but a fresh parser file (created using a freshly-examined grammar.js). I am fairly certain that some issue like that would have caused these errors. Would you mind trying whatever steps again after cleaning any intermediate artifacts (e.g. by starting with a fresh git checkout)?

Since the latest grammar can parse the file you shared in a screenshot without any errors (with any version of tree-sitter you care to throw at it), I'm confident this isn't a widespread issue. If you're willing to try those steps, we can definitively confirm how future users can recover if they get into the same locally-inconsistent state. I totally understand if you'd rather not test it out, but unless you're up for confirming the theory, I don't really have further steps to help you.

Apart from testing that out, is there anything else we should do before closing this issue?

konrad1977 commented 2 years ago

Hi! Maybe I forgot to do make clean. But here are the steps I do for now. Btw, there is a pull request for integrating your repository into tree-sitter-langs for Emacs now. 🎉 https://github.com/emacs-tree-sitter/tree-sitter-langs/pull/118

  1. git pull
  2. make clean
  3. make install
  4. rename libtree-sitter-swift.0.0 -> swift.dylib (2.4 mb file)
  5. copy swift.dylib to .emacs.d/elpa/tree-sitter-langs/bin/
  6. copy the 3 queries .emacs.d/elpa/tree-sitter-langs/queries/swift
  7. restart Emacs.
  8. open a .swift file (make sure tree-sitter-hl-mode) is enabled
alex-pinkus commented 2 years ago

Ah, got it! Did a make clean fix the issue for you? If so, there’s probably something I need to fix in the makefile that’s preventing it from autodetecting that the scanner has changed, but that does narrow down the problem!

Great to see that PR, thanks for sharing it :)

konrad1977 commented 2 years ago

Yes, now it works again after I did amake clean first. Big thanks.

Do you have a list of which 'attributes' detected and supported in the tree-sitter-swift? I noticed that sometimes labels are shown.

Btw here are the faces that Emacs detects now. Do you have any custom detections that you dont see in that list? I know that the other tree-sitter had some custom that you needed to add yourself.

Skärmavbild 2022-09-11 kl  09 44 56
alex-pinkus commented 2 years ago

I see a few in highlights.scm that aren't in the screenshot above:

I don't know if the emacs detection is hierarchical (i.e. if it doesn't recognize foo.bar, it can still match that against foo). If it is not, there's also keyword.return and keyword.operator.

konrad1977 commented 2 years ago

Thanks. Ill probably need to add them manually some how.

alex-pinkus commented 2 years ago

I moved this conversation about highlight queries to a discussion so it doesn't get lost: https://github.com/alex-pinkus/tree-sitter-swift/discussions/233

Most of the highlight queries actually come from the neovim folks, so I wonder if they have any advice on how to maintain a maximally-applicable subset in the upstream repository (i.e. here) and make lives as easy as possible for anyone integrating at the editor level (i.e. you).

theHamsta commented 2 years ago

nvim-treesitter maintains its own version of highlights.scm. We don't use the upstream highlights.scm, so there is no need to keep the upstream highlights.scm compatible with nvim-treesitter. Atom never really standardized the usage of the captures. Maybe it would be worth to agree on a common subset (for helix, nvim-treeistter, Emacs). At the moment, helix and nvim-treesitter vendor their queries (helix queries initially forked from nvim-treesitter)