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

Use ocamlinterface (mli), ocamllex (mll) and menhir (mly) filetypes #61

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

Conversation

undu
Copy link

@undu undu commented Sep 17, 2020

Some ocaml extensions share the same vim filetype. This is an issue when trying to enable structural highlighting using the official ocaml tree-sitter parsers: https://github.com/tree-sitter/tree-sitter-ocaml on neovim.

In tree-sitter-ocaml there are two parsers, oner for implementation files and another for interfaces. Initially there was a single, shared one but this caused conflicts in the grammar, so it was split. The current situation makes the nvim-plugin to interpret all ocaml files as implementation files, having a separate filetype for interfaces solves the issue.

This PR adds the new filetype and at the same time tries to maintain previous behaviour, so only external plugins and scripts depending on the ocaml filetype would be affected.

The change breaks current vim plugins that only expect a single filetype for all those kind of files, this includes ocaml/merlin, for example.

The plan to have a smooth transition is:

  1. Gather plugins used by ocaml / vim users and warn users to change their vim scripts through a discuss.ocaml.org thread
  2. Add support to all the new filetypes to those (neo)vim plugins
  3. Add filetype detection for language plugins:
  4. Upstream vim-ocaml changes to vim

The same situation happens with ocamllex files. And although there's not a tree-sitter syntax for ocamlyacc/menhir, I've split it to the menhir filetype to avoid merlin from parsing them.

@undu undu force-pushed the ocaml_interface branch from 6b34d73 to 51e03f5 Compare October 9, 2020 13:54
@undu undu changed the title [RFC] Use ocamlinterface filetype for mli files Use ocamlinterface (mli), ocamllex (mll) and menhir (mly) filetypes Oct 10, 2020
@mmottl
Copy link
Collaborator

mmottl commented Oct 11, 2020

I've only done a short test of these changes, and there is still need for some improvements. The syntax highlighting of .mli files does not work with regular Vim. The syntax is set to ocamlinterface, but there is no such syntax file. I don't know how exactly this problem should be solved in a way that still fixes your issue. You may want to test the changes with regular Vim, too, to make sure that its users can upgrade seamlessly.

@undu
Copy link
Author

undu commented Oct 11, 2020

The syntax is set to ocamlinterface, but there is no such syntax file.

Sorry I didn't create the file(s), I tested it on neovim and highlighting was working as expected.
I've added them and loaded the ocaml syntax file in a similar way to how cpp does it: https://github.com/vim/vim/blob/master/runtime/syntax/cpp.vim#L13

@mmottl
Copy link
Collaborator

mmottl commented Oct 11, 2020

Thanks, the highlighting of interface files is now working again. Another problem I've just run into is that syntax errors are not being highlighted anymore by Ale in interface files. I suppose it would require changes to some Vim-packages to fully support the proposed changes.

I'm not sure how much work it would be to update other packages. I believe it could even be done before we merge these changes, which would otherwise cause problems for some people.

If at all possible, maybe it would be best to allow tree-sitter to depend on flags other than the Vim filetype? This may allow you to add full OCaml support without breaking 3rd party Vim packages.

@undu
Copy link
Author

undu commented Oct 11, 2020

If at all possible, maybe it would be best to allow tree-sitter to depend on flags other than the Vim filetype?

I'll ask around the tree-sitter community, I'm not sure if that's possible (or wanted)

I don't mind spending some effort making other plugins compatible to get this PR integrated. I think it's valuable to have a more accurate filetype for these different types of files. I've read people complaining about existing syntax problems for menhir files, having separate syntax might even be useful to fix it in the long term.

@mmottl
Copy link
Collaborator

mmottl commented Oct 11, 2020

Sounds reasonable. It may be a good idea long term to have different filetypes for interface and implementation files as well as for other files that incorporate OCaml code.

Since the proposed changes are conceivably intrusive, I should probably not be the only person to weigh in on that. Any other suggestions or opinions here on how to proceed?

@copy
Copy link
Collaborator

copy commented Nov 9, 2020

I do think this is the right way forward, since it also fixes several oddities that vim users come across every now and then:

  • merlin (at least via ale) tries to check mll and mly files, even though it doesn't have support for these
  • it's not possible to have different syntax colouring between ml and mli files, for example to make comments more readable in interface files

I would suggest announcing this change ahead of time on discuss.ocaml.org and/or the mailing lists to make people aware of this breaking change and give some time for other plugins to merge support for the new filetypes.

@undu undu force-pushed the ocaml_interface branch from 33506fe to c5ce8dd Compare January 5, 2021 22:29
@undu undu force-pushed the ocaml_interface branch from c5ce8dd to 564856b Compare May 16, 2021 16:50
undu added a commit to undu/merlin that referenced this pull request May 16, 2021
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu pushed a commit to undu/nvim-lspconfig that referenced this pull request May 17, 2021
These are ocamlinterface, ocamllex and menhir.

This is in preparation when these filetypes will be introduced in order
to keep lsp working on them. The change is backwards compatible.

