ast icon indicating copy to clipboard operation
ast copied to clipboard

What do you think `typeset -i13b x` or `typeset -i11m x` does?

Open krader1961 opened this issue 4 years ago • 1 comments

While working on resolving issue #507 I recently introduced optget_long() as an alternative to the borg standard getopt_long() or the AST optget() function. Having dealt with all the simple commands that are easy to convert I started work on changing the typeset command to not use optget() to parse its arguments.

Begin with the recognition that the typeset command recognizes i, b and m short options but not k. With short option i optionally taking a numeric value. Furthermore, b, k and m are valid scaling suffixes for numeric args parsed by strton64() as used by optget().

Now ponder the following sequence of commands:

typeset -i v0
typeset -p v0

typeset -i13b v1
typeset -p v1
typeset -i 13b v2
typeset -p v2
typeset -i13b -b v3
typeset -p v3

typeset -i11m v4
typeset -p v4
typeset -i 11m v5
typeset -p v5
typeset -i11m -m v6
typeset -p v6

typeset -i7k v7
typeset -p v7
typeset -i 7k v8
typeset -p v8
typeset -i7k -k v9
typeset -p v9

typeset -i5a v10
typeset -p v10
typeset -i 5 -a v11
typeset -p v11
typeset -i 5a v12
typeset -p 5a
typeset -p v12

If you put those statements in a file named /tmp/k and run ksh /tmp/k it terminates prematurely. Specifically, on the typeset -i11m -m v6 statement. This appears to be a bug since both -i and -m are valid short options. Albeit not when used together. Nonetheless, I can't see any reason for it to terminate the script. ksh93u+ produces no useful diagnostic output while ksh built from the current master branch produces the marginally more useful /tmp/x[15]: typeset: : invalid variable name. The missing value in that message is presumably supposed to be -m or m since that isn't a valid option or var name. Regardless, there is no obvious reason why that error should terminate the script.

This is what you see when running each of those statements individually in an interactive shell:

KSH PROMPT:1: typeset -i v0
KSH PROMPT:2: typeset -p v0
typeset -i v0
KSH PROMPT:3: typeset -i13b v1
KSH PROMPT:4: typeset -p v1
typeset -i 6656 v1
KSH PROMPT:5: typeset -i 13b v2
KSH PROMPT:6: typeset -p v2
typeset -i 6656 v2
KSH PROMPT:7: typeset -i13b -b v3
KSH PROMPT:8: typeset -p v3
typeset -i 6656 v3=''
KSH PROMPT:9: typeset -i11m v4
KSH PROMPT:10: typeset -p v4
typeset -i 11000000 v4
KSH PROMPT:11: typeset -i 11m v5
KSH PROMPT:12: typeset -p v5
typeset -i 11000000 v5
KSH PROMPT:13: typeset -i11m -m v6
ksh: typeset: : invalid variable name
KSH PROMPT:14: typeset -p v6
KSH PROMPT:15: typeset -i7k v7
KSH PROMPT:16: typeset -p v7
typeset -i 7000 v7
KSH PROMPT:17: typeset -i 7k v8
KSH PROMPT:18: typeset -p v8
typeset -i 7000 v8
KSH PROMPT:19: typeset -i7k -k v9
ksh: typeset: -k: unknown option
Usage: typeset [-bflmnprstuxACHS] [-a[type]] [-i[base]] [-E[n]] [-F[n]] [-L[n]] [-M[mapping]]
               [-R[n]] [-X[n]] [-h string] [-T[tname]] [-Z[n]] [name[=value]...]
   Or: typeset [ options ] -f [name...]
KSH PROMPT:20: typeset -p v9
KSH PROMPT:21: typeset -i5a v10
KSH PROMPT:22: typeset -p v10
typeset -a -i 5 v10
KSH PROMPT:23: typeset -i 5 -a v11
KSH PROMPT:24: typeset -p v11
typeset -a -i 5 v11
KSH PROMPT:25: typeset -i 5a v12
ksh: typeset: 5a: invalid variable name
KSH PROMPT:26: typeset -p 5a
KSH PROMPT:27: typeset -p v12

Basically, the optget() behavior, in conjunction with the b_typeset() implementation, is so confusing it can't be properly documented or understood by anyone who didn't create this UX. I'm going to have to sleep on this state of affairs for a few days, or weeks, before proceeding. We have to get rid of the optget() function as it has numerous problems and outright bugs. The question before us is how to do so while minimizing the likelihood of breaking any scripts.

krader1961 avatar Oct 27 '19 05:10 krader1961

FWIW, Here are the current list of serious issues that suggest potential bugs are present in the optget() implementation. These are reported by clang compiler static analysis:

src/lib/libast/misc/optget.c:876:29: Value stored to 's' is never read
src/lib/libast/misc/optget.c:878:29: Value stored to 's' is never read
src/lib/libast/misc/optget.c:880:29: Value stored to 's' is never read
src/lib/libast/misc/optget.c:1695:29: Value stored to 'f' is never read
src/lib/libast/misc/optget.c:3362:29: Value stored to 'tp' is never read
src/lib/libast/misc/optget.c:3732:5: Value stored to 'n' is never read
src/lib/libast/misc/optget.c:3834:22: Value stored to 'c' is never read
src/lib/libast/misc/optget.c:4212:33: Value stored to 'psp' is never read
src/lib/libast/misc/optget.c:4619:37: Value stored to 'nov' is never read
src/lib/libast/misc/optget.c:4629:40: Value stored to 'num' is never read
src/lib/libast/misc/optget.c:4657:33: Value stored to 'a' is never read
src/lib/libast/misc/optget.c:4725:9: Value stored to 'psp' is never read
src/lib/libast/misc/optget.c:4832:34: The left operand of '<=' is a garbage value

There are many more serious style issues such as this one which make the code extremely hard to understand:

src/lib/libast/misc/optget.c:1524:60: deep nested block [size|P3] Block depth of 12 exceeds limit of 7

Note that I've already tweaked the lint rules (see ./.oclint) to increase the acceptable nested block depth from 5 to 7 just because there are so many deeply nested blocks in this project. We can argue in good faith whether nesting blocks 6, 8, or some other amount is unacceptable but I would hope everyone agrees that nesting 12 levels is unacceptable because no mere mortal can make sense of code like that.

krader1961 avatar Oct 28 '19 02:10 krader1961