refactor(runtime): convert more dist#ft functions to lua

[smjonas]

Follow-up to #18247 (working on #18241) .

[smjonas]

Converting the functions is going pretty smooth, however I think we should establish some rules to follow. Please let me know how you think these cases should be handled. This should probably be documented somewhere as well?

1. Vim often uses \< and \> for start / end of a word. Suggestion: if this occurs at the end of a pattern, simply omit it. If in the middle, use a . instead.
2. Usage of =~. Suggestion: assume ignorecase (i.e. convert the line to lowercase first and check afterwards).
3. Usage of \k. Suggestion: simply use ..

What do you think?

I don't think any of these changes will lead to test failures as the tests are not that detailed.

[smjonas]

@gpanders Should I stop adding new functions here and leave the remaining 9 for round three? 😄

[gpanders]

Yes that sounds good, it looks like there is already enough here to keep me busy reviewing 😄

[smjonas]

That's definitely true😅I will let you know once all tests pass and everything looks good to me

[clason]

1. Vim often uses \< and \> for start / end of a word. Suggestion: if this occurs at the end of a pattern, simply omit it. If in the middle, use a . instead.

I'll defer to @gpanders on this; I'm barely regex-literate.

2. Usage of =~. Suggestion: assume ignorecase (i.e. convert the line to lowercase first and check afterwards).

That sounds good to me.

3. Usage of \k. Suggestion: simply use ..

See 1.; however this is decided, I think it would be good to document this in a comment block at the top of the file. (Also previous decisions like find over match and sub.) This will be very valuable when new dist#ft functions have to be ported in the future.

I don't think any of these changes will lead to test failures as the tests are not that detailed.

And if they do, we will know :)

Should I stop adding new functions here and leave the remaining 9 for round three? 😄

Don't forget scripts.vim, which is the boss battle :P

[smjonas]

@gpanders I noticed there is no code in filetype.lua referencing 4 of these patterns (except one for upstreamlog). Which seems to be the reason why some of the tests fail. Does anything need to be added there?

" Innovation Data Processing
au BufRead,BufNewFile upstream.dat\c,upstream.*.dat\c,*.upstream.dat\c	setf upstreamdat
au BufRead,BufNewFile fdrupstream.log,upstream.log\c,upstream.*.log\c,*.upstream.log\c,UPSTREAM-*.log\c	setf upstreamlog
au BufRead,BufNewFile upstreaminstall.log\c,upstreaminstall.*.log\c,*.upstreaminstall.log\c setf upstreaminstalllog
au BufRead,BufNewFile usserver.log\c,usserver.*.log\c,*.usserver.log\c	setf usserverlog
au BufRead,BufNewFile usw2kagt.log\c,usw2kagt.*.log\c,*.usw2kagt.log\c	setf usw2kagtlog

[clason]

That's what #18219 is for...

[smjonas]

Oh, makes sense, so would it be better if I refrained from calling the new dat function for now until that PR is merged?

[clason]

Or you could add the patterns, whichever is easier.

[smjonas]

I am not really sure how I'm supposed to resolve the merge conflicts 😄 Is this even necessary or can this be done once this PR is ready to be merged? Rebasing on master sounds simple but I'm not that experienced with Git. Please let me know :)

[clason]

I would do the following:

1. download the stylua config directly to the neovim/ directory from https://raw.githubusercontent.com/neovim/neovim/master/.stylua.toml
2. run (from the repo root) stylua runtime/lua/vim/filetype.lua runtime/lua/vim/filetype/detect.lua
3. commit the result and squash your commits
4. from the Github page for your fork, select "Fetch upstream" (in the top bar "This branch is X commits behind...")
5. from the repo root, do git checkout master; git pull; git checkout filetype-detect; git rebase master (you might have to remove the .stylua.toml first)
6. git push -f

And, yes, you can do that whenever you are ready; it doesn't have to be now.

[smjonas]

With almost all tests passing (except the ones for gomod files, maybe you can figure out what's wrong), this is ready for a review ^^ @gpanders

[gpanders]

1. Vim often uses \< and \> for start / end of a word. Suggestion: if this occurs at the end of a pattern, simply omit it. If in the middle, use a . instead.

I've been thinking about this, and I think that we should be prioritizing correctness over performance. There's a chance that the patterns in filetype.vim that are using \< and \> don't actually need to be using them... but that would be very hard to verify, so I lean toward "trust the people who created the patterns in the first place, as they know the requirements of their filetypes better than I do". So I've come around to just using vim.regex() for cases like this. It is slower than plain Lua pattern matching, but we're not detecting millions of filetypes in a hot loop so it's ok to lose a little bit of perf if it means we're more correct.

2. Usage of =~. Suggestion: assume ignorecase (i.e. convert the line to lowercase first and check afterwards).

This seems like a safe assumption. Users can file an issue if this causes missed detections.

3. Usage of \k. Suggestion: simply use ..

See my response to point 1. I think we should just use vim.regex() for these after all.

[gpanders]

This seems like a safe assumption. Users can file an issue if this causes missed detections.

Actually I want to amend this. I think for cases of =~ we should use the following heuristic:

1. If the string being compared against has any upper case letters, treat the comparison as case sensitive
2. Otherwise, treat it as case-insensitive.

Basically we are applying 'smartcase'. If the author of the code explicitly used upper case characters then it's likely that case does matter (they should have used =~# but that's an easy mistake to make in Vim script).

[gpanders]

@smjonas I also just remembered that vim.regex() is case insensitive by default, so there's no need to :lower() strings before passing them to match_str().

In the cases where we need case-sensitivity, we'll need to specify \C at the beginning of the regex.

[gpanders]

@smjonas I had an idea: to avoid repeatedly re-compiling the same regex patterns over and over, we could create a cache so that each regex is only compiled once and then re-used if necessary. Something like this:

local matchre = (function()
  local cache = {}
  return function(line, regex)
    if not cache[regex] then
      cache[regex] = vim.regex(regex)
    end

    return cache[regex]:match_str(line)
  end
end)()

And then instances of e.g. vim.regex([[\<some pattern\>]]):match_str(line) could be replaced with matchre(line, [[\<some pattern\>]]).

matchre is just a name I came up with, we can use something else if you have a better idea.

[smjonas]

@gpanders I refactored to use matchre but can't figure out why the remaining tests fail...

[clason]

matchre is just a name I came up with, we can use something else if you have a better idea.

I'd suggest matchregex -- not much longer, more explicit, and matches vim.regex?

[gpanders]

I think the reason the tests are failing is that vim.regex() (in contradiction with its documentation in :h vim.regex() is not case insensitive by default. I wonder if the docs meant to say that it is case sensitive by default "regardless of 'ignorecase'".

In any case, I'm sorry that I misled you, I only just now discovered this fact while looking into why the tests are failing.

[smjonas]

No worries, thanks for letting me know! Let's see if the tests pass now :)

[smjonas]

Squishy squash? :D

[clason]

Time for a squash, indeed!

[clason]

Looks good to me (except the minor doc nit) -- test failures are the usual CI flake.

I didn't check the patterns/regex in detail, though, so @gpanders has the final word.

[clason]

Thank you! This is shaping up nicely. Onward and upward!