rmagatti / auto-session

A small automated session manager for Neovim
MIT License
1.19k stars 49 forks source link

[BUG] Error restoring session! ... Vim(help):E661: Sorry, no help for #325

Open powerman opened 1 month ago

powerman commented 1 month ago

Describe the bug If session file contains help buffers from plugins which will be loaded after VimEnter then session restore fails.

To Reproduce Save as repro.lua:

--[[ Minimal config to reproduce issues ]]
--
--  Usage: `nvim -u repro.lua`

-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify('./.repro', ':p')

-- set stdpaths to use .repro
for _, name in ipairs { 'config', 'data', 'state', 'cache' } do
    vim.env[('XDG_%s_HOME'):format(name:upper())] = root .. '/' .. name
end

-- bootstrap lazy
local lazypath = root .. '/plugins/lazy.nvim'
if not vim.loop.fs_stat(lazypath) then
    vim.fn.system {
        'git',
        'clone',
        '--filter=blob:none',
        '--single-branch',
        'https://github.com/folke/lazy.nvim.git',
        lazypath,
    }
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
    'folke/tokyonight.nvim',
    -- add any other plugins here
    {
        'rmagatti/auto-session',
        version = '*',
        lazy = false, -- Needs to restore session on Neovim start.
        init = function()
            vim.opt.sessionoptions:append 'winpos'
            vim.opt.sessionoptions:append 'localoptions'
        end,
        config = true,
    },
    {
        'folke/todo-comments.nvim',
        version = '*',
        lazy = true, -- Must be loaded but not critical, so let's use event VeryLazy.
        event = 'VeryLazy',
    },
}
require('lazy').setup(plugins, {
    root = root .. '/plugins',
})

vim.cmd.colorscheme 'tokyonight'
-- add anything else here

Then:

$ nvim -u repro.lua +qa
$ nvim -u repro.lua
:help todo-comments.nvim.txt
:qa
$ nvim -u repro.lua
Error detected while processing VimEnter Autocommands for "*":
auto-session ERROR: Error restoring session! The session might be corrupted.
Disabling auto save. Please check for errors in your config. Error:
vim/_editor.lua:0: VimEnter Autocommands for "*"..script nvim_exec2() called at VimEnter Autocommand
s for "*":0../home/powerman/.config/nvim/.repro/data/nvim/sessions/%home%powerman%.config%nvim.vim,
line 20: Vim(help):E661: Sorry, no 'en' help for todo-comments.nvim.txt
Press ENTER or type command to continue

Expected behavior I expect to restore at least all other buffers/windows/tabs, excluding missing help files, but only ones which was restored before this error are actually restored.

Baseline (please complete the following information):

sessionoptions=blank,buffers,curdir,folds,help,tabpages,winsize,terminal,winpos,localoptions

Linux powerman 6.6.38-gentoo #1 SMP PREEMPT_DYNAMIC Tue Jul  9 23:26:22 EEST 2024 x86_64 AMD Ryzen 9 5900X 12-Core Processor AuthenticAMD GNU/Linux

NVIM v0.10.0
Build type: Release
LuaJIT 2.1.0-beta3
Compilation: /usr/lib/ccache/bin/x86_64-pc-linux-gnu-gcc -march=native -O2 -pipe   -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wvla -Wdouble-promotion -Wmissing-noreturn -Wmissing-format-attribute -Wmissing-prototypes -fsigned-char -fstack-protector-strong -Wno-conversion -fno-common -Wno-unused-result -Wimplicit-fallthrough -fdiagnostics-color=always  -DUNIT_TESTING -DHAVE_UNIBILIUM -D_GNU_SOURCE -DINCLUDE_GENERATED_DECLARATIONS -I/usr/include/luajit-2.1 -I/usr/include -I/var/tmp/portage/app-editors/neovim-0.10.0-r1/work/neovim-0.10.0_build/src/nvim/auto -I/var/tmp/portage/app-editors/neovim-0.10.0-r1/work/neovim-0.10.0_build/include -I/var/tmp/portage/app-editors/neovim-0.10.0-r1/work/neovim-0.10.0_build/cmake.config -I/var/tmp/portage/app-editors/neovim-0.10.0-r1/work/neovim-0.10.0/src 

   system vimrc file: "/etc/vim/sysinit.vim"
  fall-back for $VIM: "/usr/share/nvim"

Run :checkhealth for more info

Additional context

cameronr commented 1 month ago

Thanks for sending a precise repro! There isn't much we can do about the error as it's generated by neovim directly. That said, removing help from the sessionoptions will avoid the error.

sessionoptions=blank,buffers,curdir,folds,tabpages,winsize,terminal,winpos,localoptions

Which raises the question of what the docs should recommend. On one hand, some folks may like to have help windows restored for things that are loaded before we try to restore the session. OTOH, it can break the session if it's a help window for a plugin that's lazy loaded. Probably the right answer is to at least warn in the docs and say to remove it if the problem comes up.

powerman commented 1 month ago

How about analysing session file before loading and doing require(...) to force Lazy load plugins related to required help files?

powerman commented 1 month ago

Or just adding their paths to some variable to make just a help files loadable even without Lazy knowledge?

powerman commented 1 month ago

Or try to restore session as is, detect (and hide) an error, and in case of error TEMPORARY remove help from sessionoptions and try again?

cameronr commented 1 month ago

Those are some interesting ideas but probably too niche / too variable for a good heuristic to cover enough of the cases to be worth doing, IMO. Have you seen anything similar for something other than help pages?

Or try to restore session as is, detect (and hide) an error, and in case of error TEMPORARY remove help from sessionoptions and try again?

