Assigning an array to a nameref "placeholder"

Assigning an array to a nameref "placeholder"

[Bart Schaefer]

Another one from the ksh93u+ gitlab:

> nameref unsetref; unsetref+=(foo bar). This now produces a "removing nameref attribute" warning before performing the assignment.

I would rather make this an error, but it's probably not difficult
either way.  Comments?

Re: Assigning an array to a nameref "placeholder"

[Bart Schaefer]

On Mon, Feb 26, 2024 at 2:18 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> Another one from the ksh93u+ gitlab:
>
> > nameref unsetref; unsetref+=(foo bar). This now produces a "removing nameref attribute" warning before performing the assignment.
>
> I would rather make this an error, but it's probably not difficult
> either way.  Comments?

Incidentally:

% typeset x
% typeset -p x
typeset x=''
% x+=(a b)
% typeset -p x
typeset -a x=( '' a b )

This has been the case for as long as += has been around, as far as I
can tell.  Works this way even with typesettounset, but not if the
parameter was never declared or has been explicitly removed with
"unset".

[PATCH] Appending an array to a scalar

[Bart Schaefer]

On Mon, Feb 26, 2024 at 6:51 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> % x+=(a b)
> % typeset -p x
> typeset -a x=( '' a b )
>
> This has been the case for as long as += has been around, as far as I
> can tell.  Works this way even with typesettounset

Hm, this is actually consistent with ksh, except for the
typesettounset bit, which is a recent addition.

diff --git a/Src/params.c b/Src/params.c
index 225acb8a1..8fb73f6a0 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3345,7 +3345,7 @@ assignaparam(char *s, char **val, int flags)
        } else if (!(PM_TYPE(v->pm->node.flags) & (PM_ARRAY|PM_HASHED)) &&
                 !(v->pm->node.flags & (PM_SPECIAL|PM_TIED))) {
            int uniq = v->pm->node.flags & PM_UNIQUE;
-           if (flags & ASSPM_AUGMENT) {
+           if ((flags & ASSPM_AUGMENT) && !(v->pm->node.flags & PM_UNSET)) {
                /* insert old value at the beginning of the val array */
                char **new;
                int lv = arrlen(val);

Re: Assigning an array to a nameref "placeholder"

[Stephane Chazelas]

2024-02-26 14:18:03 -0800, Bart Schaefer:
> Another one from the ksh93u+ gitlab:
> 
> > nameref unsetref; unsetref+=(foo bar). This now produces a "removing nameref attribute" warning before performing the assignment.
> 
> I would rather make this an error, but it's probably not difficult
> either way.  Comments?

An error (or forbidding typeset -n ref=var without the =var part
like in mksh) would make a lot more sense to me as well.

The full commit log (see below) and
https://github.com/ksh93/ksh/issues/678#issuecomment-1716507741
gives the rationale there for not doing so.

In zsh, there's no backward compatibility to be considered as
the feature is new, so it's our best opportunity to make it the
best interface from the get go.

Note that bash has been known not to always make the best design
choices (including when copying other shells), so it's not
necessarily a good idea to use it as a reference.

Issuing a warning here would only make sense as preserving
backward compatibility while at the same warning users against
doing anything like that.

commit ebfbe62863c36a7b99eb0aa9f831184cdde86f3f
Author: Martijn Dekker <martijn@inlv.org>
Date:   Wed Sep 13 11:25:28 2023 +0100

    Fix segfault in unsetref+=(foo); add warning

    Emanuele Torre (@emanuele6) writes:
    > Appending a non-empty indexed array to an unset nameref crashes
    > with a segmentation fault:
    > $ arch/*/bin/ksh -c 'typeset -n ref; ref+=(); typeset -p ref'
    > typeset -n ref
    > $ arch/*/bin/ksh -c 'typeset -n ref; ref+=(foo); typeset -p ref'
    > Segmentation fault (core dumped)
    [...]
    > Appending an associative array errors correctly:
    > $ arch/*/bin/ksh -c 'typeset -n ref; ref+=([foo]=bar); typeset -p ref'
    > arch/linux.i386-64/bin/ksh: ref: no reference name

    For consistency, this commit makes all array assignments to an
    unset nameref (using either = or +=) succeed, but print a warning
    that the nameref attribute is removed -- a bash-like bahaviour.

    I think that this will cause fewer backward compatibility problems
    than making such assignments error out, since they used to succeed
    in some cases (e.g. 'typeset -n ref; ref=(foo bar)' always worked).

    It does mean the associative array reproducer above now generates a
    warning instead of an error.

    src/cmd/ksh93/sh/name.c: nv_create():
    - After dereferencing a nameref, if nv_isref(np) is still true, we
      know it must be an unset nameref. In that case, add a check for
      c==0 (the value occurring upon var=(foo) and var+=(foo)), and
      produce a warning instead of an error, as argued above.

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

Re: Assigning an array to a nameref "placeholder"

[Bart Schaefer]

On Mon, Feb 26, 2024 at 10:48 PM Stephane Chazelas
<stephane@chazelas.org> wrote:
>
> 2024-02-26 14:18:03 -0800, Bart Schaefer:
> >
> > > nameref unsetref; unsetref+=(foo bar). This now produces a "removing nameref attribute" warning before performing the assignment.
> >
> > I would rather make this an error, but it's probably not difficult
> > either way.  Comments?
>
> An error (or forbidding typeset -n ref=var without the =var part
> like in mksh) would make a lot more sense to me as well.

Forbidding "typeset -n ref" would make using the "for" loop variation
a bit weird, but I'm perfectly fine with an error on assigning
something nonsensical to a nameref.

Re: Assigning an array to a nameref "placeholder"

[Stephane Chazelas]

2024-02-27 18:10:44 -0800, Bart Schaefer:
> On Mon, Feb 26, 2024 at 10:48 PM Stephane Chazelas
> <stephane@chazelas.org> wrote:
> >
> > 2024-02-26 14:18:03 -0800, Bart Schaefer:
> > >
> > > > nameref unsetref; unsetref+=(foo bar). This now produces a "removing nameref attribute" warning before performing the assignment.
> > >
> > > I would rather make this an error, but it's probably not difficult
> > > either way.  Comments?
> >
> > An error (or forbidding typeset -n ref=var without the =var part
> > like in mksh) would make a lot more sense to me as well.
> 
> Forbidding "typeset -n ref" would make using the "for" loop variation
> a bit weird, but I'm perfectly fine with an error on assigning
> something nonsensical to a nameref.

The forbidding "typeset -n ref" would also come with removing
that surprising "for" loop variation (also like in mksh) as
discussed in another thread.

I'm still not convinced we have to follow ksh93/bash on that,
but then again I didn't take part of the initial discussions on
namerefs so I acknowledge I may be too late to the party.

-- 
Stephane