Fix conditional expansions ${array[i]=value}, ${array[i]?error}

$ unset foo
    $ echo ${foo[42]=bar}
    (empty line)

Instead of the empty line, 'bar' was expected. As foo[42] was
unset, the conditional assignment should have worked.

    $ unset foo
    $ : ${foo[42]?error: unset}
    (no output)

The expansion should have thrown an error with the given message.

This bug was introduced in ksh 93t 2008-10-01. Thanks to @JohnoKing
for finding the breaking change.

Analysis: The problem was experimenally determined to be in in the
following lines of nv_putsub(). If the array member is unset (i.e.
null), the value is set to the empty string instead:

src/cmd/ksh93/sh/array.c
1250: 		else
1251:			ap->val[size].cp = Empty;

It makes some sense: if there is a value (even an empty one), the
variable is set and these expansions should behave accordingly.
Sure enough, deleting these lines fixes the bug, but at the expense
of introducing a lot of other array-related regressions. So we need
a way to special-case the affected expansions.

Where to do this? If we replace line 1251 with an abort(3) call, we
get this stack trace:

0   libsystem_kernel.dylib    __pthread_kill + 10
1   libsystem_pthread.dylib   pthread_kill + 284
2   libsystem_c.dylib         abort + 127
3   ksh                       nv_putsub + 1411 (array.c:1255)
4   ksh                       nv_endsubscript + 940 (array.c:1547)
5   ksh                       nv_create + 4732 (name.c:1066)
6   ksh                       nv_open + 1951 (name.c:1425)
7   ksh                       varsub + 4934 (macro.c:1322)
[rest omitted]

The special-casing needs to be done on line 1250 of array.c, but
flagged in varsub() which processes these expansions. So, varsub()
calls nv_open() calls nv_create() calls nv_endsubscript() calls
nv_putsub(). That's a fairly deep call stack, so passing an extra
flag argument does not seem doable. I did try an approach using a
couple of new bit flags passed via these functions' flags and mode
parameters, but the way this code base uses bit flags is so
intricate, it does not seem to be possible to add or change
anything without unwanted side effects in all sorts of places.

So the only fix I can think of adds yet another global flag
variable for a very special case. It's ugly, but it works.
An elegant fix would probably involve a fairly comprehensive
redesign, which is simply not going to happen.

src/cmd/ksh93/include/shell.h:
- Add global sh.cond_expan flag.

src/cmd/ksh93/sh/array.c: nv_putsub():
- Do not set value to empty string if sh.cond_expan is set.

src/cmd/ksh93/sh/macro.c: varsub():
- Set sh.cond_expan flag while calling nv_open() for one of the
  affected expansions.
- Minor refactoring for legibility and to make the fix fit better.
- SSOT: Instead of repeating string "REPLY", use the node's nvname.
- Do not pointlessly add an extra 0 byte when saving id for error
  message; sfstruse() already adds this.

Thanks to @oguz-ismail for the bug report.

Resolves: https://github.com/ksh93/ksh/issues/383

NEWS

@@ -5,6 +5,10 @@ Any uppercase BUG_* names are modernish shell bug IDs.
 
 2022-02-05:
 
+- Fixed: for indexed arrays, given an unset array member a[i] with i > 0,
+  ${a[i]=value} failed to assign the value and ${v[i]?error} failed to throw
+  an error.
+
 - Fixed: the -a/--allexport shell option incorrectly exported some variables
   with names containing a dot to the environment. Note that the '-x' typeset
   attribute may still be set for them, but this will now not have any

src/cmd/ksh93/include/shell.h

@@ -354,6 +354,7 @@ struct Shell_s
 	void		*defpathlist;
 	void		*cdpathlist;
 	char		**argaddr;
+	char		cond_expan;	/* set while processing ${var=val}, ${var:=val}, ${var?err}, ${var:?err} */
 	void		*optlist;
 	struct sh_scoped global;
 	struct checkpt	checkbase;

src/cmd/ksh93/sh/array.c

@@ -1247,7 +1247,7 @@ Namval_t *nv_putsub(Namval_t *np,register char *sp,register long mode)
 					nv_arraychild(np,mp,0);
 					nv_setvtree(mp);
 				}
-				else
+				else if(!sh.cond_expan)
 					ap->val[size].cp = Empty;
 				if(!sp && !array_covered(np,ap))
 					ap->header.nelem++;

src/cmd/ksh93/sh/macro.c

@@ -1304,27 +1304,28 @@ retry1:
 			if(c=='=' || (c==':' && d=='='))
 				flag |= NV_ASSIGN;
 			flag &= ~NV_NOADD;
+			sh.cond_expan = 1;	/* tell nv_putsub() not to change value from null to empty */
+			np = nv_open(id,sh.var_tree,flag|NV_NOFAIL);
+			sh.cond_expan = 0;
 		}
 #if  SHOPT_FILESCAN
-		if(sh.cur_line && *id=='R' && strcmp(id,"REPLY")==0)
+		else if(sh.cur_line && strcmp(id,REPLYNOD->nvname)==0)
 		{
 			sh.argaddr=0;
 			np = REPLYNOD;
 		}
-		else
 #endif  /* SHOPT_FILESCAN */
+		else
 		{
 			if(sh.argaddr)
 				flag &= ~NV_NOADD;
-			/*
-			 * Get a node pointer (np) to the parameter, if any.
-			 */
 			np = nv_open(id,sh.var_tree,flag|NV_NOFAIL);
-			if(!np)
+		}
-			{
+		if(!np)
-				sfprintf(sh.strbuf,"%s%c",id,0);
+		{
-				id = sfstruse(sh.strbuf);
+			/* id points to stack, which will be overwritten; save it for error message */
-			}
+			sfputr(sh.strbuf,id,-1);
+			id = sfstruse(sh.strbuf);
 		}
 		if(isastchar(mode))
 			var = 0;

src/cmd/ksh93/tests/arrays.sh

@@ -783,5 +783,25 @@ got=$("$SHELL" -c '
 [[ $exp == $got ]] || err_exit 'Unsetting all elements of an array adds a spurious [0] element' \
 	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
 
+# ======
+# https://github.com/ksh93/ksh/issues/383
+unset foo
+exp=bar
+got=$(echo "${foo[42]=bar}")
+[[ $got == "$exp" ]] || err_exit '${array[index]=value} does not assign value' \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+exp=baz
+got=$(echo "${foo[42]:=baz}")
+[[ $got == "$exp" ]] || err_exit '${array[index]:=value} does not assign value' \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+exp=': parameter not set'
+got=$(set +x; redirect 2>&1; : ${foo[42]?})
+[[ $got == *"$exp" ]] || err_exit '${array[index]?error} does not throw error' \
+	"(expected match of *$(printf %q "$exp"), got $(printf %q "$got"))"
+exp=': parameter null'
+got=$(set +x; redirect 2>&1; foo[42]=''; : ${foo[42]:?})
+[[ $got == *"$exp" ]] || err_exit '${array[index]:?error} does not throw error' \
+	"(expected match of *$(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))