Zsh Mailing List Archive
Messages sorted by:
Reverse Date,
Date,
Thread,
Author
Re: PATCH: Disallow array initializer for named reference
While trying to understand your updated patch, I discovered that PM_TYPE doesn't include PM_NAMEREF. This looks like a recipe for future bugs. In fact, the two changes in your patch fix two of such bugs. The first change is needed because the previous condition failed to account for references because PM_TYPE doesn't include PM_NAMEREF. The second change is needed because
this condition fails to account for references again because PM_TYPE doesn't include PM_NAMEREF.
Below is a patch that adds PM_NAMEREF to PM_TYPE. All tests pass and the following example then triggers an "inconsistent type for assignment" error via the second check.
typeset -n ref=()
Do you have an example that triggers the first check?
In my patch, I mainly added a "case PM_NAMEREF" to all switches on PM_TYPE that included a "case PM_SCALAR". I should still have a look at all the other calls to PM_TYPE to double check that there is nothing obviously wrong but I didn't want to do that before knowing whether such a change would be OK.
I understand that references often go through dedicated code paths and many PM_TYPE will (or at least should) never be reached by references. However, as the two bugs that you fix in your patch show, some PM_TYPE will be reached by references. For these I think it's much better that PM_TYPE includes PM_NAMEREF because otherwise it's way too easy to forget that you have to add an extra check for references.
Note that despite the error on the second declaration, all three
declarations were completed. This happens because bin_typeset() calls
typeset_single() in a loop, and does not break the loop on errflag.
Handling errors is notoriously difficult. I suspect that if we start testing other errors with always statements, we will discover plenty of bugs.
Philippe
On Tue, Jun 10, 2025 at 3:38 PM Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jun 10, 2025 at 2:07 PM Philippe Altherr
> <philippe.altherr@xxxxxxxxx> wrote:
> >
> > I would rather change the existing "inconsistent type for assignment" errors into warnings than change the new one into an error.
>
> Yeah, that probably wouldn't fly, because scripts that now stop would
> instead continue running with parameters that don't exist or have
> different type than before.
OK, the attache handles the error at the same point in the code where
other "inconsistent type" errors are handled, so that's very
consistent. Separate patch for the options mismatching.
One curious thing (and this is true of this error for pre-existing
cases, not just namerefs):
% () {
{ typeset -n ref1 ref2=(x) ref3=foo } always
{ typeset -p ref1 ref2 ref3 }
}
(anon):typeset:1: ref2: inconsistent type for assignment
(anon):typeset:2: no such variable: ref2
typeset -n ref1=''
typeset -n ref3=foo
Note that despite the error on the second declaration, all three
declarations were completed. This happens because bin_typeset() calls
typeset_single() in a loop, and does not break the loop on errflag.
Patch replaces workers/53758.
diff --git a/Src/Modules/param_private.c b/Src/Modules/param_private.c
index 044617190..57c53ef61 100644
--- a/Src/Modules/param_private.c
+++ b/Src/Modules/param_private.c
@@ -100,6 +100,7 @@ makeprivate(HashNode hn, UNUSED(int flags))
/* why have a union if we need this switch anyway? */
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
pm->gsu.s->setfn(pm, tpm->u.str);
tpm->u.str = NULL;
break;
@@ -138,6 +139,7 @@ makeprivate(HashNode hn, UNUSED(int flags))
struct gsu_closure *gsu = zalloc(sizeof(struct gsu_closure));
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
gsu->g = (void *)(pm->gsu.s);
gsu->u.s = scalar_private_gsu;
pm->gsu.s = (GsuScalar)gsu;
@@ -180,6 +182,7 @@ is_private(Param pm)
{
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
if (!pm->gsu.s || pm->gsu.s->unsetfn != pps_unsetfn)
return 0;
break;
diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index 7441c30b8..05e20d144 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -49,15 +49,14 @@ paramtypestr(Param pm)
if (pm->node.flags & PM_AUTOLOAD)
return dupstring("undefined");
- /* For simplicity we treat PM_NAMEREF as PM_TYPE(PM_SCALAR) */
- switch (PM_TYPE(f)|(f & PM_NAMEREF)) {
+ switch (PM_TYPE(f)) {
case PM_SCALAR: val = "scalar"; break;
+ case PM_NAMEREF: val = "nameref"; break;
case PM_ARRAY: val = "array"; break;
case PM_INTEGER: val = "integer"; break;
case PM_EFLOAT:
case PM_FFLOAT: val = "float"; break;
case PM_HASHED: val = "association"; break;
- case PM_NAMEREF: val = "nameref"; break;
}
DPUTS(!val, "BUG: type not handled in parameter");
val = dupstring(val);
diff --git a/Src/Zle/complete.c b/Src/Zle/complete.c
index 342611f1f..1836c761d 100644
--- a/Src/Zle/complete.c
+++ b/Src/Zle/complete.c
@@ -1308,6 +1308,7 @@ addcompparams(struct compparam *cp, Param *pp)
if ((pm->u.data = cp->var)) {
switch(PM_TYPE(cp->type)) {
case PM_SCALAR:
+ case PM_NAMEREF:
pm->gsu.s = &compvarscalar_gsu;
break;
case PM_INTEGER:
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 6ac458c91..8baf1053b 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -3699,6 +3699,7 @@ bin_compquote(char *nam, char **args, Options ops, UNUSED(int func))
if ((v = getvalue(&vbuf, &name, 0))) {
switch (PM_TYPE(v->pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
setstrvalue(v, ztrdup(comp_quote(getstrvalue(v),
OPT_ISSET(ops,'p'))));
break;
diff --git a/Src/Zle/zle_params.c b/Src/Zle/zle_params.c
index 9f4fb5ac2..9488ce8a6 100644
--- a/Src/Zle/zle_params.c
+++ b/Src/Zle/zle_params.c
@@ -207,6 +207,7 @@ makezleparams(int ro)
pm->u.data = zp->data;
switch(PM_TYPE(zp->type)) {
case PM_SCALAR:
+ case PM_NAMEREF:
pm->gsu.s = zp->gsu;
break;
case PM_ARRAY:
diff --git a/Src/builtin.c b/Src/builtin.c
index 5563bdba9..2271e3a3d 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2625,6 +2625,7 @@ typeset_single(char *cname, char *pname, Param pm, int func,
*/
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
pm->gsu.s->setfn(pm, ztrdup(""));
break;
case PM_INTEGER:
diff --git a/Src/exec.c b/Src/exec.c
index c1181c5eb..85b8ec3d0 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4517,6 +4517,7 @@ restore_params(LinkList restorelist, LinkList removelist)
tpm->node.flags = pm->node.flags;
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
tpm->gsu.s->setfn(tpm, pm->u.str);
break;
case PM_INTEGER:
diff --git a/Src/module.c b/Src/module.c
index b4b5d0a2c..e4a900008 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -1085,6 +1085,7 @@ addparamdef(Paramdef d)
*/
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
pm->gsu.s = d->gsu ? (GsuScalar)d->gsu : &varscalar_gsu;
break;
diff --git a/Src/params.c b/Src/params.c
index 7b515515e..878afd9b0 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -971,6 +971,7 @@ assigngetset(Param pm)
{
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
pm->gsu.s = &stdscalar_gsu;
break;
case PM_INTEGER:
@@ -1223,6 +1224,7 @@ copyparam(Param tpm, Param pm, int fakecopy)
}
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
tpm->u.str = ztrdup(pm->gsu.s->getfn(pm));
break;
case PM_INTEGER:
@@ -2353,6 +2355,7 @@ getstrvalue(Value v)
v->pm->base, v->pm->node.flags, NULL);
break;
case PM_SCALAR:
+ case PM_NAMEREF:
s = v->pm->gsu.s->getfn(v->pm);
break;
default:
@@ -2689,6 +2692,7 @@ assignstrvalue(Value v, char *val, int flags)
v->pm->node.flags &= ~PM_UNSET;
switch (PM_TYPE(v->pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
if (v->start == 0 && v->end == -1) {
v->pm->gsu.s->setfn(v->pm, val);
if ((v->pm->node.flags & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z)) &&
@@ -2845,6 +2849,7 @@ setnumvalue(Value v, mnumber val)
}
switch (PM_TYPE(v->pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
case PM_ARRAY:
if ((val.type & MN_INTEGER) || outputradix) {
if (!(val.type & MN_INTEGER))
@@ -3249,6 +3254,7 @@ assignsparam(char *s, char *val, int flags)
if (v->start == 0 && v->end == -1) {
switch (PM_TYPE(v->pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
v->start = INT_MAX; /* just append to scalar value */
break;
case PM_INTEGER:
@@ -3285,6 +3291,7 @@ assignsparam(char *s, char *val, int flags)
} else {
switch (PM_TYPE(v->pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
if (v->end > 0)
v->start = v->end;
else
@@ -3900,6 +3907,7 @@ stdunsetfn(Param pm, UNUSED(int exp))
{
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
if (pm->gsu.s->setfn)
pm->gsu.s->setfn(pm, NULL);
break;
@@ -5872,6 +5880,7 @@ scanendscope(HashNode hn, UNUSED(int flags))
if (!(tpm->node.flags & (PM_NORESTORE|PM_READONLY)))
switch (PM_TYPE(pm->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
pm->gsu.s->setfn(pm, tpm->u.str);
break;
case PM_INTEGER:
@@ -5981,6 +5990,7 @@ printparamvalue(Param p, int printflags)
* on the type of the parameter */
switch (PM_TYPE(p->node.flags)) {
case PM_SCALAR:
+ case PM_NAMEREF:
/* string: simple output */
if (p->gsu.s->getfn && (t = p->gsu.s->getfn(p)))
quotedzputs(t, stdout);
diff --git a/Src/subst.c b/Src/subst.c
index d0f2a1b45..1aa5e0709 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2812,14 +2812,14 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
!(v->pm->node.flags & PM_UNSET))) {
int f = v->pm->node.flags;
- switch (PM_TYPE(f)|(f & PM_NAMEREF)) {
+ switch (PM_TYPE(f)) {
case PM_SCALAR: val = "scalar"; break;
+ case PM_NAMEREF: val = "nameref"; break;
case PM_ARRAY: val = "array"; break;
case PM_INTEGER: val = "integer"; break;
case PM_EFLOAT:
case PM_FFLOAT: val = "float"; break;
case PM_HASHED: val = "association"; break;
- case PM_NAMEREF: val = "nameref"; break;
}
val = dupstring(val);
if (v->pm->level)
diff --git a/Src/zsh.h b/Src/zsh.h
index 4e5c02980..d864819f0 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1878,7 +1878,7 @@ struct tieddata {
#define PM_HASHED (1<<4) /* association */
#define PM_TYPE(X) \
- (X & (PM_SCALAR|PM_INTEGER|PM_EFLOAT|PM_FFLOAT|PM_ARRAY|PM_HASHED))
+ (X & (PM_SCALAR|PM_INTEGER|PM_EFLOAT|PM_FFLOAT|PM_ARRAY|PM_HASHED|PM_NAMEREF))
#define PM_LEFT (1<<5) /* left justify, remove leading blanks */
#define PM_RIGHT_B (1<<6) /* right justify, fill with leading blanks */
Messages sorted by:
Reverse Date,
Date,
Thread,
Author