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

feat(languages): Lean experimental tree-sitter-lean #1422

Merged
merged 9 commits into from
Jan 17, 2022

Conversation

Anderssorby
Copy link
Contributor

Trying out the experimental tree-sitter-lean from @Julian

@Julian
Copy link

Julian commented Jan 2, 2022

Cool :) Let me know how it goes. There are lots of issues (and in neovim it's super painful perf wise at the minute because the queries are large), but it does work.

@Anderssorby
Copy link
Contributor Author

BTW is the parser the C output of the compiled Lean parser? And would it make sense to rather use the Lean source for compiling this? I've been making several Lean <-> C <-> Rust bindings.

@Julian
Copy link

Julian commented Jan 3, 2022

BTW is the parser the C output of the compiled Lean parser?

It's what tree-sitter emits from the compiled typescript Lean parser if that's what you mean.

And would it make sense to rather use the Lean source for compiling this?

The grammar was mostly hand-ported (first not-very-directly, now somewhat directly) from the Lean source. It did (does) occur to me that it may be possible to transpile it but I haven't experimented with doing so.

@Anderssorby
Copy link
Contributor Author

Currently the syntax highlight only works when there is an error. The LSP works fine, but it needs to have some display of the '$/lean/fileProgress' notification (which I'm guessing is the goal view).

@Julian
Copy link

Julian commented Jan 4, 2022

fileProgress is Lean (server) saying what parts of the file it's processing. The infoview stuff is pulled by the client. In neovim this lives here: https://github.com/Julian/lean.nvim/blob/main/lua/lean/rpc.lua#L238-L281 and is done via the RPC protocol. (Neither of those should have much to do with syntax highlighting I think? But I don't know anything about Helix :) maybe I should try it.)

@the-mikedavis
Copy link
Member

in neovim it's super painful perf wise at the minute because the queries are large

I think this should be improved with tree-sitter v0.20.2 (see tree-sitter/tree-sitter#1510), but I'm not sure if dependabot has ugraded that dependency yet in helix

@Julian
Copy link

Julian commented Jan 4, 2022

Yes! Indeed that change fixed the perf issue on the neovim side.

.gitmodules Outdated Show resolved Hide resolved
Anderssorby and others added 2 commits January 4, 2022 15:54
Co-authored-by: Ivan Tham <pickfire@riseup.net>
@Anderssorby Anderssorby marked this pull request as ready for review January 4, 2022 15:29
@archseer
Copy link
Member

archseer commented Jan 5, 2022

0.20.2 is in master, but I've accidentally excluded it from the 0.6 release. I'll push a patch release later this week to include some bug fixes so it'll be included in a stable release then as well.

@Anderssorby
Copy link
Contributor Author

So do you want to merge this? It works for me although more features could be added like a lean info view.

@archseer
Copy link
Member

archseer commented Jan 5, 2022

I'll do a thorough review later today, but as mentioned on Matrix, the highlights need to be modified to match https://docs.helix-editor.com/themes.html#syntax-highlighting

There's no @number for example, we use @constant.numeric.integer

Similarly the injections.scm definitions are different

@Anderssorby
Copy link
Contributor Author

@archseer Are we okay for merging? I'll probably add an Lean info view in a later PR.

Comment on lines 8 to 9
; Variables
(identifier) @variable
Copy link
Member

@the-mikedavis the-mikedavis Jan 13, 2022

Choose a reason for hiding this comment

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

I think you'll want to move this stanza down to the bottom of the file. The higher in the file, the higher the precedence with helix (it's reversed in neovim), so this stanza will match all identifiers and so the rules on L38-L67 will never match.

Anderssorby and others added 2 commits January 13, 2022 21:09
Co-authored-by: Michael Davis <michael.davis@nfiindustries.com>
@Anderssorby
Copy link
Contributor Author

I notice that there are still several bugs in the highlightning, but this is probably an error in the lean-tree-sitter parser and not here.
Screenshot from 2022-01-13 21-22-12
Seems like def, for, partial, open, end and more fails to parse correctly sometimes. Lean4 is still experimental and the syntax keeps getting updated so I think it should be okay to merge before things are perfect.
@Julian

@Julian
Copy link

Julian commented Jan 13, 2022

Yes, there are still plenty of parsing bugs. Issues or patches are of course welcome.

Co-authored-by: Michael Davis <michael.davis@nfiindustries.com>
@Anderssorby Anderssorby changed the title Add experimental tree-sitter-lean feat(languages): Lean experimental tree-sitter-lean Jan 17, 2022
@Anderssorby
Copy link
Contributor Author

@the-mikedavis So let's merge?

@archseer
Copy link
Member

Sorry for the delay, I'll merge after the CI run 👍🏻

@archseer archseer merged commit 8ea5742 into helix-editor:master Jan 17, 2022
@Anderssorby Anderssorby deleted the acs/add-lean branch January 17, 2022 22:32
@archseer
Copy link
Member

Probably not the best place to raise this, but @Julian any idea why the compiled Lean grammar is so big? It's 16MB, compared to haskell's 3.2MB or rust's 1.1MB.

@Julian
Copy link

Julian commented Mar 11, 2022

It's a big/complicated grammar -- IIRC Agda's is similarly large and ran into some of the same tree-sitter issues I ran into. There's some improvements possible but right now improving it has been somewhat low priority for me in favor of other Lean editor things (and other obligations entirely). PRs certainly welcome.

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 this pull request may close these issues.

5 participants