For more information about the rationale of this change please read
ocaml/vim-ocaml#61
undu added a commit to undu/nvim-lspconfig that referenced this pull request May 17, 2021
These are ocamlinterface, ocamllex and menhir.

This is in preparation when these filetypes will be introduced in order
to keep lsp working on them. The change is backwards compatible.

For more information about the rationale of this change please read
ocaml/vim-ocaml#61
@undu undu force-pushed the ocaml_interface branch from 564856b to 383e7fa Compare May 19, 2021 18:52
undu added a commit to undu/merlin that referenced this pull request May 19, 2021
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request May 19, 2021
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request May 19, 2021
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request May 19, 2021
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
@undu undu force-pushed the ocaml_interface branch from 383e7fa to 564856b Compare May 22, 2021 09:13
undu added a commit to undu/merlin that referenced this pull request Jun 19, 2021
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request Jun 19, 2021
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request Feb 22, 2022
Now ocamlinterface ocamllex and menhir types must be taken into account
as well.

The filetypes are introduced by
ocaml/vim-ocaml#61
@edwintorok
Copy link

FWIW neovim-lsp-config uses "ocaml.ocamllex" and "ocaml.menhir" and "ocaml.interface".
I agree that it is nice to separate menhir/ocamllex out of the ocaml filetype, because curently if you use ocaml-lsp-server together with .mll or .mly files you get lots of warnings/errors because it tries to parse it as pure OCaml:
https://github.com/neovim/nvim-lspconfig/blob/8c9962bae5ac3b1f8b3d84c18314325f2d8df7fd/lua/lspconfig/server_configurations/ocamllsp.lua#L18
They got introduced in neovim/nvim-lspconfig@b9e79ca refering to this PR, but is the . incorrect? (this PR doesn't have a . in the filetype).

@undu
Copy link
Author

undu commented Apr 25, 2022

They got introduced in neovim/nvim-lspconfig@b9e79ca refering to this PR, but is the . incorrect? (this PR doesn't have a . in the filetype).

It's correct, that mapping is needed to translate between the vim filetype (key) and the filetype reported by LSP (the value)

@rgrinberg
Copy link
Member

@undu is this PR ready for review?

@undu
Copy link
Author

undu commented Apr 26, 2022

The merlin PR should be reviewed first: ocaml/merlin#1340

Please do test the branch and let me know if you find anything broken

@edwintorok
Copy link

I have tested this with NeoVim v0.6.1 and the new filetype detection didn't take effect (when vim-ocaml is used as a plugin). That is because filetype.vim in Neovim itself already has an autocommand that assigns a filetype to *.mll and *.mli and the ocamllex.vim in this PR uses setf which only sets the filetype if it has not been set already.

Changing ocamlinterface.vim to this makes it detect the new filetype though:

au BufNewFile,BufRead *.mli,*.mlip,*.mli.cppo set ft=ocamlinterface

I haven't tested NeoVim 0.7 yet which adds a filetype.lua (and changes how filetypes are detected instead of having N autocommands for each filetype it has one autocommand that triggers on all BufNewFile,BufRead and then checks the extension there and decides, this mode is not yet active by default though). Overriding the filetype should in theory work with both approaches.

OCamllex and menhir I can't really test yet:

  • ocamllex treesitter seems to require Neovim 0.7 because it requires grammar generation (I'll be able to test this after I upgrade). Without tree sitter there is no syntax highlighting for it though
  • menhir seems to have no tree sitter or traditional syntax highlighting

Prior to this PR I did get some highlighting for both .mll and .mly but it was often slightly wrong.

More information on how filetype works in Neovim is available here: https://neovim.io/doc/user/filetype.html#new-filetype, and for Vim here http://vimdoc.sourceforge.net/htmldoc/filetype.html#new-filetype

undu added a commit to undu/merlin that referenced this pull request May 14, 2022
Now ocamlinterface ocamllex and menhir types must be taken into account
as well. Menhir filetype is not included as there seem to be no plans to
include it and even then it would take a big effort.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request May 14, 2022
Now ocamlinterface ocamllex and menhir types must be taken into account
as well. Menhir filetype is not included as there seem to be no plans to
include it and even then it would take a big effort.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request Jan 19, 2024
Now ocamlinterface ocamllex and menhir types must be taken into account
as well. Menhir filetype is not included as there seem to be no plans to
include it and even then it would take a big effort.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request Jan 19, 2024
Now ocamlinterface ocamllex and menhir types must be taken into account
as well. Menhir filetype is not included as there seem to be no plans to
include it and even then it would take a big effort.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request Jan 29, 2024
Now ocamlinterface ocamllex and menhir types must be taken into account
as well. Menhir filetype is not included as there seem to be no plans to
include it and even then it would take a big effort.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added a commit to undu/merlin that referenced this pull request Jan 29, 2024
Now ocamlinterface ocamllex and menhir types must be taken into account
as well. Menhir filetype is not included as there seem to be no plans to
include it and even then it would take a big effort.

The filetypes are introduced by
ocaml/vim-ocaml#61
undu added 2 commits November 25, 2024 13:38
This is needed for supporting the tree-sitter parsers in ocaml because
the interface parser is different from the implementation one one.
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