fix(api): handle win_split_ins failure properly

Problem: nvim_win_set_config does not handle failure in win_split_ins properly
yet, which can cause all sorts of issues. Also nvim_open_win and
nvim_win_set_config do not set the error message to the one from win_split_ins.

Solution: handle failure by undoing winframe_remove, like in win_splitmove.
Make sure autocommands from switching to the altwin fire within a valid window,
and ensure they don't screw things up. Set the error message to that of
win_split_ins, if any.

Also change a few other small things, including:

- adjust win_append to take a tabpage_T * argument, which is more consistent
  with win_remove (and also allows us to undo a call to win_remove).

- allow winframe_restore to restore window positions. Useful if `wp` was in a
  different tabpage, as a call to win_comp_pos (which only works for the current
  tabpage) after winframe_restore should no longer be needed.

  Though enter_tabpage calls win_comp_pos anyway, this has the advantage of
  ensuring w_winrow/col remains accurate even before entering the tabpage
  (useful for stuff like win_screenpos, if used on a window in another tabpage).

  (This change should probably also be PR'd to Vim later, even though it doesn't
  use winframe_restore for a `wp` in a different tabpage yet).

4 files changed, 184 insertions, 95 deletions

diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c
index 978d8515c8..bb28000719 100644
--- a/src/nvim/api/win_config.c
+++ b/src/nvim/api/win_config.c
@@ -259,18 +259,20 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err
     }
     int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER;
 
-    if (parent == NULL) {
-      wp = win_split_ins(0, flags, NULL, 0, NULL);
-    } else {
-      tp = win_find_tabpage(parent);
-      switchwin_T switchwin;
-      // `parent` is valid in `tp`, so switch_win should not fail.
-      const int result = switch_win(&switchwin, parent, tp, true);
-      assert(result == OK);
-      (void)result;
-      wp = win_split_ins(0, flags, NULL, 0, NULL);
-      restore_win(&switchwin, true);
-    }
+    TRY_WRAP(err, {
+      if (parent == NULL || parent == curwin) {
+        wp = win_split_ins(0, flags, NULL, 0, NULL);
+      } else {
+        tp = win_find_tabpage(parent);
+        switchwin_T switchwin;
+        // `parent` is valid in `tp`, so switch_win should not fail.
+        const int result = switch_win(&switchwin, parent, tp, true);
+        assert(result == OK);
+        (void)result;
+        wp = win_split_ins(0, flags, NULL, 0, NULL);
+        restore_win(&switchwin, true);
+      }
+    });
     if (wp) {
       wp->w_config = fconfig;
     }
@@ -278,7 +280,9 @@ Window nvim_open_win(Buffer buffer, Boolean enter, Dict(win_config) *config, Err
     wp = win_new_float(NULL, false, fconfig, err);
   }
   if (!wp) {
-    api_set_error(err, kErrorTypeException, "Failed to create window");
+    if (!ERROR_SET(err)) {
+      api_set_error(err, kErrorTypeException, "Failed to create window");
+    }
     return 0;
   }
 
@@ -448,17 +452,47 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err)
       return;  // error already set
     }
 
-    if (was_split) {
-      win_T *new_curwin = NULL;
+    bool to_split_ok = false;
+    // If we are moving curwin to another tabpage, switch windows *before* we remove it from the
+    // window list or remove its frame (if non-floating), so it's valid for autocommands.
+    const bool curwin_moving_tp
+      = win == curwin && parent != NULL && win_tp != win_find_tabpage(parent);
+    if (curwin_moving_tp) {
+      if (was_split) {
+        int dir;
+        win_goto(winframe_find_altwin(win, &dir, NULL, NULL));
+      } else {
+        win_goto(win_valid(prevwin) && prevwin != win ? prevwin : firstwin);
+      }
 
+      // Autocommands may have been a real nuisance and messed things up...
+      if (curwin == win) {
+        api_set_error(err, kErrorTypeException, "Failed to switch away from window %d",
+                      win->handle);
+        return;
+      }
+      win_tp = win_find_tabpage(win);
+      if (!win_tp || !win_valid_any_tab(parent)) {
+        api_set_error(err, kErrorTypeException, "Windows to split were closed");
+        goto restore_curwin;
+      }
+      if (was_split == win->w_floating || parent->w_floating) {
+        api_set_error(err, kErrorTypeException, "Floating state of windows to split changed");
+        goto restore_curwin;
+      }
+    }
+
+    int dir = 0;
+    frame_T *unflat_altfr = NULL;
+    if (was_split) {
       // If the window is the last in the tabpage or `fconfig.win` is
       // a handle to itself, we can't split it.
       if (win->w_frame->fr_parent == NULL) {
         // FIXME(willothy): if the window is the last in the tabpage but there is another tabpage
         // and the target window is in that other tabpage, should we move the window to that
         // tabpage and close the previous one, or just error?
-        api_set_error(err, kErrorTypeValidation, "Cannot move last window");
-        return;
+        api_set_error(err, kErrorTypeException, "Cannot move last window");
+        goto restore_curwin;
       } else if (parent != NULL && parent->handle == win->handle) {
         int n_frames = 0;
         for (frame_T *fr = win->w_frame->fr_parent->fr_child; fr != NULL; fr = fr->fr_next) {
@@ -494,64 +528,67 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err)
           }
           // If the frame doesn't have a parent, the old frame
           // was the root frame and we need to create a top-level split.
-          int dir;
-          new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL);
+          winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr);
         } else if (n_frames == 2) {
           // There are two windows in the frame, we can just rotate it.
-          int dir;
-          neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL);
-          new_curwin = neighbor;
+          neighbor = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr);
         } else {
           // There is only one window in the frame, we can't split it.
-          api_set_error(err, kErrorTypeValidation, "Cannot split window into itself");
-          return;
+          api_set_error(err, kErrorTypeException, "Cannot split window into itself");
+          goto restore_curwin;
         }
-        // Set the parent to whatever the correct
-        // neighbor window was determined to be.
+        // Set the parent to whatever the correct neighbor window was determined to be.
         parent = neighbor;
       } else {
-        int dir;
-        new_curwin = winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, NULL);
-      }
-      win_remove(win, win_tp == curtab ? NULL : win_tp);
-      // move to neighboring window if we're moving the current window to a new tabpage
-      if (curwin == win && parent != NULL && new_curwin != NULL
-          && win_tp != win_find_tabpage(parent)) {
-        win_enter(new_curwin, true);
-        if (!win_valid_any_tab(parent)) {
-          // win_enter autocommands closed the `parent` to split from.
-          api_set_error(err, kErrorTypeException, "Window to split was closed");
-          return;
-        }
+        winframe_remove(win, &dir, win_tp == curtab ? NULL : win_tp, &unflat_altfr);
       }
-    } else {
-      win_remove(win, win_tp == curtab ? NULL : win_tp);
     }
+    win_remove(win, win_tp == curtab ? NULL : win_tp);
 
     int flags = win_split_flags(fconfig.split, parent == NULL) | WSP_NOENTER;
