shada: Continue dumping when variables failed to dump

Closes #3721

3 files changed, 102 insertions, 50 deletions

diff --git a/src/nvim/shada.c b/src/nvim/shada.c
index 340c14066a..046d9285f7 100644
--- a/src/nvim/shada.c
+++ b/src/nvim/shada.c
@@ -122,7 +122,7 @@ KHASH_SET_INIT_STR(strset)
 //   E576: Missing '>'
 //   E577: Illegal register name
 //   E886: Can't rename viminfo file to %s!
-// Now only five of them are used:
+// Now only six of them are used:
 //   E137: ShaDa file is not writeable (for pre-open checks)
 //   E138: All %s.tmp.X files exist, cannot write ShaDa file!
 //   RCERR (E576) for critical read errors.
@@ -130,6 +130,7 @@ KHASH_SET_INIT_STR(strset)
 //   RERR (E575) for various errors inside read ShaDa file.
 //   SERR (E886) for various “system” errors (always contains output of
 //   strerror)
+//   WERR (E574) for various ignorable write errors
 
 /// Common prefix for all errors inside ShaDa file
 ///
@@ -148,6 +149,9 @@ KHASH_SET_INIT_STR(strset)
 /// Common prefix for all “rename” errors
 #define RNERR "E136: "
 
