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

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

Closed
konrad1977 opened this issue Sep 9, 2022 · 12 comments · Fixed by #231
Closed

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

konrad1977 opened this issue Sep 9, 2022 · 12 comments · Fixed by #231

Comments

@konrad1977
Copy link

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
Copy link
Owner

alex-pinkus commented Sep 9, 2022

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
Copy link
Author

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

@alex-pinkus
Copy link
Owner

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
Copy link
Author

konrad1977 commented Sep 10, 2022

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
Copy link
Owner

alex-pinkus commented Sep 11, 2022

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
Copy link
Author

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. 🎉 emacs-tree-sitter/tree-sitter-langs#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
Copy link
Owner

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
Copy link
Author

konrad1977 commented Sep 11, 2022

Yes, now it works again after I did a make 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
Copy link
Owner

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

  • parameter (probably should be the same as variable.parameter)
  • include (used for import statements, not sure how you would want to highlight it)
  • repeat (for keywords like for and in, and while)
  • conditional (keywords like guard and if)
  • float and boolean (for literals; perhaps they should be highlighted like number?)

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.

alex-pinkus added a commit that referenced this issue Sep 11, 2022
Currently, a non-clean build after updating `grammar.js` will not pick
up any new changes. This happens because as far as the makefile is
concerned, `parser.c` is a source file, and it has not been updated.

To fix this, we simply add a rule that can generate `parser.c`. If
someone runs `make install` after `npm install`, this rule will do
nothing, because the output file will already be newer. However, if
someone updates the repo, this will ensure all artifacts get updated.

Fixes #229
alex-pinkus added a commit that referenced this issue Sep 11, 2022
Currently, a non-clean build after updating `grammar.js` will not pick
up any new changes. This happens because as far as the makefile is
concerned, `parser.c` is a source file, and it has not been updated.

To fix this, we simply add a rule that can generate `parser.c`. If
someone runs `make install` after `npm install`, this rule will do
nothing, because the output file will already be newer. However, if
someone updates the repo, this will ensure all artifacts get updated.

Fixes #229
@alex-pinkus alex-pinkus changed the title Errors occurs and syntax highlighting is lost or bleeding. make install builds an inconsistent dylib when grammar.js changes Sep 11, 2022
@konrad1977
Copy link
Author

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

@alex-pinkus
Copy link
Owner

I moved this conversation about highlight queries to a discussion so it doesn't get lost: #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
Copy link

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)

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 a pull request may close this issue.

3 participants