+    TRY_WRAP(err, {
+      const bool need_switch = parent != NULL && parent != curwin;
+      switchwin_T switchwin;
+      if (need_switch) {
+        // `parent` is valid in its tabpage, so switch_win should not fail.
+        const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true);
+        (void)result;
+        assert(result == OK);
+      }
+      to_split_ok = win_split_ins(0, flags, win, 0, unflat_altfr) != NULL;
+      if (!to_split_ok) {
+        // Restore `win` to the window list now, so it's valid for restore_win (if used).
+        win_append(win->w_prev, win, win_tp == curtab ? NULL : win_tp);
+      }
+      if (need_switch) {
+        restore_win(&switchwin, true);
+      }
+    });
+    if (!to_split_ok) {
+      if (was_split) {
+        // win_split_ins doesn't change sizes or layout if it fails to insert an existing window, so
+        // just undo winframe_remove.
+        winframe_restore(win, dir, unflat_altfr);
+      }
+      if (!ERROR_SET(err)) {
+        api_set_error(err, kErrorTypeException, "Failed to move window %d into split", win->handle);
+      }
 
-    if (parent == NULL) {
-      if (!win_split_ins(0, flags, win, 0, NULL)) {
-        // TODO(willothy): What should this error message say?
-        api_set_error(err, kErrorTypeException, "Failed to split window");
-        return;
+restore_curwin:
+      // If `win` was the original curwin, and autocommands didn't move it outside of curtab, be a
+      // good citizen and try to return to it.
+      if (curwin_moving_tp && win_valid(win)) {
+        win_goto(win);
       }
-    } else {
-      switchwin_T switchwin;
-      // `parent` is valid in its tabpage, so switch_win should not fail.
-      const int result = switch_win(&switchwin, parent, win_find_tabpage(parent), true);
-      (void)result;
-      assert(result == OK);
-      win_split_ins(0, flags, win, 0, NULL);
-      restore_win(&switchwin, true);
+      return;
     }
+
     if (HAS_KEY_X(config, width)) {
       win_setwidth_win(fconfig.width, win);
     }
     if (HAS_KEY_X(config, height)) {
       win_setheight_win(fconfig.height, win);
     }
-    return;
   } else {
     win_config_float(win, fconfig);
     win->w_pos_changed = true;
diff --git a/src/nvim/autocmd.c b/src/nvim/autocmd.c
index 3f93906942..652b6ba74e 100644
--- a/src/nvim/autocmd.c
+++ b/src/nvim/autocmd.c
@@ -1333,7 +1333,7 @@ void aucmd_prepbuf(aco_save_T *aco, buf_T *buf)
 
     block_autocmds();  // We don't want BufEnter/WinEnter autocommands.
     if (need_append) {
-      win_append(lastwin, auc_win);
+      win_append(lastwin, auc_win, NULL);
       pmap_put(int)(&window_handles, auc_win->handle, auc_win);
       win_config_float(auc_win, auc_win->w_config);
     }
diff --git a/src/nvim/window.c b/src/nvim/window.c
index 80cb9ab6a0..b55dd79260 100644
--- a/src/nvim/window.c
+++ b/src/nvim/window.c
@@ -1218,13 +1218,13 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir, frame_T *to_fl
     if (new_wp == NULL) {
       wp = win_alloc(oldwin, false);
     } else {
-      win_append(oldwin, wp);
+      win_append(oldwin, wp, NULL);
     }
   } else {
     if (new_wp == NULL) {
       wp = win_alloc(oldwin->w_prev, false);
     } else {
-      win_append(oldwin->w_prev, wp);
+      win_append(oldwin->w_prev, wp, NULL);
     }
   }
 
@@ -1783,13 +1783,13 @@ static void win_exchange(int Prenum)
   if (wp->w_prev != curwin) {
     win_remove(curwin, NULL);
     frame_remove(curwin->w_frame);
-    win_append(wp->w_prev, curwin);
+    win_append(wp->w_prev, curwin, NULL);
     frame_insert(frp, curwin->w_frame);
   }
   if (wp != wp2) {
     win_remove(wp, NULL);
     frame_remove(wp->w_frame);
-    win_append(wp2, wp);
+    win_append(wp2, wp, NULL);
     if (frp2 == NULL) {
       frame_insert(wp->w_frame->fr_parent->fr_child, wp->w_frame);
     } else {
@@ -1863,7 +1863,7 @@ static void win_rotate(bool upwards, int count)
 
       // find last frame and append removed window/frame after it
       for (; frp->fr_next != NULL; frp = frp->fr_next) {}
-      win_append(frp->fr_win, wp1);
+      win_append(frp->fr_win, wp1, NULL);
       frame_append(frp, wp1->w_frame);
 
       wp2 = frp->fr_win;                // previously last window
@@ -1878,7 +1878,7 @@ static void win_rotate(bool upwards, int count)
       assert(frp->fr_parent->fr_child);
 
       // append the removed window/frame before the first in the list
-      win_append(frp->fr_parent->fr_child->fr_win->w_prev, wp1);
+      win_append(frp->fr_parent->fr_child->fr_win->w_prev, wp1, NULL);
       frame_insert(frp->fr_parent->fr_child, frp);
     }
 
@@ -1937,7 +1937,7 @@ int win_splitmove(win_T *wp, int size, int flags)
 
   // Split a window on the desired side and put "wp" there.
   if (win_split_ins(size, flags, wp, dir, unflat_altfr) == NULL) {
-    win_append(wp->w_prev, wp);
+    win_append(wp->w_prev, wp, NULL);
     if (!wp->w_floating) {
       // win_split_ins doesn't change sizes or layout if it fails to insert an
       // existing window, so just undo winframe_remove.
@@ -2016,7 +2016,7 @@ void win_move_after(win_T *win1, win_T *win2)
     }
     win_remove(win1, NULL);
     frame_remove(win1->w_frame);
-    win_append(win2, win1);
+    win_append(win2, win1, NULL);
     frame_append(win2->w_frame, win1->w_frame);
 
     win_comp_pos();  // recompute w_winrow for all windows
@@ -3152,6 +3152,55 @@ void win_free_all(void)
 win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_altfr)
   FUNC_ATTR_NONNULL_ARG(1, 2)
 {
+  frame_T *altfr;
+  win_T *wp = winframe_find_altwin(win, dirp, tp, &altfr);
+  if (wp == NULL) {
+    return NULL;
+  }
+
+  frame_T *frp_close = win->w_frame;
+  // Remove this frame from the list of frames.
+  frame_remove(frp_close);
+
+  if (*dirp == 'v') {
+    frame_new_height(altfr, altfr->fr_height + frp_close->fr_height,
+                     altfr == frp_close->fr_next, false);
+  } else {
+    assert(*dirp == 'h');
+    frame_new_width(altfr, altfr->fr_width + frp_close->fr_width,
+                    altfr == frp_close->fr_next, false);
+  }
+
+  // If rows/columns go to a window below/right its positions need to be
+  // updated.  Can only be done after the sizes have been updated.
+  if (altfr == frp_close->fr_next) {
+    int row = win->w_winrow;
+    int col = win->w_wincol;
+
+    frame_comp_pos(altfr, &row, &col);
+  }
+
+  if (unflat_altfr == NULL) {
+    frame_flatten(altfr);
+  } else {
+    *unflat_altfr = altfr;
+  }
+
+  return wp;
+}
+
+/// Find the window that will get the freed space from a call to `winframe_remove`.
+/// Makes no changes to the window layout.
+///
+/// @param dirp  set to 'v' or 'h' for the direction where "altfr" will be resized
+///              to fill the space
+/// @param tp    tab page "win" is in, NULL for current
+/// @param altfr if not NULL, set to pointer of frame that will get the space
+///
+/// @return      a pointer to the window that will get the freed up space.
+win_T *winframe_find_altwin(win_T *win, int *dirp, tabpage_T *tp, frame_T **altfr)
+  FUNC_ATTR_NONNULL_ARG(1, 2)
+{
   assert(tp == NULL || tp != curtab);
 
   // If there is only one window there is nothing to remove.
@@ -3161,13 +3210,10 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al
 
   frame_T *frp_close = win->w_frame;
 
-  // Remove the window from its frame.
+  // Find the window and frame that gets the space.
   frame_T *frp2 = win_altframe(win, tp);
   win_T *wp = frame2win(frp2);
 
-  // Remove this frame from the list of frames.
-  frame_remove(frp_close);
-
   if (frp_close->fr_parent->fr_layout == FR_COL) {
     // When 'winfixheight' is set, try to find another frame in the column
     // (as close to the closed frame as possible) to distribute the height
@@ -3194,8 +3240,6 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al
         }
       }
     }
-    frame_new_height(frp2, frp2->fr_height + frp_close->fr_height,
-                     frp2 == frp_close->fr_next, false);
     *dirp = 'v';
   } else {
     // When 'winfixwidth' is set, try to find another frame in the column
@@ -3223,24 +3267,12 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al
         }
       }
     }