+/// Common prefix for all ignorable “write” errors
+#define WERR "E574: "
+
 /// Flags for shada_read_file and children
 typedef enum {
   kShaDaWantInfo = 1,       ///< Load non-mark information
@@ -198,6 +202,9 @@ typedef enum {
                          ///< a ShaDa file.
   kSDWriteFailed,        ///< Writing was not successfull (e.g. because there
                          ///< was no space left on device).
+  kSDWriteIgnError,      ///< Writing resulted in a error which can be ignored
+                         ///< (e.g. when trying to dump a function reference or
+                         ///< self-referencing container in a variable).
 } ShaDaWriteResult;
 
 /// Flags for shada_read_next_item
@@ -1666,11 +1673,14 @@ static char *shada_filename(const char *file)
 /// @param[in]  entry      Entry written.
 /// @param[in]  max_kbyte  Maximum size of an item in KiB. Zero means no
 ///                        restrictions.
-static bool shada_pack_entry(msgpack_packer *const packer,
-                             ShadaEntry entry,
-                             const size_t max_kbyte)
+///
+/// @return kSDWriteSuccessfull, kSDWriteFailed or kSDWriteIgnError.
+static ShaDaWriteResult shada_pack_entry(msgpack_packer *const packer,
+                                         ShadaEntry entry,
+                                         const size_t max_kbyte)
   FUNC_ATTR_NONNULL_ALL
 {
+  ShaDaWriteResult ret = kSDWriteFailed;
   msgpack_sbuffer sbuf;
   msgpack_sbuffer_init(&sbuf);
   msgpack_packer *spacker = msgpack_packer_new(&sbuf, &msgpack_sbuffer_write);
@@ -1742,6 +1752,9 @@ static bool shada_pack_entry(msgpack_packer *const packer,
       msgpack_pack_array(spacker, arr_size);
       PACK_BIN(cstr_as_string(entry.data.global_var.name));
       if (vim_to_msgpack(spacker, &entry.data.global_var.value) == FAIL) {
+        ret = kSDWriteIgnError;
+        EMSG2(_(WERR "Failed to write variable %s"),
+              entry.data.global_var.name);
         goto shada_pack_entry_error;
       }
       DUMP_ADDITIONAL_ELEMENTS(entry.data.global_var.additional_elements);
@@ -1946,11 +1959,11 @@ static bool shada_pack_entry(msgpack_packer *const packer,
   }
   msgpack_packer_free(spacker);
   msgpack_sbuffer_destroy(&sbuf);
-  return true;
+  return kSDWriteSuccessfull;
 shada_pack_entry_error:
   msgpack_packer_free(spacker);
   msgpack_sbuffer_destroy(&sbuf);
-  return false;
+  return ret;
 }
 #undef PACK_STRING
 
@@ -1965,13 +1978,13 @@ shada_pack_entry_error:
 ///                        is assumed that entry was already converted.
 /// @param[in]  max_kbyte  Maximum size of an item in KiB. Zero means no
 ///                        restrictions.
-static bool shada_pack_encoded_entry(msgpack_packer *const packer,
-                                     const vimconv_T *const sd_conv,
-                                     PossiblyFreedShadaEntry entry,
-                                     const size_t max_kbyte)
+static ShaDaWriteResult shada_pack_encoded_entry(msgpack_packer *const packer,
+                                                 const vimconv_T *const sd_conv,
+                                                 PossiblyFreedShadaEntry entry,
+                                                 const size_t max_kbyte)
   FUNC_ATTR_NONNULL_ALL
 {
-  bool ret = true;
+  ShaDaWriteResult ret = kSDWriteSuccessfull;
   if (entry.can_free_entry) {
     ret = shada_pack_entry(packer, entry.data, max_kbyte);
     shada_free_shada_entry(&entry.data);
@@ -2244,9 +2257,7 @@ static inline ShaDaWriteResult shada_read_when_writing(
         assert(false);
       }
       case kSDItemUnknown: {
-        if (!shada_pack_entry(packer, entry, 0)) {
-          ret = kSDWriteFailed;
-        }
+        ret = shada_pack_entry(packer, entry, 0);
         shada_free_shada_entry(&entry);
         break;
       }
@@ -2262,9 +2273,7 @@ static inline ShaDaWriteResult shada_read_when_writing(
       }
       case kSDItemHistoryEntry: {
         if (entry.data.history_item.histtype >= HIST_COUNT) {
-          if (!shada_pack_entry(packer, entry, 0)) {
-            ret = kSDWriteFailed;
-          }
+          ret = shada_pack_entry(packer, entry, 0);
           shada_free_shada_entry(&entry);
           break;
         }
@@ -2275,9 +2284,7 @@ static inline ShaDaWriteResult shada_read_when_writing(
       case kSDItemRegister: {
         const int idx = op_reg_index(entry.data.reg.name);
         if (idx < 0) {
-          if (!shada_pack_entry(packer, entry, 0)) {
-            ret = kSDWriteFailed;
-          }
+          ret = shada_pack_entry(packer, entry, 0);
           shada_free_shada_entry(&entry);
           break;
         }
@@ -2286,9 +2293,7 @@ static inline ShaDaWriteResult shada_read_when_writing(
       }
       case kSDItemVariable: {
         if (!in_strset(&wms->dumped_variables, entry.data.global_var.name)) {
-          if (!shada_pack_entry(packer, entry, 0)) {
-            ret = kSDWriteFailed;
-          }
+          ret = shada_pack_entry(packer, entry, 0);
         }
         shada_free_shada_entry(&entry);
         break;
@@ -2296,9 +2301,7 @@ static inline ShaDaWriteResult shada_read_when_writing(
       case kSDItemGlobalMark: {
         const int idx = mark_global_index(entry.data.filemark.name);
         if (idx < 0) {
-          if (!shada_pack_entry(packer, entry, 0)) {
-            ret = kSDWriteFailed;
-          }
+          ret = shada_pack_entry(packer, entry, 0);
           shada_free_shada_entry(&entry);
           break;
         }
@@ -2465,7 +2468,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
   }
 
   // Write header
-  if (!shada_pack_entry(packer, (ShadaEntry) {
+  if (shada_pack_entry(packer, (ShadaEntry) {
     .type = kSDItemHeader,
     .timestamp = os_time(),
     .data = {
@@ -2486,7 +2489,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
         }),
       }
     }
-  }, 0)) {
+  }, 0) == kSDWriteFailed) {
     ret = kSDWriteFailed;
     goto shada_write_exit;
   }
@@ -2526,7 +2529,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
       };
       i++;
     }
-    if (!shada_pack_entry(packer, buflist_entry, 0)) {
+    if (shada_pack_entry(packer, buflist_entry, 0) == kSDWriteFailed) {
       xfree(buflist_entry.data.buffer_list.buffers);
       ret = kSDWriteFailed;
       goto shada_write_exit;
@@ -2552,7 +2555,8 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
       } else {
         copy_tv(&vartv, &tgttv);
       }
-      if (!shada_pack_entry(packer, (ShadaEntry) {
+      ShaDaWriteResult spe_ret;
+      if ((spe_ret = shada_pack_entry(packer, (ShadaEntry) {
         .type = kSDItemVariable,
         .timestamp = cur_timestamp,
         .data = {
@@ -2562,7 +2566,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
             .additional_elements = NULL,
           }
         }
-      }, max_kbyte)) {
+      }, max_kbyte)) == kSDWriteFailed) {
         clear_tv(&vartv);
         clear_tv(&tgttv);
         ret = kSDWriteFailed;
@@ -2570,8 +2574,10 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
       }
       clear_tv(&vartv);
       clear_tv(&tgttv);
-      int kh_ret;
-      (void) kh_put(strset, &wms->dumped_variables, name, &kh_ret);
+      if (spe_ret == kSDWriteSuccessfull) {
+        int kh_ret;
+        (void) kh_put(strset, &wms->dumped_variables, name, &kh_ret);
+      }
     } while (var_iter != NULL);
   }
 
@@ -2828,9 +2834,9 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
   do { \
     for (size_t i_ = 0; i_ < ARRAY_SIZE(wms_array); i_++) { \
       if (wms_array[i_].data.type != kSDItemMissing) { \
-        if (!shada_pack_encoded_entry(packer, &sd_writer->sd_conv, \
-                                      wms_array[i_], \
-                                      max_kbyte)) { \
+        if (shada_pack_encoded_entry(packer, &sd_writer->sd_conv, \
+                                     wms_array[i_], \
+                                     max_kbyte) == kSDWriteFailed) { \
           ret = kSDWriteFailed; \
           goto shada_write_exit; \
         } \
@@ -2840,8 +2846,8 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
   PACK_WMS_ARRAY(wms->global_marks);
   PACK_WMS_ARRAY(wms->registers);
   for (size_t i = 0; i < wms->jumps_size; i++) {
-    if (!shada_pack_encoded_entry(packer, &sd_writer->sd_conv, wms->jumps[i],
-                                  max_kbyte)) {
+    if (shada_pack_encoded_entry(packer, &sd_writer->sd_conv, wms->jumps[i],
+                                 max_kbyte) == kSDWriteFailed) {
       ret = kSDWriteFailed;
       goto shada_write_exit;
     }
@@ -2849,8 +2855,8 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
 #define PACK_WMS_ENTRY(wms_entry) \
   do { \
     if (wms_entry.data.type != kSDItemMissing) { \
-      if (!shada_pack_encoded_entry(packer, &sd_writer->sd_conv, wms_entry, \
-                                    max_kbyte)) { \
+      if (shada_pack_encoded_entry(packer, &sd_writer->sd_conv, wms_entry, \
+                                   max_kbyte) == kSDWriteFailed) { \
         ret = kSDWriteFailed; \
         goto shada_write_exit; \
       } \
@@ -2877,16 +2883,16 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
   for (size_t i = 0; i < file_markss_to_dump; i++) {
     PACK_WMS_ARRAY(all_file_markss[i]->marks);
     for (size_t j = 0; j < all_file_markss[i]->changes_size; j++) {
-      if (!shada_pack_encoded_entry(packer, &sd_writer->sd_conv,
-                                    all_file_markss[i]->changes[j],
-                                    max_kbyte)) {
+      if (shada_pack_encoded_entry(packer, &sd_writer->sd_conv,
+                                   all_file_markss[i]->changes[j],
+                                   max_kbyte) == kSDWriteFailed) {
         ret = kSDWriteFailed;
         goto shada_write_exit;
       }
     }
     for (size_t j = 0; j < all_file_markss[i]->additional_marks_size; j++) {
-      if (!shada_pack_entry(packer, all_file_markss[i]->additional_marks[j],
-                            0)) {
+      if (shada_pack_entry(packer, all_file_markss[i]->additional_marks[j],
+                           0) == kSDWriteFailed) {
         shada_free_shada_entry(&all_file_markss[i]->additional_marks[j]);
         ret = kSDWriteFailed;
         goto shada_write_exit;
@@ -2903,11 +2909,11 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
       if (dump_one_history[i]) {
         hms_insert_whole_neovim_history(&wms->hms[i]);
         HMS_ITER(&wms->hms[i], cur_entry, {
-          if (!shada_pack_encoded_entry(
+          if (shada_pack_encoded_entry(
               packer, &sd_writer->sd_conv, (PossiblyFreedShadaEntry) {
                 .data = cur_entry->data,
                 .can_free_entry = cur_entry->can_free_entry,
-              }, max_kbyte)) {
+              }, max_kbyte) == kSDWriteFailed) {
             ret = kSDWriteFailed;
             break;
           }
@@ -3043,6 +3049,7 @@ shada_write_file_nomerge: {}
   const ShaDaWriteResult sw_ret = shada_write(&sd_writer, (nomerge
                                                            ? NULL
                                                            : &sd_reader));
+  assert(sw_ret != kSDWriteIgnError);
 #ifndef UNIX
   sd_writer.close(&sd_writer);
 #endif
diff --git a/test/functional/shada/errors_spec.lua b/test/functional/shada/errors_spec.lua
index 62b9e6c84d..f6265bf24b 100644
--- a/test/functional/shada/errors_spec.lua
+++ b/test/functional/shada/errors_spec.lua
@@ -1,6 +1,7 @@
 -- ShaDa errors handling support
 local helpers = require('test.functional.helpers')
-local nvim_command, eq, exc_exec = helpers.command, helpers.eq, helpers.exc_exec
+local nvim_command, eq, exc_exec, redir_exec =
+  helpers.command, helpers.eq, helpers.exc_exec, helpers.redir_exec
 
 local shada_helpers = require('test.functional.shada.helpers')
 local reset, clear, get_shada_rw =
@@ -492,4 +493,21 @@ $
     eq('Vim(wshada):E576: Error while reading ShaDa file: last entry specified that it occupies 47 bytes, but file ended earlier', exc_exec('wshada ' .. shada_fname))
     eq(0, exc_exec('wshada! ' .. shada_fname))
   end)
+
+  it('errors when a funcref is stored in a variable', function()
+    nvim_command('let F = function("tr")')
+    nvim_command('set shada+=!')
+    eq('\nE475: Invalid argument: attempt to dump function reference'
+       .. '\nE574: Failed to write variable F',
+       redir_exec('wshada'))
+  end)
+
+  it('errors when a self-referencing list is stored in a variable', function()
+    nvim_command('let L = []')
+    nvim_command('call add(L, L)')
+    nvim_command('set shada+=!')
+    eq('\nE475: Invalid argument: container references itself'
+       .. '\nE574: Failed to write variable L',
+       redir_exec('wshada'))
+  end)
 end)
diff --git a/test/functional/shada/variables_spec.lua b/test/functional/shada/variables_spec.lua
index 6225971e5f..22054db353 100644
--- a/test/functional/shada/variables_spec.lua
+++ b/test/functional/shada/variables_spec.lua
@@ -1,7 +1,7 @@
 -- ShaDa variables saving/reading support
 local helpers = require('test.functional.helpers')
-local meths, funcs, nvim_command, eq =
-  helpers.meths, helpers.funcs, helpers.command, helpers.eq
+local meths, funcs, nvim_command, eq, exc_exec =
+  helpers.meths, helpers.funcs, helpers.command, helpers.eq, helpers.exc_exec
 
 local shada_helpers = require('test.functional.shada.helpers')
 local reset, set_additional_cmd, clear =
@@ -136,4 +136,31 @@ describe('ShaDa support code', function()
     eq({['\171']={{'\171'}, {['\171']='\171'}, {a='Test'}}},
        meths.get_var('NESTEDVAR'))
   end)
+
+  it('errors and writes when a funcref is stored in a variable',
+  function()
+    nvim_command('let F = function("tr")')
+    meths.set_var('U', '10')
+    nvim_command('set shada+=!')
+    set_additional_cmd('set shada+=!')
+    eq('Vim(wshada):E475: Invalid argument: attempt to dump function reference',
+       exc_exec('wshada'))
+    meths.set_option('shada', '')
+    reset()
+    eq('10', meths.get_var('U'))
+  end)
+
+  it('errors and writes when a self-referencing list is stored in a variable',
+  function()
+    meths.set_var('L', {})
+    nvim_command('call add(L, L)')
+    meths.set_var('U', '10')
+    nvim_command('set shada+=!')
+    eq('Vim(wshada):E475: Invalid argument: container references itself',
+       exc_exec('wshada'))
+    meths.set_option('shada', '')
+    set_additional_cmd('set shada+=!')
+    reset()
+    eq('10', meths.get_var('U'))
+  end)
 end)