Syntax: Don't highlight the LHS of type decl as types

[Julow]

I would expect ocamlTypeConstr to apply only to type constructors within type expressions, not to the identifier after type in:

type foo = 'a bar list

foo is not highlighted with:

hi link ocamlTypeConstr Type

This makes the LHS of types, exceptions and constraints be matched as ocamlTypedef, which is not highlighted by default.

Variance and type variables on the LHS remain highlighted as before.

[Julow]

Ping @Maelan

[Maelan]

Thanks @Julow! I’m fine either way. I’m just slightly concerned by adding complexity to that already complex piece, but I let our maintainers judge.

You’d also have to deal with type t = ... and u = ..., though.

[Julow]

I fixed the problems with and and restored the highlighting of exception. module-rec-and (which are broken on master) and let-and are not altered.

Comparison before/after with hi link ocamlTypeConstr Type:

[copy]

Fundamentally this change is fine, but I think we leave the current behavior configurable (personally, I prefer the current behavior).

This seems to break some things, in particular (from type-linter-test.ml):

  type ('a, 'b) t = 'a list * ('a, 'b) result (* the left pair of parenthesis *)
  type nonrec 'a o = 'a option = private None | Some of 'a (* the second equal sign *)
  type t += C of int (* the of *)

The module-rec-and find is interesting, would you mind adding it to type-linter-test.ml with a fixme?

[Julow]

I missed type-linter-test.ml, thanks.

I've added a way to get back the current behavior, what should be the default ? This is governed by hi link ocamlTypeIdentifier x which could be ocamlLCIdentifier (new) or ocamlTypeConstr (previous).

I've fixed the bugs with type params and equal signs but I introduced new bugs around let-bindings, which are matched as type decl. I would like ocamlTypeDefImpl to take effect only when invoked with nextgroup, is here a way to do that ? Otherwise, we need stronger rules for let-bindings.

[Julow]

Any opinion on this ?

Here's how it can be used:

hi link ocamlTypeConstr Type
hi link ocamlTypeIdentifier Identifier

The first line is both before and after this PR. The second line is with the two line above applied:

[rgrinberg]

@copy do you want to make the call on this?

[copy]

This change and the mechanism to configure it looks good.

However, it causes some problems with the highlighting of number literals (highlighted as error across the type-linter-test.ml file), the fun keyword (highlighted like a type on line 332 and 338) and comments (a correct type highlighting is removed from line 349). You can use vim's :TOhtml followed by diff (or vimdiff, patdiff, ...) to check.

[Julow]

Thanks for the review! I fixed this issue, it was a missing ocamlContained declaration. I also changed my editor setup to use this patch everyday to be sure to test it well.

[copy]

Thanks! Super-minor, but it still removes the type highlighting from the following (on the int):

  type t (*c*) =  int

[Julow]

Good catch! I'm failing to fix it.

The ocamlTypeDef region ends at t and jump into the ocamlTypeDefImpl region, which starts at = using nextgroup. I couldn't find a way to accept the comment without interfering with nextgroup, which is very handy to handle types without RHS and and types. Any idea ?

[copy]

This comment from @Maelan looks relevant: https://github.com/ocaml/vim-ocaml/pull/81#discussion_r924975501

[Julow]

Sorry for the slow reply. I managed to handle the comment at this location while keeping nextgroup working for and types.

[copy]

The latest commit seems to have broken the highlighting of identifiers in some places (e.g. the f in line 49 is recognized as a type). You can use :TOhtml and diff to find any differences between this branch and master.

[Julow]

I think I managed to fix the identifier issue. The TOhtml diff seems fine now. The added Error class is due to a newly added test case.

[copy]

On the last commit (817e8f75a8d1324b2d28c0958534a742656d8350), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.