.sh.subshell is unset in PS4

[McDutchie]

Description of problem:

I like to track the level of non-forking subshells (${.sh.subshell}) in xtrace output, so I make it part of the PS4 prompt. However, on the current development version, .sh.subshell appears unset in the main shell environment -- but only in PS4 (that is, something like echo ${.sh.subshell} outputs 0 as expected).

Ksh version:

Version A 2017.0.0-devel-2141-g1b6198b

How reproducible:

Always

Steps to reproduce:

build/src/cmd/ksh93/ksh -u -c \
    'PS4="\${.sh.subshell:+S\${.sh.subshell},}$PS4";
        set -x; echo one; echo $(echo two); echo `echo $(echo three)`'

Actual results:

build/src/cmd/ksh93/ksh: .sh.subshell: parameter not set
${.sh.subshell:+S${.sh.subshell},}+ echo one
one
S1,+ echo two
+ echo two
two
S1,+ echo three
+ echo three
+ echo three
three

Expected results:

S0,+ echo one
one
S1,+ echo two
S0,+ echo two
two
S2,+ echo three
S1,+ echo three
S0,+ echo three
three

Additional info:

Note also that the result of the ${.sh.subshell:+S\${.sh.subshell},} expansion is corrupted on the first expansion: it should not be erroring out on account of the -u (a.k.a. -o nounset) option.

[siteshwar]

It's reproducible on ksh 93v- too.

[krader1961]

And unsurprisingly it works as expected in ksh93u+. It is a shame that hindsight is 20/20 because I really wish we could rewind the clock and decide instead to base our work on the last stable release. My xmas wish is for someone to interview the people making changes to the code between ~2012 and ~2014 to help us understand how so many, significant, bugs managed to be introduced in that two year period.

[siteshwar]

Attributes of .sh.subshell seems to be getting modified when it's accessed. For e.g. this script:

set -u
set -x
echo ${.sh.subshell}
echo ${.sh.subshell}

outputs:

+ echo 0
0
/tmp/test.sh: line 4: .sh.subshell: parameter not set

[siteshwar]

This is the code in question:

1290             if (np && (flag & NV_NOADD) && nv_isnull(np)) {
1291                 if (nv_isattr(np, NV_NOFREE)) {
1292                     nv_offattr(np, NV_NOFREE);
1293                 } else if (np != REPLYNOD || !mp->shp->cur_line) {
1294                     np = 0;
1295                 }
1296             }

Line 1292 turns off NV_NOFREE attribute that later changes behavior.

[siteshwar]

Definition for nv_isnull was extended in last beta release from:

#define nv_isnull(np)	(!(np)->nvalue.cp && (nv_isattr(np,NV_SHORT|NV_INTEGER)!=(NV_SHORT|NV_INTEGER)) && !_nv_hasget(np))

to:

int nv_isnull(Namval_t *np) {
    if (np->nvalue.cp) return 0;
    if (np == IFSNOD) return 1;
    if (nv_isattr(np, NV_INT16) == NV_INT16 && !np->nvfun) {
        return nv_isattr(np, NV_NOTSET) == NV_NOTSET;
    }
    if (!nv_attr(np) || nv_isattr(np, NV_NOTSET) != NV_NOTSET) {
        return !np->nvfun || !np->nvfun->disc || !nv_hasget(np);
    }
    return 0;
}

Restoring old definition fixes this issue, but breaks test cases.

[siteshwar]

Quickest workaround I can think of this issue is:

diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c
index 02e192b1..d07ab00c 100644
--- a/src/cmd/ksh93/sh/name.c
+++ b/src/cmd/ksh93/sh/name.c
@@ -3163,7 +3163,10 @@ void *nv_context(Namval_t *np) { return np->nvfun; }
 int nv_isnull(Namval_t *np) {
     if (np->nvalue.cp) return 0;
     if (np == IFSNOD) return 1;
-    if (nv_isattr(np, NV_INT16) == NV_INT16 && !np->nvfun) {
+
+    // `shtab_variables` have `np->nvfun` set, but not `np->nvfun->disc`
+    // https://github.com/att/ast/issues/1092
+    if (nv_isattr(np, NV_INT16) == NV_INT16 && (!np->nvfun || !np->nvfun->disc)) {
         return nv_isattr(np, NV_NOTSET) == NV_NOTSET;
     }
     if (!nv_attr(np) || nv_isattr(np, NV_NOTSET) != NV_NOTSET) {

It does not break any test case, but I am still not sure if this is correct fix for this issue. I am going to put this issue in my backlog.