Detect original filetype of an embedded puppet file

shadowwa commented 1 year ago

vim-puppet is currently loading syntax/sh.vim and adding specific puppet region for highlighting.

I tried to detect more finely the original type by asking vim to do a filetypedetect on a filename without 'epp' extension. I also changed the &filetype value to originalfiletype.epuppet so various plugins like ALE or coc.nvim recognise the original filetype and allow linting or autocompletion I tried yaml, toml, various conf file and didn't find any drawback.

lelutin commented 4 months ago

thanks for working on this @shadowwa ! it was something that I wanted to get to for a while

I've tried the changes in this PR and so far the results are:

the "sh" default syntax somehow does not seem to get loaded
I tried opening a file name templates/config.inc.php.epp and the underlying type was not detected as php

should number 2 have figured out the type via the file name?

shadowwa commented 4 months ago

Strange, I tried with a minimal .vimrc to be sure that I haven't any other plugin or configuration interfering

call plug#begin()
Plug 'shadowwa/vim-puppet', { 'branch': 'detect_original_filetype','for': ['puppet', 'epuppet'] }
call plug#end()

and it's working fine

a simple .epp file without any shebang : sh and epuppet highlight, filetype detected by vim: epuppet
a simple .epp file with shebang !/bin/bash: sh and epuppet highlight, filetype detected by vim: sh.epuppet
a .sh.epp file: sh and epuppet highlight, filetype detected by vim: sh.epuppet
a .php.epp file: php and epuppet highlight, filetype detected by vim: php.epuppet

i initially used vim 9.0 but I have the same behaviour with vim 9.1 and neovim 0.7.2 and neovim 0.9.5

lelutin commented 4 months ago

hmm strange.. I've commented out everything in my .vimrc and kept just the plugin activation like in your example, and disabled all other loaded files except for plug.vim and I'm still seeing the same thing.

when I do :echo &ft I get epuppet.epuppet

I'm using vim 9.1 in debian sid.

I'll try and do some tracing of the filetype detection to see if I can understand what's happening

lelutin commented 4 months ago

err ok something's off on my side. I cloned your repo and switched to the branch for this PR and now it's working as expected. I'll see if I have some weird state in my clone of rodjek's repository

lelutin commented 4 months ago

ah! I found something out. vim 9.1 has something that was merged upstream and it's already defining the 'epuppet' filetype detection:

/usr/share/vim/vim91/filetype.vim:au BufNewFile,BufRead *.epp       setf epuppet

so the issue is not with your code but with this file short-circuiting things

lelutin commented 4 months ago

@shadowwa I took a peek at how subtype detection is currently done in upstream vim for eruby and I was able to replicate what it does for epuppet, squashing the bug that I have in vim 9.1

Basically the difference is that subtype detection is done inside ftplugin/$type.vim instead of inside ftdetect/. Also the type detection for eruby avoids calling out to doautocmd, I'm guessing that's in order to avoid type detection loops or weird conditions.

I've prepared a branch in my own fork that builds on top of your branch to get the final result that works for me in 9.1: https://github.com/lelutin/vim-puppet/tree/detect_epuppet_subtype

can I ask you to test if that works well for you as well in 9.0 ? if it does, we'll only need to add some unit tests for the new subtype detection and then we can merge this feature to master in this repo

shadowwa commented 4 months ago

The first version of the patch I wrote was also base on the eruby.vim but I found out that it was only taking into account the file extension and not the path or shebang as vim would do for files without .epp. for example: if opening a file templates/etc/apache/test.conf.epp, templates/etc/sudoers.d/test.epp or a perlscript with shebang but no extension, the eruby way would detect conf.epuppet or just epuppet instead of apache.epuppet, sudoers.epuppet and perl.epuppet respectively.

I tried to reproduce your error with my patch by using a fresh installed version of bookworm upgraded to sid but didn't succeeded. here are the diffents result I got:

	debian bookworm vim 9.0.1378-2	ubuntu mantic vim 9.0.1672	debian sid vim 9.1.0016	debian bookworm neovim 0.7.2-7	ubuntu mantic neovim 0.9.5-6	mageia 10 vim 9.1.111	mageia 10 neovim 0.9.5-1
patch shad
perl.epp	perl.epuppet	perl.epuppet	perl.epuppet	perl.epuppet	perl.epuppet	perl.epuppet	perl.epuppet
shell.epp	epuppet	epuppet.epuppet [^1]	epuppet	epuppet	epuppet	epuppet	epuppet
shebanshell.epp	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet
shell.sh.epp	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet
scriptweb.php.epp	php.epuppet	php.epuppet	php.epuppet	php.epuppet	php.epuppet	php.epuppet	php.epuppet
etc/apache/test.conf.epp	apache.epuppet	apache.epuppet	apache.epuppet	apache.epuppet	apache.epuppet	apache.epuppet	apache.epuppet

