New typeset is a bit too verbose, and skips some assignments

New typeset is a bit too verbose, and skips some assignments

[Bart Schaefer]

torch% typeset x=()
torch% typeset x=() x[2]=a x[4]=b
x=()
torch% typeset -p x
typeset -a x
x=('' a '' b)
torch% 

The value of x should not have been displayed by the second typeset in
that example, because all the names have assignments.  It also looks
wrong, because it's the value from before the assignments to elements.

This only happens if the type is array before the new assignment, but
the initial empty assignment doesn't have any effect:

torch% typeset y   
torch% typeset y=() y[2]=a y[4]=b
torch% typeset y=() y[1]=x y[3]=z
y=('' a '' b)
torch% typeset -p y
y=(x a z b)
torch% 

I expected y=(x '' z '').

Re: New typeset is a bit too verbose, and skips some assignments

[Peter Stephenson]

On Thu, 25 Jun 2015 09:21:31 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> torch% typeset x=()
> torch% typeset x=() x[2]=a x[4]=b
> x=()
> torch% typeset -p x
> typeset -a x
> x=('' a '' b)
> torch% 
> 
> The value of x should not have been displayed by the second typeset in
> that example, because all the names have assignments.  It also looks
> wrong, because it's the value from before the assignments to elements.

That's related to my previous fix --- x=() causes x to be marked as an
array but with no value, not just an empty list.  However, the mere fact
that that assignment is marked as an array (this is completely separate
from the parameter currently being marked as an array or the typeset
having the -a flag, sob) means there was a value there originally, so
that's good enough to use to shut it up.

It's outputing x empty because it's only triggered by the x=() bit at
the start, i.e. exactly as if you'd put "x" there.

(NO_TYPESET_SILENT is pretty useless as a feature these days but it does
have a useful effect of showing up unnecessary declarations...)

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index ba38406..bc68545 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2141,7 +2141,8 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	    if (OPT_ISSET(ops,'p'))
 		paramtab->printnode(&pm->node, PRINT_TYPESET);
 	    else if (!OPT_ISSET(ops,'g') &&
-		     (unset(TYPESETSILENT) || OPT_ISSET(ops,'m')))
+		     (unset(TYPESETSILENT) || OPT_ISSET(ops,'m'))
+		     && !asg->is_array)
 		paramtab->printnode(&pm->node, PRINT_INCLUDEVALUE);
 	    return pm;
 	}

Re: New typeset is a bit too verbose, and skips some assignments

[Bart Schaefer]

On Jun 25,  5:40pm, Peter Stephenson wrote:
} Subject: Re: New typeset is a bit too verbose, and skips some assignments
}
} On Thu, 25 Jun 2015 09:21:31 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > The value of x should not have been displayed by the second typeset
} 
} That's related to my previous fix --- x=() causes x to be marked as an
} array but with no value, not just an empty list.

Thanks for the patch, but it still doesn't address my second point:

> the initial empty assignment doesn't have any effect:
> 
> torch% typeset y   
> torch% typeset y=() y[2]=a y[4]=b
> torch% typeset y=() y[1]=x y[3]=z
> y=('' a '' b)
> torch% typeset -p y
> y=(x a z b)
> torch% 
> 
> I expected y=(x '' z '').

This works for scalars, i.e.

torch% typeset x=abcd x[1]=1 x[3]=3
torch% typeset -p x
typeset x=1b3d
torch% typeset x='' x[2]=2 x[4]=4
torch% typeset -p x              
typeset x=24
torch% 

Note that I did NOT get x=1234 out of that last.

Re: New typeset is a bit too verbose, and skips some assignments

[Peter Stephenson]

On Fri, 26 Jun 2015 14:56:42 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Thanks for the patch, but it still doesn't address my second point:

Ah, you're simply saying "typeset thing=()" doesn't explicitly assign an
empty array.  (Long series of examples make my eyes glaze over, here as
elsewhere.)