-    frame_new_width(frp2, frp2->fr_width + frp_close->fr_width,
-                    frp2 == frp_close->fr_next, false);
     *dirp = 'h';
   }
 
-  // If rows/columns go to a window below/right its positions need to be
-  // updated.  Can only be done after the sizes have been updated.
-  if (frp2 == frp_close->fr_next) {
-    int row = win->w_winrow;
-    int col = win->w_wincol;
-
-    frame_comp_pos(frp2, &row, &col);
-  }
-
-  if (unflat_altfr == NULL) {
-    frame_flatten(frp2);
-  } else {
-    *unflat_altfr = frp2;
+  assert(wp != win && frp2 != frp_close);
+  if (altfr != NULL) {
+    *altfr = frp2;
   }
 
   return wp;
@@ -3305,9 +3337,10 @@ static void frame_flatten(frame_T *frp)
 }
 
 /// Undo changes from a prior call to winframe_remove, also restoring lost
-/// vertical separators and statuslines.
+/// vertical separators and statuslines, and changed window positions for
+/// windows within "unflat_altfr".
 /// Caller must ensure no other changes were made to the layout or window sizes!
-static void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr)
+void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr)
   FUNC_ATTR_NONNULL_ALL
 {
   frame_T *frp = wp->w_frame;
@@ -3333,13 +3366,24 @@ static void winframe_restore(win_T *wp, int dir, frame_T *unflat_altfr)
     }
   }
 
+  int row = wp->w_winrow;
+  int col = wp->w_wincol;
+
   // Restore the lost room that was redistributed to the altframe.
   if (dir == 'v') {
     frame_new_height(unflat_altfr, unflat_altfr->fr_height - frp->fr_height,
                      unflat_altfr == frp->fr_next, false);
+    row += frp->fr_height;
   } else if (dir == 'h') {
     frame_new_width(unflat_altfr, unflat_altfr->fr_width - frp->fr_width,
                     unflat_altfr == frp->fr_next, false);
+    col += frp->fr_width;
+  }
+
+  // If rows/columns went to a window below/right, its positions need to be
+  // restored.  Can only be done after the sizes have been updated.
+  if (unflat_altfr == frp->fr_next) {
+    frame_comp_pos(unflat_altfr, &row, &col);
   }
 }
 
@@ -4445,7 +4489,7 @@ static void tabpage_check_windows(tabpage_T *old_curtab)
     if (wp->w_floating) {
       if (wp->w_config.external) {
         win_remove(wp, old_curtab);
-        win_append(lastwin_nofloating(), wp);
+        win_append(lastwin_nofloating(), wp, NULL);
       } else {
         ui_comp_remove_grid(&wp->w_grid_alloc);
       }
@@ -5085,7 +5129,7 @@ win_T *win_alloc(win_T *after, bool hidden)
   block_autocmds();
   // link the window in the window list
   if (!hidden) {
-    win_append(after, new_wp);
+    win_append(after, new_wp, NULL);
   }
 
   new_wp->w_wincol = 0;
@@ -5255,21 +5299,29 @@ void win_free_grid(win_T *wp, bool reinit)
   }
 }
 
-// Append window "wp" in the window list after window "after".
-void win_append(win_T *after, win_T *wp)
+/// Append window "wp" in the window list after window "after".
+///
+/// @param tp  tab page "win" (and "after", if not NULL) is in, NULL for current
+void win_append(win_T *after, win_T *wp, tabpage_T *tp)
+  FUNC_ATTR_NONNULL_ARG(2)
 {
+  assert(tp == NULL || tp != curtab);
+
+  win_T **first = tp == NULL ? &firstwin : &tp->tp_firstwin;
+  win_T **last = tp == NULL ? &lastwin : &tp->tp_lastwin;
+
   // after NULL is in front of the first
-  win_T *before = after == NULL ? firstwin : after->w_next;
+  win_T *before = after == NULL ? *first : after->w_next;
 
   wp->w_next = before;
   wp->w_prev = after;
   if (after == NULL) {
-    firstwin = wp;
+    *first = wp;
   } else {
     after->w_next = wp;
   }
   if (before == NULL) {
-    lastwin = wp;
+    *last = wp;
   } else {
     before->w_prev = wp;
   }
diff --git a/src/nvim/winfloat.c b/src/nvim/winfloat.c
index cddc4ed9dd..10d8c7ac90 100644
--- a/src/nvim/winfloat.c
+++ b/src/nvim/winfloat.c
@@ -61,7 +61,7 @@ win_T *win_new_float(win_T *wp, bool last, WinConfig fconfig, Error *err)
     XFREE_CLEAR(wp->w_frame);
     win_comp_pos();  // recompute window positions
     win_remove(wp, NULL);
-    win_append(lastwin_nofloating(), wp);
+    win_append(lastwin_nofloating(), wp, NULL);
   }
   wp->w_floating = true;
   wp->w_status_height = 0;