patch lutin
perl.epp	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	epuppet [^2][^3]	.epuppet [^2]
shell.epp	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	epuppet [^2][^3]	.epuppet [^2]
shebanshell.epp	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	.epuppet [^2]	epuppet [^2][^3]	.epuppet [^2]
shell.sh.epp	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	sh.epuppet	epuppet [^3]	sh.epuppet
scriptweb.php.epp	php.epuppet	php.epuppet	php.epuppet	php.epuppet	php.epuppet	epuppet [^3]	php.epuppet
etc/apache/test.conf.epp	conf.epuppet	conf.epuppet	conf.epuppet	conf.epuppet	conf.epuppet	epuppet [^2]	conf.epuppet

[^2]: seem there is a uninitialized variable, I got the following error each time: "templates/perl.epp" 6L, 70B Error detected while processing BufRead Autocommands for ".epp"..FileType Autocommands for "epuppet"..function 4_lod_ft[4]..4_doautocmd[2]..FileType Autocommands for ""..function 7_LoadFTPlugin[18]..s cript /home/shad/.vim/plugged/vim-puppet/ftplugin/epuppet.vim: line 40: E121: Undefined variable: g:epuppet_default_subtype Press ENTER or type command to continue

[^3]: the vim packaged by mageia give a 'epuppet' filetype every time, the same version from appimage give same results than other vim versions

[^1]: vim is slow to start (3seconds) and if I tried the same vim version from appimage, vim start instantaneously and the filetype is epuppet like othe (neo)vim versions

lelutin commented 3 months ago

huh interesting, thanks for doing all of those tests on many different releases. it's kind of a shame how much effort we must put on the github CI in order to test in other releases

I

lelutin commented 3 months ago

Gah.. I hit something by mistake while typing. Didn't mean to close this.

I was going to say: I did some additional testing just now and somehow now I'm not getting epuppet.puppet with your code anymore. However, I can see that the tests re-ran because of my accidentally closing the PR and they are exhibiting what I used to see. e.g. the filetype detection ends up with epuppet.epuppet:

  Starting Vader: /home/runner/work/vim-puppet/vim-puppet/test/filetype/epuppet.vader
    (1/2) [EXECUTE] Filetype detection on a new empty file
    (1/2) [EXECUTE] (X) 'epuppet' should be equal to 'epuppet.epuppet'
    (2/2) [  GIVEN] epuppet file with leading tag
    (2/2) [EXECUTE] trigger filetype detection
    (2/2) [   THEN] should be detected as epuppet
    (2/2) [   THEN] (X) 'epuppet' should be equal to 'epuppet.epuppet'

So your code does a better job at detecting than mine, but the detection is flaky. I'm currently out of ideas for where to direct things. Do you have an idea for how we could make detection in your code more reliable, given the presence of filetype detection in upstream vim? maybe we could try to add sub-type detection in upstream vim directly?

shadowwa commented 3 months ago

while trying to complete vader tests, I saw that (even on the rodjek/vim-puppet master branch) all tests for file detection using

Given (buffer content):
  a buffer content

Execute (trigger filetype detection):
  file filename.extension
  edit

Then (should be detected as extension):
  AssertEqual &filetype, 'extension'

failed when using vim from latest debian, ubuntu or mageia (and neovim 9.5). Did you also noticed a failure of these tests on the rodjek/vim-puppet master branch?

Using vim appimage, I was able to see that while version 9.0.0260 was working fine, 9.0.0270 broke the tests. I suspected https://github.com/vim/vim/commit/2eddbacd6dc17c84e4bdc41e60e81949a36bb973 and tried to use true file and replacing the previous tests by

Execute (Load true file):
  edit test/test-files/filename.extension
  AssertEqual &filetype, 'extension'

I've created another branch to test and it seems to work for vim above 9.0.0270 and with neovim 9.5 https://github.com/shadowwa/vim-puppet/tree/test-with-files It wont solve detection problems from my code but at least, results from vader test and manual test should be more consistent. If you're ok with theses test, I will update them here while adding other filetype detection tests.

shadowwa commented 3 months ago

I've found another reason explaining the "flakiness" of our results: I assumed that loading the plugin by modifying rtp (as done in test/init.vim) or using plug-vim would be the same, but I was wrong:

when modifying rtp option, the upstream filetype detection of vim come in the way and produce the epuppet.epuppet we were getting sometime.
when loading the plugin with vim-plug, the manager seems to change the loading order of runtime scripts by sourcing them instead of just modifiying rtp options.

To solve the problem, I just have to do the same thing that is done for .pp file: use au! instead of au.

Now the subfiletype is correctly detected on vim and nvim (at least up to v0.7.2 from mantic) and even solve the slow starting and epuppet.epuppet I got on vim native package of mantic. There is still a problem left: neovim from version 8.0 switched filedetection from filetype.vim to filetype.lua and the symptom of epuppet.epuppet detection persists (everything works fine when using vim-plug). I've yet have to find how to override the native detection for epp files.

shadowwa commented 3 months ago

I've finally solved the filetype detection for neovim >= 0.8.0. Had to redo the same detection in ftdetect/puppet.lua and disable the autocmd in ftdetect/puppet.vim to be able to override the native detection on recent neovim. The detection is now correct for vim and neovim (0.7.0, 0.8.0, 0.9.0 and 0.9.5) with or without plug-vim

rodjek / vim-puppet

Detect original filetype of an embedded puppet file #146