Skip to content

Add neocaml support for odoc mld files.#46

Open
tmcgilchrist wants to merge 11 commits into
bbatsov:mainfrom
tmcgilchrist:odoc
Open

Add neocaml support for odoc mld files.#46
tmcgilchrist wants to merge 11 commits into
bbatsov:mainfrom
tmcgilchrist:odoc

Conversation

@tmcgilchrist

Copy link
Copy Markdown
Contributor

Tree-sitter based major mode for editing odoc documentation (.mld) files, provides full syntax highlighting for OCaml code blocks.

I'm working on syntax highlighting for common code block languages like dune, opam and sh/bash. That covers the common cases I use and can be extended for other languages.

This is what it looks like with an OCaml code block.
Screenshot 2026-04-06 at 9 11 59 pm

Needs some testing and updating the copy-n-paste comments from other neocaml-*.el files. :-)

@tmcgilchrist

Copy link
Copy Markdown
Contributor Author

Examples of dune and opam highlighting. The bash/sh highlighting is a mess and quite difficult to extract into a useful function.
Screenshot 2026-04-06 at 9 24 49 pm

Comment thread neocaml-odoc.el Outdated

@bbatsov bbatsov left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the contribution! The code looks good at a glance — nice injection architecture and solid coverage of the odoc markup features. I left a few inline remarks below.

Before wrapping up, it'd be nice to add some unit tests (font-lock, imenu, auto-mode) similar to those for the other modes — check test/neocaml-dune-test.el or test/neocaml-cram-test.el for examples. A sample .mld fixture in test/resources/ would also be helpful.

Comment thread neocaml-odoc.el Outdated
Comment thread neocaml-odoc.el Outdated
(neocaml-mode--font-lock-settings 'ocaml))
('dune
(require 'neocaml-dune)
(symbol-value 'neocaml-dune--font-lock-settings))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

A couple of things here:

  • (or (neocaml-odoc--font-lock-for-grammar grammar) nil) — the (or x nil) is a no-op, just x is fine.
  • delete-dups mutates its argument destructively. Safe here since mapcar returns a fresh list, but seq-uniq communicates intent better.
  • The cl-loop + delete-dups + mapcar combo could be simplified to avoid the cl-lib dependency:
(mapcan (lambda (grammar)
          (copy-sequence
           (neocaml-odoc--font-lock-for-grammar grammar)))
        (seq-uniq (mapcar #'cdr injections)))

Comment thread neocaml-odoc.el Outdated
Comment thread neocaml-odoc.el
;;; Indentation

(defvar neocaml-odoc--indent-rules
`((odoc

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The no-node catch-all indents every continuation line by neocaml-odoc-indent-offset. For plain paragraph text inside document, this means continuation lines of paragraphs get indented, which is probably wrong for a markup format. Consider restricting this or adding a (parent-is "document") rule with column-0 0 before the catch-all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've changed the code but the interaction in Emacs needs more testing. I'll use this for a few days in my Emacs session and report back.

Comment thread neocaml-odoc.el
(when (y-or-n-p "Odoc tree-sitter grammar is not installed. Install it now?")
(neocaml-odoc-install-grammar))
(unless (treesit-ready-p 'odoc)
(error "Cannot activate neocaml-odoc-mode without the odoc grammar")))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The feature list includes features not defined by any odoc font-lock rule (comment, definition, keyword, string, constant, variable, operator, delimiter, property, label, function). These only exist when injected grammars provide them, which varies at runtime. Consider trimming levels 1-2 to only features the odoc rules actually define, and keeping the injection-dependent features at higher levels.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand the suggestion here. Could you elaborate?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hmm, seems my comment move to the wrong line for some reason. I meant that the injection-dependant font-locking should be handled separately, e.g. like here:

(defun neocaml-menhir--font-lock-settings ()
  "Return font-lock settings for `neocaml-menhir-mode'.
When OCaml injection is available, includes font-lock rules for
embedded OCaml code inside action blocks."
  (append
   neocaml-menhir--font-lock-settings
   (when (neocaml-menhir--injection-available-p)
     (require 'neocaml)
     (neocaml-mode--font-lock-settings 'ocaml))))

For the odoc feature list the default should be just this IMO:

(setq-local treesit-font-lock-feature-list
              '((heading tag)
                (markup code math reference)
                (escape-sequence list)
                (bracket)))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, got it. I'll try this out.

Comment thread neocaml-odoc.el
Comment thread neocaml-odoc.el
@tmcgilchrist

Copy link
Copy Markdown
Contributor Author

@bbatsov I think I've covered most of the code review comments. I've left open the two comments that need more attention.

Comment thread neocaml-odoc.el Outdated
Comment thread neocaml-odoc.el Outdated
@dbuenzli

dbuenzli commented Apr 7, 2026

Copy link
Copy Markdown

Very nice. Have been waiting on that for years :–)

However could we have per feature font faces with resonable font-lock-*-face defaults? It makes it easier to remap colors if your theme ends up making wrong emphasis or fails to make some distinctions.

See for example how I do it in my alternative neocaml font lock mode.

@tmcgilchrist

Copy link
Copy Markdown
Contributor Author

Thanks @dbuenzli I'll take a look at your font lock changes.

@dbuenzli

dbuenzli commented Apr 7, 2026

Copy link
Copy Markdown

Note I didn't check your font-lock results. So maybe they are good.

But I'd like to possibly allow myself to make them match those I have in markdown-mode (using the default there) and possibly neocaml-mode, e.g. for identifier references. The idea of having one font-face per feature makes that much easier.

@tmcgilchrist

Copy link
Copy Markdown
Contributor Author

@dbuenzli See what you think of ff1ef39
Screenshot 2026-04-08 at 4 59 21 pm

@dbuenzli dbuenzli mentioned this pull request May 11, 2026
@bbatsov

bbatsov commented May 21, 2026

Copy link
Copy Markdown
Owner

So, what are we doing about this?

@tmcgilchrist

Copy link
Copy Markdown
Contributor Author

I'd like to tidy up an initial version of this support for *.mld files and get some code review on the current structure as is. We can revisit the feedback from @dbuenzli when he has time to try out the font-lock changes.

There is follow on work if someone want to do it. We could apply the grammar to code comments in source files and highlighting other places code comments appear like LSP responses.

@tmcgilchrist tmcgilchrist marked this pull request as ready for review May 21, 2026 21:54
Tree-sitter based major mode for editing odoc documentation (.mld) files,
provides full syntax highlighting for OCaml code blocks.
neocaml-odoc--injection-language-alist maps odoc language tags to tree-sitter
grammars: ocaml, dune, opam, sh/bash. Falls back to font-lock-string-face when
no grammar is available. Requires Emacs 30 for language injection.
Remove { bracket token in font-lock rules. Add :override t to param/raise name
tag rules so they display correctly inside parent tag nodes.
Also added odoc grammar installation step for CI.
Roughly follows markdown-mode with odoc specific changes: headings get bold
weight, code inherits fixed-pitch, references and links inherit link, structural
markup inherits shadow.
@dbuenzli

Copy link
Copy Markdown

We can revisit the feedback from @dbuenzli when he has time to try out the font-lock changes.

By all means don't wait on that. It seems you went with the face per feature approach I mentioned in my comment which makes it easy to tweak to wish (notably to perhaps unify certain aspects with my alternate neocaml mode).

Looking forward to have some color signposts in these long .mld documents!

@tmcgilchrist

Copy link
Copy Markdown
Contributor Author

@bbatsov Could you re-review this? I've fixed two bugs in the underlying tree-sitter-odoc around media links and stray \ breaking font-locking.

@bbatsov bbatsov left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the updates - the earlier review points are all addressed and the injection setup reads cleanly. I built it locally: byte-compiles clean with warnings-as-errors and the odoc tests pass.

A few things left, inline below. The main one to decide before merging is the sh/bash support, which is advertised in the commentary, the docstring and the sample but isn't actually wired up. Also missing a CHANGELOG entry and a README mention (the intro and feature list enumerate every other mode).

Comment thread neocaml-odoc.el
'(("ocaml" . ocaml)
("dune" . dune)
("opam" . opam)
("sh" . bash)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These sh/bash entries are still here, but neocaml-odoc--font-lock-for-grammar has no bash case, so they only spin up an unfontified parser. The commentary, the docstring, and the {@sh[...]} block in sample.mld all advertise sh/bash as well. Since there's no bash mode in the repo, I'd just drop the two entries (plus the commentary line and the sample block). With them gone, the seq-uniq and the maphash group-by-grammar logic also become unnecessary - each tag maps to a single grammar.

Comment thread neocaml-odoc.el
(let ((injections (neocaml-odoc--available-injections)))
(append
neocaml-odoc--font-lock-settings
(if injections

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This gates the plain-code fallback on zero injections being available. Once OCaml (or any one grammar) is installed, a code block in some other unsupported language gets no face at all. Better to append the code_block_content string rule unconditionally - injection overrides it for the languages that do inject.

Comment thread neocaml-odoc.el
:type 'natnum
:safe 'natnump
:group 'neocaml-odoc
:package-version '(neocaml . "0.8.0"))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This says 0.8.0 (already shipped, without this mode) while the faces below all say 0.9.0. It all lands at once, so use the same next-release version everywhere.

Comment thread neocaml-odoc.el

;;; Navigation

(defun neocaml-odoc--defun-name (node)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

treesit-node-text returns the whole node, so imenu/which-func show {0 My Library} instead of My Library, and multi-line tags come through with the @tag marker and embedded newlines. Worth pulling out the title/name child the way neocaml-menhir--defun-name does.

Comment thread neocaml-odoc.el
(setq-local treesit-simple-imenu-settings neocaml-odoc--imenu-settings)

;; Navigation
(setq-local treesit-defun-type-regexp

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This covers only heading|tag_param|tag_return|tag_raise, but the imenu Tag regexp on line 400 covers seven tag types - the two disagree. Treating tags as defuns for C-M-a/C-M-e is a bit odd anyway; I'd narrow this to just heading.

Comment thread neocaml-odoc.el
((parent-is "ordered_list") parent-bol neocaml-odoc-indent-offset)
((parent-is "light_list") parent-bol neocaml-odoc-indent-offset)
((parent-is "li_list_item") parent-bol neocaml-odoc-indent-offset)
((parent-is "dash_list_item") parent-bol neocaml-odoc-indent-offset)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Checked against the grammar: dash_list_item and table_row (line 386) aren't real node types - light lists parse as light_list_item and light tables as table_light_cell/table_separator - so these rules never fire. Harmless but dead; worth dropping or correcting.

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.

3 participants