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

Add tree-sitter enabled zig-ts-mode major mode #95

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nanzhong
Copy link

@nanzhong nanzhong commented Jan 4, 2024

Addresses #89.

This PR refactors the existing zig-mode to derive from a new generic major mode zig-base-mode that holds common zig major mode configuration and then introduces a new tree-sitter enabled zig-ts-mode major mode that makes use of the zig tree-sitter grammar from https://github.com/maxxnino/tree-sitter-zig.

At the moment zig-ts-mode is pretty usable and supports tree-sitter powered syntax highlighting and indentation. It also derives common functionality like reformatter support, electric indent config, etc. from zig-base-mode. Two notable missing features are navigation and imenu integration (I don't use these much day to day -- I'm also very new to zig and have only been playing around with the language on the side -- so I have not yet implemented them).

I wanted to create this draft PR to outline and share the approach that I've taken. If it's directionally sound, I'm happy to do the remaining work needed to get the PR across the finish line (e.g. readme updates, tests).

I've configured it for use as follows (using elpaca and use-package):

(use-package zig-mode
  :elpaca (zig-mode :host github :repo "nanzhong/zig-mode" :branch "tree-sitter")
  :demand t
  :mode (("\\.zig\\'" . zig-ts-mode)
         ("\\.zon\\'" . zig-ts-mode))
  :init
  (add-to-list 'treesit-language-source-alist
               '(zig "https://github.com/maxxnino/tree-sitter-zig"))
  :hook (zig-ts-mode . (lambda ()
                         (setq treesit-font-lock-level 4))))

This change sets up the foundation for introducing a tree-sitter
enabled zig-ts-mode that can also derive from zig-base-mode.
This initial version of zig-ts-mode only makes use of tree-sitter
queries for syntax highlighting.
@nanzhong nanzhong marked this pull request as ready for review January 4, 2024 03:34
@jcs090218
Copy link
Collaborator

I genuinely think this should be a separated package, similar to the approach taken by numerous other major modes such as clojure-ts-mode, php-ts-mode, and so on.

@purcell
Copy link
Contributor

purcell commented Mar 1, 2024

Would definitely need to be a separate package, otherwise it couldn't be byte-compiled on the older Emacsen that this package supports.

@Doomwhite
Copy link

I can't seem to be able to run eglot or lsp-mode with this major-mode...

@Dev380
Copy link

Dev380 commented Mar 20, 2024

Eglot works for me, do you have logs you're willing to share?

Edit: it started breaking for me recently too, I'm not sure why. Eglot will say it can't detect the language server, and once it's given zls, it'll succeed but say it's managing 0 buffers in the project.

@joachimschmidt557
Copy link
Member

I agree with @jcs090218 and @purcell. At least as long as we support older Emacs versions which do not include tree sitter support by default, it is best to separate this into a different package/repository.

There is https://github.com/ryleelyman/zig-ts-mode already, but it would be nice to have an official package under the ziglang organization. @andrewrk can we get a zig-ts-mode repository in the ziglang organization?

@Dev380
Copy link

Dev380 commented Apr 3, 2024

Would additional work be required to integrate the treesitter mode with this one? Or else commands that aren't specific to either the regular or tree sitter mode (such as the run command) would need to be duplicated across both. Kind of like what clojure is doing and IIRC Rustic mode used to have this too (where you can sub the treesitter mode in as a "backend" to the regular mode). Would a future official zig-ts-mode follow this architecture?

@purcell
Copy link
Contributor

purcell commented Apr 3, 2024

There are two general patterns you'll see.

Common base mode

In this scheme you'd have an artificial zig-base-mode in zig-mode.el, as a parent mode for zig-mode. Then a separate zig-ts-mode package could use zig-base-mode as the parent mode too. Configuration like LSP etc. would then get hooked to zig-base-mode, while zig-mode would provide the non-treesitter font-locking and indentation.

This pattern is probably the best overall, but you won't see it in some cases because it requires the "classic" major mode author to add that artificial base mode.

Examples: python-mode/python-ts-mode, and other Emacs built-ins.

Completely independent ts mode

Here you'd have a zig-ts-mode package with no package dependency on zig-mode, and basically nothing in common. This pattern occurs most often in cases where the "base" package is heavyweight or opinionated, and the -ts-mode author wants to make a clean break from it.

Examples: nix-ts-mode, clojure-ts-mode, ocaml-ts-mode,


If maintained in the same github org, or even if not, I'd suggest the first pattern is preferable.

@Ziqi-Yang
Copy link

Ziqi-Yang commented Sep 24, 2024

Since this thread doesn't seems to have much process by months, and github:robbielyman/zig-ts-mode is still an half-completed work (only font lock is based on tree sitter). I made my own zig-ts-mode, which is on https://codeberg.org/meow_king/zig-ts-mode

@purcell
Copy link
Contributor

purcell commented Sep 24, 2024

I made my own zig-ts-mode

Looks good! I'd be happy to add this to MELPA directly pretty much as-is, but I have a couple of requests first:

  • Don't autoload the auto-mode-alist entry: it should be possible to install both this and zig-mode on machines without treesitter, without breakage. Treesitter and individual grammars don't show up as package dependencies, which makes things messy: every *-ts-mode package uses a different method for all this, and they all have disadvantages. The best compromise is to just tell the user to modify auto-mode-alist, or major-mode-remap-alist.
  • The variables like zig-ts-mode-font-lock-rules-definition are described as customisation, but I don't think those are useful extension points: if people improve these rules, they should send the improvements to you, not apply them locally. I'd just hard-code the zig-ts-mode-font-lock-rules without those vars.

@purcell
Copy link
Contributor

purcell commented Sep 24, 2024

(Also the indent rules look surprisingly minimal, and at least one comment mentions rust-mode, probably copy-and-pasted)

@Ziqi-Yang
Copy link

Ziqi-Yang commented Sep 24, 2024

Thank you for ur advice. They all make sense :). I've correct it according to ur advice.

Also the indent rules look surprisingly minimal,

Copied from the upstream zig tree sitter grammar query file for indentation. They should work for most cases


Edit:
Found some indentation issues. Working on it.

@purcell
Copy link
Contributor

purcell commented Sep 24, 2024

Cool, added to MELPA — it should start building within a few hours. If you add a version tag to the repo, it'll show up in MELPA Stable too.

@purcell
Copy link
Contributor

purcell commented Sep 24, 2024

If in future everyone thinks it best, your repo could get moved to this GitHub org: we'd then need to adjust the MELPA recipe. Thanks for sharing your work!

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.

7 participants