Fix bugs in testing if a parameter is set

This fixes three related bugs:
1. Expansions like ${var+set} remained static when used within a
   'for', 'while' or 'until' loop; the expansions din't change
   along with the state of the variable, so they could not be used
   to check whether a variable is set within a loop if the state of
   that variable changed in the course of the loop. (BUG_ISSETLOOP)
2. ${IFS+s} always yielded 's', and [[ -v IFS ]] always yielded
   true, even if IFS is unset. (BUG_IFSISSET)
3. IFS was incorrectly exempt from '-u' ('-o nounset') checks.

src/cmd/ksh93/sh/macro.c: varsub():
- When getting a node pointer (np) to the parameter to test,
  special-case IFS by checking if it has a value and not setting
  the pointer if not. The node to IFS always exists, even after
  'unset -v IFS', so before this fix it always followed the code
  path for a parameter that is set. This fixes BUG_IFSISSET for
  ${IFS+s} and also fixes set -u (-o nounset) with IFS.
- Before using the 'nv_isnull' macro to check if a regular variable
  is set, call nv_optimize() if needed. This fixes BUG_ISSETLOOP.
  Idea from Kurtis Rader: https://github.com/att/ast/issues/1090
  Of course this only works if SHOPT_OPTIMIZE==1 (the default),
  but if not, then this bug is not triggered in the first place.
- Add some comments for future reference.

src/cmd/ksh93/bltins/test.c: test_unop():
- Fix BUG_IFSISSET for [[ -v IFS ]]. The nv_optimize() method
  doesn't seem to have any effect here, so the only way that I can
  figure out is to special-case IFS, nv_getval()'ing it to check if
  IFS has a value in the current scope.

src/cmd/ksh93/tests/variables.sh:
- Add regression tests for checking if a varariable is set within a
  loop, within and outside a function with that variable made local
  (to check if the scope is honoured). Repeat these tests for a
  regular variable and for IFS, for ${foo+set} and [[ -v foo ]].

(cherry picked from commit a2cf79cb98fa3e47eca85d9049d1d831636c9b16)

NEWS

@@ -4,6 +4,17 @@ For full details, see the git log at:
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2020-05-20:
+
+- Fix BUG_ISSETLOOP. Expansions like ${var+set} remained static when used
+  within a 'for', 'while' or 'until' loop; the expansions din't change along
+  with the state of the variable, so they could not be used to check whether a
+  variable is set within a loop if the state of that variable changed in the
+  course of the loop.
+
+- Fix BUG_IFSISSET. ${IFS+s} always yielded 's', and [[ -v IFS ]] always
+  yielded true, even if IFS is unset. This applied to IFS only.
+
 2020-05-19:
 
 - Fix 'command -p'. The -p option causes the operating system's standard

TODO

@@ -65,15 +65,6 @@ https://github.com/modernish/modernish/tree/0.16/lib/modernish/cap/
   are erroneously interpreted as wildcards when quoted "$*" is used as the
   glob pattern.
 
-- BUG_IFSISSET: ${IFS+s} always yields 's' even if IFS is unset. This applies
-  to IFS only.
-
-- BUG_ISSETLOOP: Expansions like ${var+set} remain static when used within a
-  'for', 'while' or 'until' loop; the expansions don't change along with the
-  state of the variable, so they cannot be used to check whether a variable
-  is set within a loop if the state of that variable may change in the
-  course of the loop.
-
 - BUG_KUNSETIFS: ksh93: Can't unset IFS under very specific circumstances.
   unset -v IFS is a known POSIX shell idiom to activate default field
   splitting. With this bug, the unset builtin silently fails to unset IFS

src/cmd/ksh93/bltins/test.c

@@ -451,6 +451,8 @@ int test_unop(Shell_t *shp,register int op,register const char *arg)
 		}
 		if(ap = nv_arrayptr(np))
 			return(nv_arrayisset(np,ap));
+		if(*arg=='I' && strcmp(arg,"IFS")==0)
+			return(nv_getval(np)!=NULL);  /* avoid BUG_IFSISSET */
 		return(!nv_isnull(np) || nv_isattr(np,NV_INTEGER));
 	    }
 	    default:

src/cmd/ksh93/include/version.h

@@ -17,4 +17,4 @@
 *                  David Korn <dgk@research.att.com>                   *
 *                                                                      *
 ***********************************************************************/
-#define SH_RELEASE	"93u+m 2020-05-19"
+#define SH_RELEASE	"93u+m 2020-05-20"

src/cmd/ksh93/sh/macro.c