I tested this case and it won't work. It seems like sessionoptions controls what's written to the session file but not how a session is read, so the error still happens even when help isn't in sessionoptions when the session is read.

powerman commented 1 month ago

Have you seen anything similar for something other than help pages?

I'm not sure is there are other similar issues. After all, it's not about "help pages" per se, it's about lazy loading - which affect only rtp and things which expect it to include some plugin's dir.

cameronr commented 1 month ago

A session file is just a series of vim commands that are run when the session is sourced and it's explicitly a help command that's failing in this issue:

...
exe '2resize ' . ((&lines * 31 + 32) / 65)
argglobal
enew | setl bt=help
help todo-comments.nvim.txt@en
balt a/a2.txt
setlocal keymap=
...

I was asking if there are other cases of things not loading correctly other than help to see if there's a more generalized problem or if it's a problem that only shows up for help pages for things that have been lazy loaded.

rmagatti commented 1 month ago

Does it work if you set the plugin as a dependency of auto-session like so?

--[[ Minimal config to reproduce issues ]]
--
--  Usage: `nvim -u repro.lua`

-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify('./.repro', ':p')

-- set stdpaths to use .repro
for _, name in ipairs { 'config', 'data', 'state', 'cache' } do
  vim.env[('XDG_%s_HOME'):format(name:upper())] = root .. '/' .. name
end

-- bootstrap lazy
local lazypath = root .. '/plugins/lazy.nvim'
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system {
    'git',
    'clone',
    '--filter=blob:none',
    '--single-branch',
    'https://github.com/folke/lazy.nvim.git',
    lazypath,
  }
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  'folke/tokyonight.nvim',
  -- add any other plugins here
  {
    'rmagatti/auto-session',
    version = '*',
    lazy = false, -- Needs to restore session on Neovim start.
    init = function()
      vim.opt.sessionoptions:append 'winpos'
      vim.opt.sessionoptions:append 'localoptions'
    end,
    dependencies = {
      "folke/todo-comments.nvim"
    },
    config = true,
  },
  {
    'folke/todo-comments.nvim',
    version = '*',
    lazy = true, -- Must be loaded but not critical, so let's use event VeryLazy.
    event = 'VeryLazy',
  },
}
require('lazy').setup(plugins, {
  root = root .. '/plugins',
})

vim.cmd.colorscheme 'tokyonight'
-- add anything else here

The reason I ask is because I agree with @cameronr here that there's probably not a good heuristic to solve this, parsing the vim session file and forcing a specific order of plugin loading feels risky at best and like overreach from auto-session at worst; if adding plugins you normally have buffers open for in your sessions as a dependency solves the issue, that sounds like the more user-driven approach since everyone will have their own versions of this. This isn't a final opinion by any means but it's at least what's going through my head on this one so far

powerman commented 1 month ago

adding plugins you normally have buffers open for

I don't have some specific help files open "normally". Any one of them can be opened. So this way we effectively disable lazy loading for all plugins.

cameronr commented 1 month ago

As described in #337, I think we can actually keep loading the session file even if there are errors.

I whipped up a test branch if you want to give it a try. You'll have to update your auto-session config as follows:

-- 'rmagatti/auto-session', 'cameronr/auto-session', branch = 'silent-source',

After changing the config, you'll have to open Lazy and update the plugin to actually pull it down (and restart nvim just to be sure)

cameronr commented 1 month ago

I just retested your repro case and it will show an error on loading the session but it will still restore the session as desired. You will still have to re-enable autosaving in that case, tho, as we disable it when there's an error

powerman commented 1 month ago

You will still have to re-enable autosaving in that case, tho, as we disable it when there's an error

Sounds like it makes more harm than good. This way we not only get partially restored session but also will lose all changes in that session if user forgets to re-enable autosaving (and user will forget it for sure).

powerman commented 1 month ago

My point is: auto-saving is much more valuable than auto-restoring everything. It's better to not restore some buffers (and let user handle partially restored session manually) than not restore all of them because of error with some of them. It's better to save current session in any case (unless user has manually disabled auto-saving) than keep old session with errors unmodified.

cameronr commented 1 month ago

Maybe I wasn't clear, the new behavior in that branch will restore as much of the session as possible (instead stopping at the first error like before), so I think that's strictly better.

Disabling of autosave if there's an error is the same as it was before and I think that's the safer default as it gives the user time to debug the error if they want to.

I could imagine a setting (eg 'disable_autosave_on_restore_error') that could be set to false for those that want to keep autosave on even after an error. The other option is having an 'on_restore_error' hook that would let the user decide the behavior (eg they could re-enable auto saving in that case). i probably lean towards the hook as the more general solution.

@rmagatti any preference?

powerman commented 1 month ago

Debugging is a valid reason but most users are not interested in this so it (disabling autosave on restore error) shouldn't be default behaviour IMO.

rmagatti commented 3 weeks ago

This setting is true by default not because of debugging, but because if the session is saved after errors occur it can essentially cause undefined behaviour. At times I'd see errors for things that weren't valid anymore but because the session was saved after the errors occurred, some errors would manifest on load. So ultimately this is done to protect the user against much more frustrating behaviour down the road

powerman commented 3 weeks ago

@rmagatti Can you please provide an example of such an issue?

Probably I miss something, but I can't see how this may hurt UX. After session (partially) fails to restore I see these possible cases:

Also, let's consider user got error on restore and did nothing because of this (didn't re-enabled autosave or saved session manually - I suppose this will happens in most cases but it's a speculation of course). I suppose in this case user gets same error on next nvim start anyway - how it's differ from the case you've explained when auto-enabled autosave after error will save session which will error on restore?