[Suggestion] Add `try` to keywords, and remove `try`, `try?` and `try!` from the operators.

7ombie commented 9 months ago

Xcode highlights try as a keyword, and highlights ? and ! as operators. I think that works better, but it's kinda subjective.

alex-pinkus commented 9 months ago

This seems reasonable in the abstract, although it’s hard to say for sure without screenshots. I can probably find some time to experiment with this the weekend after next; if you have time for a PR earlier than that then I’ll happily review.

7ombie commented 8 months ago

Thanks for considering it. No rush, either way. I'll try and make a PR, or share some screenshots or something, soon. Thanks again.

7ombie commented 8 months ago

Hey, @alex-pinkus. I had another look at this, and found a handful of issues. If I understand correctly, most (all but one) are upstream of this project, but I'll reiterate here, so you're aware at least (and I may be wrong):

any is missing from the keywords list (this is the one I think you can fix)
defer should be a keyword
inout is highlighted, but borrowing and consuming are not
Self and self should be highlighted the same way (Self is also a builtin variable)
the new if- and switch-expressions are not parsed correctly (highlighting fails when you use them as expressions)
in Xcode, try is highlighted as a keyword, and ? and ! as suffix operators (when in suffix position), but try? and try! are lexed as single (operator) tokens

Adding "any" to Helix's list of keywords (below "some" in highlights.scm) fixed that issue. I could open a PR that adds "any" to your scm file, if that'd even be helpful?? The rest seem to be TreeSitter issues.

I wasn't able to experiment with highlighting try separately to the ? and ! operators, as try? and try! are lexed as single tokens, so I'll need to open an issue with TreeSitter (likewise for the other issues). You can preview that approach in Xcode though. I also tried highlighting the entire tokens as keywords, but it just looked wrong.

alex-pinkus commented 8 months ago

Thank you so much for this thorough analysis! At a quick glance I think all of these are on me — the base tree-sitter only gives us some infrastructure for parsers but all of this is work needed on the grammar.

I’ll try to find some time to add support for if- and switch-expressions — that one seems most likely to require meaningful grammar changes. Separating try from its punctuation shouldn’t be too hard; if I recall, I special case those as tokens and can just change to token + immediates. If you’re interested I could probably provide enough guidance to show you how to do that or others as a PR (if-expressions is the only one that I think is too tricky to take on as a first issue).

alex-pinkus commented 8 months ago

And if you’re updating highlights.scm in the helix version, I would definitely appreciate a PR to keep this in sync!

7ombie commented 8 months ago

No problem. I see. I don't understand how everything fits together. I'd be happy to dive into it more, but need to revisit the TreeSitter docs and better understand how everything works.

I'll make a PR for the any keyword, so you're in sync with Helix again, and I'll get back to you on the other stuff.

Note: I only added await and any to the Helix version of highlights.scm (so far), and you already have await. Once any is added to your highlights.scm, the rest of the fixes should flow downstream.

Thanks again, mate. Much appreciated.

7ombie commented 8 months ago

I was just looking through some of the other open issues. They're generally all issues for me/Helix too. Once I understand how TreeSitter works better, I'll try and help you close a few.

7ombie commented 8 months ago

I haven't forgotten. I just didn't realize I had to install Node, so I'm experimenting with connecting to GitHub Codespaces (from a local VSCode installation), and doing PRs that way.

7ombie commented 8 months ago

I gave this a go, and got a bit stuck with it. I thought I just needed to update highlights.scm and more or less copy the tests for some, replacing some with any. I'm now thinking I need to update the parser too, and am unsure how to.

I added any to highlights.scm, and grepped .../test/corpus/ for some. The tests use some* a lot already for meta variables (like someProperty, someType etc), which was a bit unfortunate, but adding a space (like some) worked fine, so no biggy.

The tree-sitter tests match a Swift string to its S-expression AST, and that's when I realized that the parser will need to know that any is a keyword. That should've been obvious, but I hadn't had to do anything like that when I added any to Helix, so never considered it. I don't know my way around the parser, so ended up a bit lost.

alex-pinkus commented 8 months ago

So you're on the right track, but you actually don't need to mess with test/corpus. That's where we unit test the grammar itself, such as the rule that defines the any Type pattern to begin with. Since that definition already exists, the parser knows that any is a keyword here, so your highlight query works as intended.

Since your goal is to test highlights.scm, what you want to do is add a new example to test/highlight that illustrates the cases you wish to highlight. The format of those files is pretty self-explanatory. All of the existing cases come from repositories that were at one point in Github's list of top starred Swift repositories (which I test my parser on on every change). When I remember to update the highlight tests, I copy a relevant file into test/highlight, then add extra comment lines like ^ keyword or ^ punctuation.bracket that point to the highlighted contents., just to demonstrate -- then if yo

I can create a PR real quick for your any change to demonstrate -- if you've got the idea from that then I'll let you do some of the other cases that you've mentioned above.

7ombie commented 8 months ago

I see. I think I get it now. I was looking through the stuff you mentioned.

I can create a PR real quick for your any change to demonstrate -- if you've got the idea from that then I'll let you do some of the other cases that you've mentioned above.

Cool. Yeah, If you don't mind. That'd be helpful. It'd also be nice to know it's done, and I can spend a bit of time looking through the code, and the other issues. Nice one, buddy. Thanks again.

alex-pinkus commented 8 months ago

I had some free time to address all of the issues you listed above; many of them ended up being a bit more involved than just changing highlight queries so it was probably for the best that I did it. Feel free to submit a PR with any additional enhancements you come up with.

7ombie commented 8 months ago

Nice work, Alex. Well done, mate! You're an awesome maintainer.

I'm still keen to improve Swift support in Helix, and looking forward to contributing to your project along the way.

alex-pinkus / tree-sitter-swift

[Suggestion] Add `try` to keywords, and remove `try`, `try?` and `try!` from the operators. #351