@@ -1306,7 +1306,15 @@ retry1:
 		{
 			if(mp->shp->argaddr)
 				flag &= ~NV_NOADD;
-			np = nv_open(id,mp->shp->var_tree,flag|NV_NOFAIL);
+			/*
+			 * Get a node pointer (np) to the parameter, if any.
+			 *
+			 * The IFS node always exists, so we must special-case IFS by checking if it has
+			 * a value; otherwise it always follows the code path for a set parameter, so is
+			 * not subject to 'set -u', and may test as set even after 'unset -v IFS'.
+			 */
+			if(!(*id=='I' && strcmp(id,"IFS")==0 && nv_getval(sh_scoped(mp->shp,IFSNOD)) == NULL))
+				np = nv_open(id,mp->shp->var_tree,flag|NV_NOFAIL);
 			if(!np)
 			{
 				sfprintf(mp->shp->strbuf,"%s%c",id,0);
@@ -1393,8 +1401,12 @@ retry1:
 		if((type==M_VNAME||type==M_SUBNAME)  && mp->shp->argaddr && strcmp(nv_name(np),id))
 			mp->shp->argaddr = 0;
 		c = (type>M_BRACE && isastchar(mode));
+		/*
+		 * Check if the parameter is set or unset.
+		 */
 		if(np && (type==M_TREE || !c || !ap))
 		{
+			/* The parameter is set. */
 			char *savptr;
 			c = *((unsigned char*)stkptr(stkp,offset-1));
 			savptr = stkfreeze(stkp,0);
@@ -1430,7 +1442,13 @@ retry1:
 					if(ap)
 						v = nv_arrayisset(np,ap)?(char*)"x":0;
 					else
+					{
+#if SHOPT_OPTIMIZE
+						if(mp->shp->argaddr)
+							nv_optimize(np);  /* avoid BUG_ISSETLOOP */
+#endif /* SHOPT_OPTIMIZE */
 						v = nv_isnull(np)?0:(char*)"x";
+					}
 				}
 				else
 					v = nv_getval(np);
@@ -1446,6 +1464,7 @@ retry1:
 		}
 		else
 		{
+			/* The parameter is unset. */
 			if(sh_isoption(SH_NOUNSET) && !isastchar(mode) && (type==M_VNAME || type==M_SIZE))
 				errormsg(SH_DICT,ERROR_exit(1),e_notset,id);
 			v = 0;

src/cmd/ksh93/tests/variables.sh

@@ -680,4 +680,62 @@ level=$($SHELL -c $'$SHELL -c \'print -r "$SHLVL"\'')
 $SHELL -c 'unset .sh' 2> /dev/null
 [[ $? == 1 ]] || err_exit 'unset .sh should return 1'
 
+#'
+
+# ======
+# ${var+set} within a loop.
+_test_isset() { eval "
+	$1=initial_value
+	function _$1_test {
+		typeset $1	# make local and initially unset
+		for i in 1 2 3 4 5; do
+			case \${$1+s} in
+			( s )	print -n 'S'; unset -v $1 ;;
+			( '' )	print -n 'U'; $1='' ;;
+			esac
+		done
+	}
+	_$1_test
+	[[ -n \${$1+s} && \${$1} == initial_value ]] || exit
+	for i in 1 2 3 4 5; do
+		case \${$1+s} in
+		( s )	print -n 's'; unset -v $1 ;;
+		( '' )	print -n 'u'; $1='' ;;
+		esac
+	done
+"; }
+expect='USUSUsusus'
+actual=$(_test_isset var)
+[[ "$actual" = "$expect" ]] || err_exit "\${var+s} expansion fails in loops (expected '$expect', got '$actual')"
+actual=$(_test_isset IFS)
+[[ "$actual" = "$expect" ]] || err_exit "\${IFS+s} expansion fails in loops (expected '$expect', got '$actual')"
+
+# [[ -v var ]] within a loop.
+_test_v() { eval "
+	$1=initial_value
+	function _$1_test {
+		typeset $1	# make local and initially unset
+		for i in 1 2 3 4 5; do
+			if	[[ -v $1 ]]
+			then	print -n 'S'; unset -v $1
+			else	print -n 'U'; $1=''
+			fi
+		done
+	}
+	_$1_test
+	[[ -v $1 && \${$1} == initial_value ]] || exit
+	for i in 1 2 3 4 5; do
+		if	[[ -v $1 ]]
+		then	print -n 's'; unset -v $1
+		else	print -n 'u'; $1=''
+		fi
+	done
+"; }
+expect='USUSUsusus'
+actual=$(_test_v var)
+[[ "$actual" = "$expect" ]] || err_exit "[[ -v var ]] expansion fails in loops (expected '$expect', got '$actual')"
+actual=$(_test_v IFS)
+[[ "$actual" = "$expect" ]] || err_exit "[[ -v IFS ]] expansion fails in loops (expected '$expect', got '$actual')"
+
+# ======
 exit $((Errors<125?Errors:125))