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

languages: add koka #8727

Merged
merged 2 commits into from Mar 18, 2024
Merged

languages: add koka #8727

merged 2 commits into from Mar 18, 2024

Conversation

mtoohey31
Copy link
Contributor

@mtoohey31 mtoohey31 commented Nov 5, 2023

This PR adds support for Koka. It includes a grammar, highlights, and indents queries.

There isn't currently a complete language server, so I haven't included one (though there is an unmerged PR which implements one, so if that gets merged I'll create a follow-up PR).

The grammar isn't completely up-to-date with the latest version of the language: it doesn't include the new fip/fbip keywords added in 2.4.2 because the official grammar specification hasn't been updated to include them yet either. I intend to ask about this upstream; depending on when the official grammar spec is updated I may update the tree-sitter grammar's version in this PR before it's merged, or I might do that in a follow-up PR, if that's ok.

Edit: nevermind, I found more details on another upstream branch, so this should be fixed now.

Here's a screenshot with the gruvbox theme:

koka

@mtoohey31
Copy link
Contributor Author

Oh, one other thing I should probably explain: the tab-width is 8 but the unit is " " because whitespace is significant and the lexer treats tabs as having a width of 8, but the convention is to use two spaces per indent-level.

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-language-support Area: Support for programming/text languages labels Nov 6, 2023
the-mikedavis
the-mikedavis previously approved these changes Nov 8, 2023
@chtenb
Copy link
Contributor

chtenb commented Nov 13, 2023

This looks great! I've looked through some example koka code, and noticed the following things, probably something for future improvements.

Is the "scoped" keyword missing?
image

No "local variable" support yet (https://tree-sitter.github.io/tree-sitter/syntax-highlighting#local-variables)
image

@mtoohey31
Copy link
Contributor Author

Is the "scoped" keyword missing?

Yes, thanks for catching that @chtenb! I have a fix on this branch here: https://github.com/mtoohey31/tree-sitter-koka/tree/fix/scoped, would you mind taking a look? The places where scoped can be used aren't described in the grammar spec, but it's clearly supported by the compiler. I wasn't quite sure about which of the three effect declaration cases to allow it in. The spec line linked in my commit message on that branch seems to indicate it should be allowed for the first two but not for the third, but it's not very clear.

No "local variable" support yet (https://tree-sitter.github.io/tree-sitter/syntax-highlighting#local-variables)

I've updated the locals queries to handle the example above in d17962b. Let me know if you spot anything else that's being missed.

Copy link
Contributor

@chtenb chtenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The places where scoped can be used aren't described in the grammar spec, but it's clearly supported by the compiler.

Yeah, I see. It looks like the language is in flux and the spec isn't fully keeping up.

Are linear effects already in the language? If so, that keyword can also be added https://koka-lang.github.io/koka/doc/book.html#sec-linear

I think I looked through all language samples, and there was nothing else that stood out to me.

EDIT:
It would be nice to have var declarations stand out from other (immutable) identifers such as vals and function parameters. Perhaps we can find a suitable scope for these? The list of scopes at our disposal is not particularly accomodating unfortunately: https://docs.helix-editor.com/themes.html. What about variable.other? Or special?

@mtoohey31
Copy link
Contributor Author

Are linear effects already in the language? If so, that keyword can also be added https://koka-lang.github.io/koka/doc/book.html#sec-linear

I do have linear in the grammar and highlights, but I see what you mean in that some of the examples in test/algeff put it after the effect keyword for some reason? The grammar spec doesn't appear to allow that: https://github.com/koka-lang/koka/blob/875c0cee6ba42e296dfd4f612cb17731c53e236d/doc/spec/grammar/parser.y#L256-L281 though, so I'm not sure which to follow...

It would be nice to have var declarations stand out from other (immutable) identifers such as vals and function parameters. Perhaps we can find a suitable scope for these? The list of scopes at our disposal is not particularly accomodating unfortunately: https://docs.helix-editor.com/themes.html. What about variable.other? Or special?

Yeah it looks like we don't have much selection... variable.other doesn't appear to be highlighted in its own right (just variable.other.member), and it looks like special is the same colour as keywords or functions in a handful of themes. Maybe we should hold off on this for now until there's better options? Kotlin's val/var, which I believe are similar, don't appear to be highlighted differently.

@chtenb
Copy link
Contributor

chtenb commented Nov 16, 2023

so I'm not sure which to follow...

I guess we should leave it for now then

Yeah it looks like we don't have much selection... variable.other doesn't appear to be highlighted in its own right (just variable.other.member), and it looks like special is the same colour as keywords or functions in a handful of themes. Maybe we should hold off on this for now until there's better options? Kotlin's val/var, which I believe are similar, don't appear to be highlighted differently.

The reason I thought of this is because this is how F# highlighting works in Visual Studio. Similar to Koka and many functional languages, immutability is the default. Mutable variables are highlighted by a distinctive color, which makes it easier to grasp the code flow when reading source code.

I will create a separate issue for this.

@chtenb
Copy link
Contributor

chtenb commented Nov 16, 2023

Turns out there is an open issue already #6659

Moreover, the rust highlights have an open TODO for this feature:

; TODO: variable.mut to highlight mutable identifiers via locals.scm

The suggested name is variable.mut, but variable.mutable, variable.other.mutable seem also logical to me. @archseer Can we just add this feature to koka language support, and if so, do you have a preference for the naming of this scope?

@mtoohey31 mtoohey31 force-pushed the feat/koka branch 2 times, most recently from 887df4a to 31c638d Compare November 17, 2023 22:23
@mtoohey31
Copy link
Contributor Author

Alright, I've pushed a small fix for ntlappexpr highlighting, and I've added a TODO comment for adding the mutability highlight.

I agree with your suggested solution @chtenb, but I think it should probably be a separate PR cause it'll involve a lot of theme updates, as well as updates to the Rust and Kotlin highlight queries at the very least. Does that sound fair?

If so, I think this PR is ready.

Copy link
Contributor

@chtenb chtenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. LGTM

@chtenb
Copy link
Contributor

chtenb commented Nov 19, 2023

I've noticed a minor issue in the indent queries. When entering a new line after something like below, the cursor starts indented.

fun parse-input() { many1(parse-line) }
  [] <- cursor starts here

@chtenb
Copy link
Contributor

chtenb commented Nov 21, 2023

A small highlighting discrepancy I hadn't noticed before:
image
The dot-notation is a regular function call, so I'd expect the same color.

@mtoohey31 mtoohey31 force-pushed the feat/koka branch 2 times, most recently from 674c252 to a7d6d8e Compare November 23, 2023 05:21
@mtoohey31
Copy link
Contributor Author

Thanks, I've fixed the function highlighting and resolved the merge conflict. I'm working on the indent query. I can reproduce it but haven't figured out how to fix it yet.

@chtenb
Copy link
Contributor

chtenb commented Nov 27, 2023

I noticed another undocumented keyword missing: ref. It's an alias for reference it seems. I noticed this because compiler warnings mention this keyword. It looks like Koka has a trend of introducing abbreviations for longer keywords. And the documented grammar is not kept up-to-date with every minor change.

warning (line 1, column 11): keyword "reference" is deprecated. Consider using "ref" instead

@chtenb
Copy link
Contributor

chtenb commented Nov 27, 2023

I've noticed a minor issue in the indent queries. When entering a new line after something like below, the cursor starts indented.

fun parse-input() { many1(parse-line) }
  [] <- cursor starts here

Here is a somewhat similar situation

fun handle-graph-as-igraph(g : graph, f : () -> <igraph,exn|e> a) : <exn|e> a
  with handler
[]     <---- cursor starts here

I think it would make sense to have two or at least one level of indentation here.

@mtoohey31
Copy link
Contributor Author

I've added a couple missing keywords in 9c2af05. You may want to take a look at mtoohey31/tree-sitter-koka@4eef46e to see if that's all covered by koka-lang/koka#364.

I'll fix the merge conflicts again in a minute.

Still working on the indentation.

@chtenb
Copy link
Contributor

chtenb commented Dec 21, 2023

Found a case where keywords are not correctly highlighted, namely in the shorthand effect declaration:

image

@chtenb
Copy link
Contributor

chtenb commented Dec 26, 2023

One liners with a return type signature:
image
Arguably bad practice, but I use it for compact snippets to share on e.g. github..

@mtoohey31 mtoohey31 force-pushed the feat/koka branch 3 times, most recently from bd6e42e to 88b15d3 Compare December 27, 2023 17:57
@mtoohey31
Copy link
Contributor Author

One liners with a return type signature:
image
Arguably bad practice, but I use it for compact snippets to share on e.g. github..

This is invalid according to the grammar spec. Is this used anywhere in the upstream repo? According to the grammar, the only valid ways to do oneliners are by omitting the result type declaration:

pub fun take-exn(i : () -> exn int) i()
pub fun get() 42

...or by explicitly including braces:

pub fun take-exn(i : () -> exn int) : exn int { i() }
pub fun get() : e int { 42 }

@mtoohey31
Copy link
Contributor Author

image

This is an interesting one. Should arguments that are functions also be colored as functions? Notice how ordering is colored as a parameter in the parameter list, but as a function in the body when it's called, and as a variable again when it's passed in.

Based on how this is handled in other languages, I've chosen to fix this by highlighting it as a variable.parameter at call sites too.

@chtenb
Copy link
Contributor

chtenb commented Dec 27, 2023

This is invalid according to the grammar spec. Is this used anywhere in the upstream repo?

I haven't seen this used, no. I just noticed that the compiler accepts it, but adding the braces there makes sense for readability purposes. If the grammar spec doesn't allow it either, putting this in the "won't fix" bin is fine I think.

@mtoohey31
Copy link
Contributor Author

If the grammar spec doesn't allow it either, putting this in the "won't fix" bin is fine I think.

Sounds good!

Sorry about all the 👍 reactions lol, I've been using them to mark off what I've addressed. I believe I've fixed everything as of a26cb86.

@chtenb
Copy link
Contributor

chtenb commented Dec 27, 2023

Sorry about all the 👍 reactions lol, I've been using them to mark off what I've addressed. I believe I've fixed everything as of a26cb86.

Cool! No worries, I don't get notifications about reactions or anything

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment about a field that can be removed, otherwise this looks good to me

It could use a master-merge or rebase though to resolve the conflicts

languages.toml Outdated
scope = "source.koka"
injection-regex = "koka"
file-types = ["kk"]
roots = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This key can be dropped since this is the default value now (#8803)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, should be fixed as of 62bda67.

the-mikedavis
the-mikedavis previously approved these changes Jan 9, 2024
@pascalkuthe pascalkuthe merged commit 2e4653e into helix-editor:master Mar 18, 2024
6 checks passed
jpaju pushed a commit to jpaju/helix that referenced this pull request Mar 18, 2024
Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
Desdaemon pushed a commit to Desdaemon/helix that referenced this pull request Mar 26, 2024
Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
shortc pushed a commit to shortc/helix that referenced this pull request Mar 31, 2024
Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 20, 2024
Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants