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

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

Open
powerman opened this issue Jul 10, 2024 · 20 comments
Open
Assignees
Labels
bug Something isn't working

Comments

@powerman
Copy link

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

  • Make this error non-fatal and continue to load session somehow?
  • Add an option to exclude ALL help buffers from saved sessions?
  • If this can't be solved then it makes sense to at least document this behaviour as an incompatibility with lazy loading in general.
@powerman powerman added the bug Something isn't working label Jul 10, 2024
@cameronr
Copy link
Collaborator

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
Copy link
Author

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

@powerman
Copy link
Author

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

@powerman
Copy link
Author

powerman commented Jul 11, 2024

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Collaborator

cameronr commented Aug 5, 2024

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
Copy link
Collaborator

cameronr commented Aug 5, 2024

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
Copy link
Author

powerman commented Aug 5, 2024

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
Copy link
Author

powerman commented Aug 5, 2024

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
Copy link
Collaborator

cameronr commented Aug 5, 2024

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
Copy link
Author

powerman commented Aug 5, 2024

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
Copy link
Owner

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
Copy link
Author

@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:

  • It can be saved and then restored without issues. In this case auto-enabling autosave is a good thing.
  • It can be saved but will have error on restore. Here we've two sub-cases:
    • It'll restore nothing at all. In this case we'll have just annoying error for user on each nvim start. User can manually fix this by executing :SessionDelete.
    • It'll restore session partially. IMO this is better than nothing (in case user won't remember to re-enable autosaving manually after restore error).

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?

@rmagatti
Copy link
Owner

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:

  • It can be saved and then restored without issues. In this case auto-enabling autosave is a good thing.

  • It can be saved but will have error on restore. Here we've two sub-cases:

    • It'll restore nothing at all. In this case we'll have just annoying error for user on each nvim start. User can manually fix this by executing :SessionDelete.
    • It'll restore session partially. IMO this is better than nothing (in case user won't remember to re-enable autosaving manually after restore error).

The misconception here is that these are the only possibilities. The third possibility is that the session saves with invalid configs, then suppose you go ahead the fix the errors in your config, you then attempt to restore that session and keep getting the same errors, you're left wondering why the fix in your config doesn't seem to take effect; until you delete the "corrupted" session file and create a new session, the error remains. This was the real reason for disabling auto save after errors restoring, like I mentioned, it leads to undefined behaviour but it also leads to issues that are hard to notice and fix when "corrupted sessions" are saved.

@powerman
Copy link
Author

You're right, this might happens sometimes. But isn't such a differences between current config and config in a saved session are anyway possible even without obvious errors on restore? AFAIR it often happens in workflow like this:

  1. You've noticed some issue with vim setup.
  2. You've executed some :commands trying to figure it out and fix.
  3. After fix was found you've added required changes to vim config and restarted vim.
  4. But in current session some leftovers from your experiments at step 2 are still applied and this feels like either your changes in vim config wasn't correct or the current session is "broken".

Still, I'd prefer to have a "broken" session with my files than a "bad surprise" when all my changes in a current session will be just lost.

Is there any other possibilities to solve such an UX issue? Maybe it's possible to just ask user to save current "broken" session or not on some event like VimLeavePre?

@rmagatti
Copy link
Owner

rmagatti commented Dec 3, 2024

I do see where you're coming from on this. Here's my take:
We can do what @cameronr suggested in his comment. A config instead of a hook is best for this specific case, as like other configs, it controls a very specific behaviour of the plugin explicitly.

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.

The user can then control the behaviour themselves, but by default the current behaviour is safer and has already saved me personally many times from long session debugging so I am not willing to change that up.

Thanks for the input on this so far!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants