vim-patch:8.2.0602: :unlet $VAR does not work properly (#13238)

Problem:    :unlet $VAR does not work properly.
Solution:   Make ":lockvar $VAR" fail.  Check the "skip" flag.
7e0868efcf

Include patch 8.2.0601 changes so that ex_unletlock() can execute a callback if there are no errors.

src/nvim/eval.c

@@ -64,6 +64,7 @@ static char *e_dictrange = N_("E719: Cannot use [:] with a Dictionary");
 static char *e_illvar = N_("E461: Illegal variable name: %s");
 static char *e_cannot_mod = N_("E995: Cannot modify existing variable");
 static char *e_invalwindow = N_("E957: Invalid window number");
+static char *e_lock_unlock = N_("E940: Cannot lock or unlock variable %s");
 
 // TODO(ZyX-I): move to eval/executor
 static char *e_letwrong = N_("E734: Wrong variable type for %s=");
@@ -2638,22 +2639,18 @@ void set_context_for_expression(expand_T *xp, char_u *arg, cmdidx_T cmdidx)
   xp->xp_pattern = arg;
 }
 
-/*
+/// ":unlet[!] var1 ... " command.
- * ":unlet[!] var1 ... " command.
- */
 void ex_unlet(exarg_T *eap)
 {
-  ex_unletlock(eap, eap->arg, 0);
+  ex_unletlock(eap, eap->arg, 0, do_unlet_var);
 }
 
 // TODO(ZyX-I): move to eval/ex_cmds
 
-/*
+/// ":lockvar" and ":unlockvar" commands
- * ":lockvar" and ":unlockvar" commands
- */
 void ex_lockvar(exarg_T *eap)
 {
-  char_u      *arg = eap->arg;
+  char_u *arg = eap->arg;
   int deep = 2;
 
   if (eap->forceit) {
@@ -2663,30 +2660,41 @@ void ex_lockvar(exarg_T *eap)
     arg = skipwhite(arg);
   }
 
-  ex_unletlock(eap, arg, deep);
+  ex_unletlock(eap, arg, deep, do_lock_var);
 }
 
 // TODO(ZyX-I): move to eval/ex_cmds
 
-/*
+/// Common parsing logic for :unlet, :lockvar and :unlockvar.
- * ":unlet", ":lockvar" and ":unlockvar" are quite similar.
+///
- */
+/// Invokes `callback` afterwards if successful and `eap->skip == false`.
-static void ex_unletlock(exarg_T *eap, char_u *argstart, int deep)
+///
+/// @param[in]  eap  Ex command arguments for the command.
+/// @param[in]  argstart  Start of the string argument for the command.
+/// @param[in]  deep  Levels to (un)lock for :(un)lockvar, -1 to (un)lock
+///                   everything.
+/// @param[in]  callback  Appropriate handler for the command.
+static void ex_unletlock(exarg_T *eap, char_u *argstart, int deep,
+                         ex_unletlock_callback callback)
+  FUNC_ATTR_NONNULL_ALL
 {
-  char_u      *arg = argstart;
+  char_u *arg = argstart;
   char_u *name_end;
   bool error = false;
   lval_T lv;
 
   do {
     if (*arg == '$') {
-      const char *name = (char *)++arg;
+      lv.ll_name = (const char *)arg;
-
+      lv.ll_tv = NULL;
+      arg++;
       if (get_env_len((const char_u **)&arg) == 0) {
-        EMSG2(_(e_invarg2), name - 1);
+        EMSG2(_(e_invarg2), arg - 1);
         return;
       }
-      os_unsetenv(name);
+      if (!error && !eap->skip && callback(&lv, arg, eap, deep) == FAIL) {
+        error = true;
+      }
       name_end = arg;
     } else {
       // Parse the name and find the end.
@@ -2707,17 +2715,8 @@ static void ex_unletlock(exarg_T *eap, char_u *argstart, int deep)
         break;
       }
 
-      if (!error && !eap->skip) {
+      if (!error && !eap->skip && callback(&lv, name_end, eap, deep) == FAIL) {
-        if (eap->cmdidx == CMD_unlet) {
+        error = true;
-          if (do_unlet_var(&lv, name_end, eap->forceit) == FAIL) {
-            error = true;
-          }
-        } else {
-          if (do_lock_var(&lv, name_end, deep,
-                          eap->cmdidx == CMD_lockvar) == FAIL) {
-            error = true;
-          }
-        }
       }
 
       if (!eap->skip) {
@@ -2732,8 +2731,19 @@ static void ex_unletlock(exarg_T *eap, char_u *argstart, int deep)
 
 // TODO(ZyX-I): move to eval/ex_cmds
 
-static int do_unlet_var(lval_T *const lp, char_u *const name_end, int forceit)
+/// Unlet a variable indicated by `lp`.
+///
+/// @param[in]  lp  The lvalue.
+/// @param[in]  name_end  End of the string argument for the command.
+/// @param[in]  eap  Ex command arguments for :unlet.
+/// @param[in]  deep  Unused.
+///
+/// @return OK on success, or FAIL on failure.
+static int do_unlet_var(lval_T *lp, char_u *name_end, exarg_T *eap,
+                        int deep FUNC_ATTR_UNUSED)
+  FUNC_ATTR_NONNULL_ALL
 {
+  int forceit = eap->forceit;
   int ret = OK;
   int cc;
 
@@ -2741,8 +2751,10 @@ static int do_unlet_var(lval_T *const lp, char_u *const name_end, int forceit)
     cc = *name_end;
     *name_end = NUL;
 
-    // Normal name or expanded name.
+    // Environment variable, normal name or expanded name.
-    if (do_unlet(lp->ll_name, lp->ll_name_len, forceit) == FAIL) {
+    if (*lp->ll_name == '$') {
+      os_unsetenv(lp->ll_name + 1);
+    } else if (do_unlet(lp->ll_name, lp->ll_name_len, forceit) == FAIL) {
       ret = FAIL;
     }
     *name_end = cc;
@@ -2816,7 +2828,7 @@ static int do_unlet_var(lval_T *const lp, char_u *const name_end, int forceit)
 ///
 /// @param[in]  name  Variable name to unlet.
 /// @param[in]  name_len  Variable name length.
-/// @param[in]  fonceit  If true, do not complain if variable doesn’t exist.
+/// @param[in]  forceit  If true, do not complain if variable doesn’t exist.
 ///
 /// @return OK if it existed, FAIL otherwise.
 int do_unlet(const char *const name, const size_t name_len, const bool forceit)
@@ -2883,14 +2895,21 @@ int do_unlet(const char *const name, const size_t name_len, const bool forceit)
 
 // TODO(ZyX-I): move to eval/ex_cmds
 
-/*
+/// Lock or unlock variable indicated by `lp`.
- * Lock or unlock variable indicated by "lp".
+///
- * "deep" is the levels to go (-1 for unlimited);
+/// Locks if `eap->cmdidx == CMD_lockvar`, unlocks otherwise.
- * "lock" is TRUE for ":lockvar", FALSE for ":unlockvar".
+///
- */
+/// @param[in]  lp  The lvalue.
-static int do_lock_var(lval_T *lp, char_u *const name_end, const int deep,
+/// @param[in]  name_end  Unused.
-                       const bool lock)
+/// @param[in]  eap  Ex command arguments for :(un)lockvar.
+/// @param[in]  deep  Levels to (un)lock, -1 to (un)lock everything.
+///
+/// @return OK on success, or FAIL on failure.
+static int do_lock_var(lval_T *lp, char_u *name_end FUNC_ATTR_UNUSED,
+                       exarg_T *eap, int deep)
+  FUNC_ATTR_NONNULL_ARG(1, 3)
 {
+  bool lock = eap->cmdidx == CMD_lockvar;
   int ret = OK;
 
   if (deep == 0) {  // Nothing to do.
@@ -2898,25 +2917,31 @@ static int do_lock_var(lval_T *lp, char_u *const name_end, const int deep,
   }
 
   if (lp->ll_tv == NULL) {
-    // Normal name or expanded name.
+    if (*lp->ll_name == '$') {
-    dictitem_T *const di = find_var(
+      EMSG2(_(e_lock_unlock), lp->ll_name);
-        (const char *)lp->ll_name, lp->ll_name_len, NULL,
-        true);
-    if (di == NULL) {
       ret = FAIL;
-    } else if ((di->di_flags & DI_FLAGS_FIX)
-               && di->di_tv.v_type != VAR_DICT
-               && di->di_tv.v_type != VAR_LIST) {
-      // For historical reasons this error is not given for Lists and
-      // Dictionaries. E.g. b: dictionary may be locked/unlocked.
-      emsgf(_("E940: Cannot lock or unlock variable %s"), lp->ll_name);
     } else {
-      if (lock) {
+      // Normal name or expanded name.
-        di->di_flags |= DI_FLAGS_LOCK;
+      dictitem_T *const di = find_var(
+          (const char *)lp->ll_name, lp->ll_name_len, NULL,
+          true);
+      if (di == NULL) {
+        ret = FAIL;
+      } else if ((di->di_flags & DI_FLAGS_FIX)
+                 && di->di_tv.v_type != VAR_DICT
+                 && di->di_tv.v_type != VAR_LIST) {
+        // For historical reasons this error is not given for Lists and
+        // Dictionaries. E.g. b: dictionary may be locked/unlocked.
+        EMSG2(_(e_lock_unlock), lp->ll_name);
+        ret = FAIL;
       } else {
-        di->di_flags &= ~DI_FLAGS_LOCK;
+        if (lock) {
+          di->di_flags |= DI_FLAGS_LOCK;
+        } else {
+          di->di_flags &= ~DI_FLAGS_LOCK;
+        }
+        tv_item_lock(&di->di_tv, deep, lock);
       }
-      tv_item_lock(&di->di_tv, deep, lock);
     }
   } else if (lp->ll_range) {
     listitem_T    *li = lp->ll_li;

src/nvim/eval.h

@@ -233,6 +233,8 @@ typedef enum {
   kDictListItems,  ///< List dictionary contents: [keys, values].
 } DictListType;
 
+typedef int (*ex_unletlock_callback)(lval_T *, char_u *, exarg_T *, int);
+
 // Used for checking if local variables or arguments used in a lambda.
 extern bool *eval_lavars_used;
 

src/nvim/testdir/test_vimscript.vim

@@ -1393,6 +1393,20 @@ func Test_compound_assignment_operators()
     let @/ = ''
 endfunc
 
+func Test_unlet_env()
+    let $TESTVAR = 'yes'
+    call assert_equal('yes', $TESTVAR)
+    call assert_fails('lockvar $TESTVAR', 'E940')
+    call assert_fails('unlockvar $TESTVAR', 'E940')
+    call assert_equal('yes', $TESTVAR)
+    if 0
+        unlet $TESTVAR
+    endif
+    call assert_equal('yes', $TESTVAR)
+    unlet $TESTVAR
+    call assert_equal('', $TESTVAR)
+endfunc
+
 func Test_funccall_garbage_collect()
     func Func(x, ...)
         call add(a:x, a:000)