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

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

Closed
7ombie opened this issue Feb 8, 2024 · 13 comments

Comments

@7ombie
Copy link

7ombie commented Feb 8, 2024

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

@alex-pinkus
Copy link
Owner

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

7ombie commented Feb 9, 2024

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

7ombie commented Feb 11, 2024

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

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

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

@7ombie
Copy link
Author

7ombie commented Feb 12, 2024

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

7ombie commented Feb 12, 2024

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.

alex-pinkus added a commit that referenced this issue Feb 13, 2024
As pointed out in #351.

[SE-0380](https://github.com/apple/swift-evolution/blob/main/proposals/0380-if-switch-expressions.md)
made it possible to do this, but support was lacking in this repository.
The fix turned out to be easier than anticipated, simply requiring the
statement to be added in some new places. Theoretically, it should also
be renamed, but that would make this a breaking change.

The proposal does not make clear whether labels are permitted on
expression-position `if` and `switch` statements. This assumes they are
not, for simplicity.
alex-pinkus added a commit that referenced this issue Feb 13, 2024
As pointed out in #351.

[SE-0380](https://github.com/apple/swift-evolution/blob/main/proposals/0380-if-switch-expressions.md)
made it possible to do this, but support was lacking in this repository.
The fix turned out to be easier than anticipated, simply requiring the
statement to be added in some new places. Theoretically, it should also
be renamed, but that would make this a breaking change.

The proposal does not make clear whether labels are permitted on
expression-position `if` and `switch` statements. This assumes they are
not, for simplicity.
@7ombie
Copy link
Author

7ombie commented Feb 13, 2024

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

7ombie commented Feb 18, 2024

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

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.

alex-pinkus added a commit that referenced this issue Feb 18, 2024
In helix-editor/helix#9586, the Helix editor added highlighting for the
`any` keyword.

However, while reviewing that, I noticed that both `some` and `any`
should be more specific: these keywords are legal as identifiers, but
should not be highlighted as identifiers when they are keywords.

For instance, I can write:
```
let any: any Protocol = AnyImplementation()
```

In this example, only the second `any` should be highlighted.

See discussion on #351
alex-pinkus added a commit that referenced this issue Feb 18, 2024
WIP. It should be possible to have `token.immediate` behavior for this;
currently an expression where `try` is lifted to the top level and
followed by a negation will mis-parse.

See #351
@7ombie
Copy link
Author

7ombie commented Feb 18, 2024

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 added a commit that referenced this issue Feb 18, 2024
In helix-editor/helix#9586, the Helix editor added highlighting for the
`any` keyword.

However, while reviewing that, I noticed that both `some` and `any`
should be more specific: these keywords are legal as identifiers, but
should not be highlighted as identifiers when they are keywords.

For instance, I can write:
```
let any: any Protocol = AnyImplementation()
```

In this example, only the second `any` should be highlighted.

See discussion on #351
@alex-pinkus
Copy link
Owner

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

7ombie commented Mar 4, 2024

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.

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

No branches or pull requests

2 participants