feat: validate some option string values

[Akmadan23]

This can be useful to warn the user when a string value is not "accepted" in those cases where strings act like a sort of enum. I don't know if this can be considered a feature or something, let me know...

[alex-courtis]

Many thanks, this greatly improves usability.

Now we need to refactor FIELD_OVERRIDE_TYPECHECK and FIELD_SKIP_VALIDATE to use this same table structure...

Tested:

sort.sorter
view.side
view.signcolumn
renderer.highlight_opened_files
renderer.highlight_modified
renderer.icons.git_placement
renderer.icons.diagnostics_placement
renderer.icons.modified_placement

[przepompownia]

Unfortunately, Neovim crashes if a multiline message is routed through Noice. See neovim/neovim#22919 (comment)

I needed to run nvim --headless to see the a multiline validation error from here before I reduced it to the form shown in that comment.

Is \n really needed here?

[Akmadan23]

Sorry @przepompownia, but I think you posted the comment in the wrong place...

[przepompownia]

@Akmadan23 please test some invalid config using such init.lua

local configDir = vim.fn.expand('<sfile>:p:h')

vim.env['XDG_CONFIG_HOME'] = configDir
vim.env['XDG_DATA_HOME'] = configDir .. '/.xdg/data'
vim.env['XDG_STATE_HOME'] = configDir .. '/.xdg/state'
vim.env['XDG_CACHE_HOME'] = configDir .. '/.xdg/cache'
local stdPathConfig = vim.fn.stdpath('config')

vim.opt.runtimepath:prepend(stdPathConfig)
vim.opt.packpath:prepend(stdPathConfig)
local plugins_path = vim.fn.fnamemodify('plugins/', ':p')

for name, url in pairs {
  ['nvim-tree.lua'] = 'https://github.com/nvim-tree/nvim-tree.lua',
  ['nui'] = 'https://github.com/MunifTanjim/nui.nvim',
  ['noice.nvim'] = 'https://github.com/folke/noice.nvim',
} do
  local install_path = plugins_path .. name
  if vim.fn.isdirectory(install_path) == 0 then
    vim.fn.system { 'git', 'clone', '--depth=1', url, install_path }
  end
  vim.opt.runtimepath:append(install_path)
end

require('noice').setup({})
require('nvim-tree').setup({
  sort_by = 'foo',
  renderer = 1,
})

with and without \n in https://github.com/nvim-tree/nvim-tree.lua/pull/2404/files#diff-d2d7203cea884a09928ac1574693d436d6e3db28e62daabfa53f5f9f7740f51aR694

[Akmadan23]

@przepompownia ok, now I see the issue. Since nvim-tree was not mentioned anywhere in the comment you linked I assumed it was unrelated. However the \n is for sure unrelated to this specific PR, and the whole issue is probably unrelated to nvim-tree, but if you will you can open a new issue so that we can discuss about it there.

[przepompownia]

Right, this PR only revealed the problem - this line existed before but was not reached for me.

[Akmadan23]

@alex-courtis back to the topic...

Now we need to refactor FIELD_OVERRIDE_TYPECHECK and FIELD_SKIP_VALIDATE to use this same table structure...

Yeah, FIELD_OVERRIDE_TYPECHECK this way would be more intuitive and easy to read, but FIELD_SKIP_VALIDATE that only has one field (which appears twice in the config) maybe doesn't need such change?

[alex-courtis]

Unfortunately, Neovim crashes if a multiline message is routed through Noice. See neovim/neovim#22919 (comment)

I needed to run nvim --headless to see the a multiline validation error from here before I reduced it to the form shown in that comment.

Is \n really needed here?

The newline was added in an attempt to ensure that startup messages are seen. #2385 (comment)

It's an attempt to work around neovim/neovim#17832

Reverting that change.

Thanks for bringing it up @przepompownia

[alex-courtis]

Yeah, FIELD_OVERRIDE_TYPECHECK this way would be more intuitive and easy to read, but FIELD_SKIP_VALIDATE that only has one field (which appears twice in the config) maybe doesn't need such change?

You're right open_win_config is indeed a special case. We could just leave that one until another arises, if ever.

I wouldn't be unhappy if you did refactor that one though ;)

[Akmadan23]

I wouldn't be unhappy if you did refactor that one though ;)

I almost already did it, just a question: min and max fields of FIELD_OVERRIDE_TYPECHECK are from diagnostics.severity? If so in the documentation there is no mention of the possibility of being strings or functions...

Edit: never mind, I found out they are from view.width, I was confused because they are not in the default options.
Now the issue is: since view.width is already in FIELD_OVERRIDE_TYPECHECK because it can be multiple types, how do we specify the types for its elements when it's a table?

[przepompownia]

@alex-courtis thanks for 94c7c81. Adding this validation increased the chance of crash Neovim process if the user has also Noice and some invalid option. In such case the user cannot see any validation error unless see it in headless mode. Finding where the bug lies is also not trivial.

[alex-courtis]

Edit: never mind, I found out they are from view.width, I was confused because they are not in the default options.

Yup. These ambiguous fields are the reason I'd like to exactly specify them.

Now the issue is: since view.width is already in FIELD_OVERRIDE_TYPECHECK because it can be multiple types, how do we specify the types for its elements when it's a table?

I was thinking it could look like ACCEPTED_STRINGS - array rather than map of booleans. Something like

local ACCEPTED_TYPES = {
  view = {
    width = { "string", "function", "number", "table" },
    ---
  },
  ---
}

[alex-courtis]

Adding this validation increased the chance of crash Neovim process

Which validation is the one increasing the chance? Do you mean this new type checking validation specifically?

Option validation and error notification has been around for years.

Are you saying that it's just he newline that is a problem?

[Akmadan23]

I was thinking it could look like ACCEPTED_STRINGS - array rather than map of booleans.

Yes, I am indeed refactoring it following this structure, so the only solution I can think of is something like this

local ACCEPTED_TYPES = {
  view = {
    width = { "string", "function", "number", "table",
      min = { "string", "function", "number" },
      max = { "string", "function", "number" },
    },
  },
  -- ...
}

[Akmadan23]

Are you saying that it's just the newline that is a problem?

From what I understood that seems to be the case, @przepompownia correct me if I'm wrong.

[przepompownia]

Are you saying that it's just he newline that is a problem?

For me, only the newline. More precisely, this change only increases the amount of cases that lead to crash Neovim in combination with the other conditions mentioned.

I started using nvim-tree about 11 months ago. Some (currently invalid) config entries (like sort_by = 'case_insensitive') survived from the start in my configuration. They have only started to be caught with this change.