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:

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.

[Julow]

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

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.

[Julow]

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

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.

[Julow]

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

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.

[Julow]

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

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.

[Julow]

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

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.

[Julow]

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

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.

[Julow]

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

typefoo = 'abarlist

foo is not highlighted with:

hilinkocamlTypeConstrType

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.

[Maelan]

I note that with this technique, there must be no comment (* *) between the defined type name and the equal sign =, which may be acceptable (we already have that limitation elsewhere).

[Julow]

I don't see this. Perhaps because the end regex don't contain \@= ?

[Maelan]

I haven’t actually tested your code, but from what I understand about nextgroup= and contained, the ocamlTypeDefImpl group will never be tried except immediately after the end of the ocamlTypeDef group.

[Julow]

This is what skipwhite is doing, from the :h:

These arguments are only used in combination with "nextgroup".	They can be
used to allow the next group to match after skipping some text:
skipwhite	skip over space and tab characters
skipnl skip over the end of a line
skipempty	skip over empty lines (implies a "skipnl")

I took thought this was doing something between start and end and nothing outside but it's actually the opposite, they take effect after end and do nothing inside the region (whitespace are skipped by default it seems).

[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 = 'alist* ('a, 'b) result(* the left pair of parenthesis *)typenonrec 'a o = 'aoption=privateNone | Someof'a(* the second equal sign *)typet += Cofint(* 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:

hilinkocamlTypeConstrTypehilinkocamlTypeIdentifierIdentifier

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):

typet (*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: #81 (comment)

[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 (817e8f7), some highlights are still incorrect (e.g. the let f on line 49), and show up in the diff.