The issue is the same as before --- asg->value.array is empty for an
array, but actually the fact that asg->is_array is set means there must
have been a value.  So the answer is a more general fix, replacing the
previous one --- ASB_VALUEP() is true any time asg->is_array is true.
One subtlety where we explicitly set asg->value.array to NULL in the
expectation it would be ignored now needs a variable to do the job.  (Or
we could get rid of asg->is_array but this is safer.)

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 3da1678..ac5a568 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -452,11 +452,13 @@ execbuiltin(LinkList args, LinkList assigns, Builtin bn)
 		    if (asg->is_array) {
 			LinkNode arrnode;
 			fprintf(xtrerr, "=(");
-			for (arrnode = firstnode(asg->value.array);
-			     arrnode;
-			     incnode(arrnode)) {
-			    fputc(' ', xtrerr);
-			    quotedzputs((char *)getdata(arrnode), xtrerr);
+			if (asg->value.array) {
+			    for (arrnode = firstnode(asg->value.array);
+				 arrnode;
+				 incnode(arrnode)) {
+				fputc(' ', xtrerr);
+				quotedzputs((char *)getdata(arrnode), xtrerr);
+			    }
 			}
 			fprintf(xtrerr, " )");
 		    } else if (asg->value.scalar) {
@@ -1975,7 +1977,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	       int on, int off, int roff, Asgment asg, Param altpm,
 	       Options ops, int joinchar)
 {
-    int usepm, tc, keeplocal = 0, newspecial = NS_NONE, readonly;
+    int usepm, tc, keeplocal = 0, newspecial = NS_NONE, readonly, dont_set = 0;
     char *subscript;
 
     /*
@@ -2131,8 +2133,9 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
      */
     if (usepm) {
 	if (asg->is_array ?
-	    (asg->value.array && !(PM_TYPE(pm->node.flags) & (PM_ARRAY|PM_HASHED))) :
-	    (asg->value.scalar && (PM_TYPE(pm->node.flags & (PM_ARRAY|PM_HASHED))))) {
+	    !(PM_TYPE(pm->node.flags) & (PM_ARRAY|PM_HASHED)) :
+	    (asg->value.scalar && (PM_TYPE(pm->node.flags &
+					   (PM_ARRAY|PM_HASHED))))) {
 	    zerrnam(cname, "%s: inconsistent type for assignment", pname);
 	    return NULL;
 	}
@@ -2141,8 +2144,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	    if (OPT_ISSET(ops,'p'))
 		paramtab->printnode(&pm->node, PRINT_TYPESET);
 	    else if (!OPT_ISSET(ops,'g') &&
-		     (unset(TYPESETSILENT) || OPT_ISSET(ops,'m'))
-		     && !asg->is_array)
+		     (unset(TYPESETSILENT) || OPT_ISSET(ops,'m')))
 		paramtab->printnode(&pm->node, PRINT_INCLUDEVALUE);
 	    return pm;
 	}
@@ -2199,9 +2201,10 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	    DPUTS(ASG_ARRAYP(asg), "BUG: typeset got array value where scalar expected");
 	    if (asg->value.scalar && !(pm = setsparam(pname, ztrdup(asg->value.scalar))))
 		return NULL;
-	} else if (asg->value.array) {
-	    DPUTS(!ASG_ARRAYP(asg), "BUG: typeset got scalar value where array expected");
-	    if (!(pm = setaparam(pname, zlinklist2array(asg->value.array))))
+	} else if (asg->is_array) {
+	    if (!(pm = setaparam(pname, asg->value.array ?
+				 zlinklist2array(asg->value.array) :
+				 mkarray(NULL))))
 		return NULL;
 	}
 	pm->node.flags |= (on & PM_READONLY);
@@ -2211,7 +2214,7 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
     }
 
     if (asg->is_array ?
-	(asg->value.array && !(on & (PM_ARRAY|PM_HASHED))) :
+	!(on & (PM_ARRAY|PM_HASHED)) :
 	(asg->value.scalar && (on & (PM_ARRAY|PM_HASHED)))) {
 	zerrnam(cname, "%s: inconsistent type for assignment", pname);
 	return NULL;
@@ -2343,20 +2346,21 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	     */
 	    if (!(pm = setsparam(pname, ztrdup(asg->value.scalar ? asg->value.scalar : ""))))
 		return NULL;
-	    asg->value.scalar = NULL;
+	    dont_set = 1;
 	    asg->is_array = 0;
 	    keeplocal = 0;
 	    on = pm->node.flags;
 	} else if (PM_TYPE(on) == PM_ARRAY && ASG_ARRAYP(asg)) {
-	    if (!(pm = setaparam(pname, asg->value.array ? zlinklist2array(asg->value.array) :
+	    if (!(pm = setaparam(pname, asg->value.array ?
+				 zlinklist2array(asg->value.array) :
 				 mkarray(NULL))))
 		return NULL;
-	    asg->value.array = NULL;
+	    dont_set = 1;
 	    keeplocal = 0;
 	    on = pm->node.flags;
 	} else {
 	    zerrnam(cname,
-		    "%s: inconsistent array element or slice assignment", pname);
+		    "%s: i1;nconsistent array element or slice assignment", pname);
 	    return NULL;
 	}
     }
@@ -2422,11 +2426,13 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	pm->level = keeplocal;
     else if (on & PM_LOCAL)
 	pm->level = locallevel;
-    if (ASG_VALUEP(asg)) {
+    if (ASG_VALUEP(asg) && !dont_set) {
 	Param ipm = pm;
 	if (pm->node.flags & (PM_ARRAY|PM_HASHED)) {
 	    DPUTS(!ASG_ARRAYP(asg), "BUG: inconsistent scalar value for array");
-	    if (!(pm=setaparam(pname, zlinklist2array(asg->value.array))))
+	    if (!(pm=setaparam(pname, asg->value.array ?
+			       zlinklist2array(asg->value.array) :
+			       mkarray(NULL))))
 		return NULL;
 	} else {
 	    DPUTS(ASG_ARRAYP(asg), "BUG: inconsistent array value for scalar");
diff --git a/Src/zsh.h b/Src/zsh.h
index ee06094..ce9b979 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1162,13 +1162,11 @@ struct asgment {
 
 /*
  * Assignment has value?
- * We need to arrange for each of the values
- * to be the same type or the compiler will
- * get fed up.
+ * If the assignment is an arrray, then it certainly has a value --- we
+ * can only tell if there's an expicit assignment.
  */
 
-#define ASG_VALUEP(asg) (ASG_ARRAYP(asg) ?			\
-			 ((asg)->value.array != (LinkList)0) :	\
+#define ASG_VALUEP(asg) (ASG_ARRAYP(asg) ||			\
 			 ((asg)->value.scalar != (char *)0))
 
 /* node in command path hash table (cmdnamtab) */
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index 5d69e5d..4c033cc 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -691,3 +691,19 @@
 >4 one umm er five
 >2 one five
 >4 nothing to see here
+
+  array=(no really nothing here)
+  fn() {
+    typeset array=() array[2]=two array[4]=four
+    typeset -p array
+    typeset array=() array[3]=three array[1]=one
+    typeset -p array
+  }
+  fn
+  print $array
+0:setting empty array in typeset
+>typeset -a array
+>array=('' two '' four)
+>typeset -a array
+>array=(one '' three)
+>no really nothing here