local keyword is not recognized in a function

[siteshwar]

Rerpoducer script:

test_func() {
    local stat=$(< /proc/self/stat)
    echo $stat
}

test_func

Running this script gives this error :

 local: local can only be used in a function

[dannyweldon]

Interestingly, it works correctly if the function is defined using the function keyword.

[deekej]

By the way, one more thing I'm seeing (ksh-20120801-31 version in Fedora 25):

$ cat ./test.ksh 
#!/bin/ksh

function func {
  local stat=($(< /proc/self/stat))
  echo $stat
}

func

exit 0

Results in:
./test.ksh: syntax error at line 4: (' unexpected`

However, changing the script like this:

#!/bin/ksh

function func {
  stat=($(< /proc/self/stat))
  echo $stat
  unset stat
}

func

exit 0

Will work OK - for example:

$ ./test.ksh
6785

The syntax error occurs only when code causing it is inside a function, and the variable is set to be local for that function.

[dannyweldon]

Hmm, I'm also seeing no output when running just:

echo $(</proc/self/stat)

[krader1961]

Note that this issue seems to be predicated on the SHOPT_BASH symbol. Which is being discussed in issue #238 for removal. The state of the ksh source code at this time which implements the local keyword is obviously incomplete and/or broken with respect to this feature.

[hlangeveld]

IIRC The AST group subscribed strongly to the idea of one interface, one name, one behaviour. When introducing new behaviour they would go out of their way to create a new interface with a new name.

From what I remember when typeset was introduced, the function keyword was created precisely for supporting local variables. The AST group did not want to "contaminate" old sh style functions with different behaviour, so a new type of function was created.

[colstrom]

@hlangeveld This is consistent with my understanding as well. func()-style functions behave as sh, function func-style functions support things like local.

[bk2204]

I'll point out a few things about the local keyword:

1. There's a request to the Austin Group (the POSIX folks) about standardizing it as part of POSIX which has garnered a lot of comments in favor. ksh has typically been capable of running POSIX commands, so it may be worthwhile to implement for that reason.
2. Debian requires all shells that can be used as /bin/sh to implement local (and they don't build with SHOPT_BASH). Consequently, it isn't possible to use ksh as /bin/sh on a Debian or Ubuntu system.
3. Git requires a shell providing local in order to run the testsuite and shell commands. Consequently, it isn't possible to use ksh as the shell for Git.
4. All other open source POSIX-compliant shells that I'm aware of implement local: bash, dash, mksh, pdksh (and derivatives, like OpenBSD's ksh), and zsh.

The first three would necessitate an implementation outside of SHOPT_BASH. Now, nothing requires that ksh fit these niches; it may be that folks prefer to preserve historical behavior over adding local, and that's fine. But if there's a desire to fill these use cases, then local should probably be implemented as a standard feature.

[dannyweldon]

I am in favour of having local as a standard builtin. It might be worth checking if it breaks scripts that have local defined as an alias like this, but it should be okay:

alias local=typeset

[krader1961]

Note that local is defined as a builtin alias for typeset here:

ast/src/cmd/ksh93/data/builtins.c

Lines 69 to 72 in 856e095

	#if SHOPT_BASH
	{"declare", NV_BLTIN | BLT_ENV | BLT_SPC | BLT_DCL, bltin(typeset)},
	{"local", NV_BLTIN | BLT_ENV | BLT_DCL, bltin(typeset)},
	#endif // SHOPT_BASH

What do people think about also making declare an explicit alias for typeset?

Note that the use of BLT_SPC in the definition of declare means it cannot be overridden by a function (and possibly other things) whereas local can. Whether the definition of local should also include BLT_SPC is unclear.

[krader1961]

Note that due to how the builtin definitions interact with the alias command you can override even typeset with an alias that does not invoke the builtin of the same name. Whether that is a feature or bug is open for debate. 😄 This means that adding local as a builtin without regard to whether ksh was built with SHOPT_BASH set to true does not affect something like alias local=typeset or alias local='echo WTF '.

[krader1961]

Note: the local command doesn't exist in the ksh93u+ release. It was added after that release as part of the (bogus, IMHO) attempt to implement a bash emulation mode. I'm happy to see new behaviors implemented such as local being an alias for typeset. But that should be done because it is generally useful. Not in a misguided attempt to emulate the behavior of a different shell and only if that emulation is turned on (e.g., by being invoked as /bin/bash).

[krader1961]

Also, the declare and local commands only depend on the shell being built with the SHOPT_BASH preprocessor symbol being "true". Whereas other behaviors involving that symbol also depend on the program being invoked as "bash". Personally, I think those two aliases for typeset should be enabled unconditionally. Whether that can be done in the post ksh93u+ release is an open question. The AST team seemed to think the answer is "yes" since they made that behavior the default in the ksh93v- release whereas in the ksh93u+ release the answer was "no".

[krader1961]

I just noticed that /etc/profile on WSL (Windows Subsystem for Linux) assumes the shell supports the local command. I'm going to just enable local and declare. The risk of causing problems for existing ksh scripts seems so negligible that it isn't worth worrying about. Clearly the AST team felt the same since they intended ksh be built with SHOPT_BASH set true.

[dannyweldon]

I'm going to just enable local and declare.

Sounds good to me. One thing to be aware of though is that in bash, local will only work inside a function. Using it outside a function gives this error:

bash: local: can only be used in a function

Having a local builtin which checks this before calling typedef would make it more compatible, but whether the behaviour of bash is conceptually correct or desirable is another matter. I just checked and dash does the same thing, but zsh 5.1.1 doesn't seem to care, not that it cares for standards.

[krader1961]

The AST team who implemented local already ensured it is only usable inside a function:

ast/src/cmd/ksh93/bltins/typeset.c

Lines 409 to 414 in da398b6

	#if SHOPT_BASH
	if (local && context->shp->var_base == context->shp->var_tree) {
	errormsg(SH_DICT, ERROR_exit(1), "local can only be used in a function");
	__builtin_unreachable();
	}
	#endif // SHOPT_BASH

KSH PROMPT:1: local wtf
src/cmd/ksh93/ksh: local: local can only be used in a function

Whether or not that test is 100% correct is TBD.

[krader1961]

I just noticed a bug in the existing code. Notice this definition:

ast/src/cmd/ksh93/include/builtins.h

Lines 43 to 47 in 014e908

	#if SHOPT_BASH
	#define SYSLOCAL (shgd->bltin_cmds + 17)
	#else
	#define SYSLOCAL NULL
	#endif

That corresponds to an offset in shtab_builtins[]. The problem is that declare is at offset 17 and local is at offset 18 in that table:

ast/src/cmd/ksh93/data/builtins.c

Lines 51 to 72 in 014e908

	const struct shtable3 shtab_builtins[] = {
	{"login", NV_BLTIN | BLT_ENV | BLT_SPC, Bltin(login)},
	{"exec", NV_BLTIN | BLT_ENV | BLT_SPC, bltin(exec)},
	{"set", NV_BLTIN | BLT_ENV | BLT_SPC, bltin(set)},
	{":", NV_BLTIN | BLT_ENV | BLT_SPC, bltin(true)},
	{"true", NV_BLTIN | BLT_ENV, bltin(true)},
	{"command", NV_BLTIN | BLT_ENV | BLT_EXIT, bltin(command)},
	{"cd", NV_BLTIN | BLT_ENV, bltin(cd)},
	{"break", NV_BLTIN | BLT_ENV | BLT_SPC, bltin(break)},
	{"continue", NV_BLTIN | BLT_ENV | BLT_SPC, bltin(break)},
	{"typeset", NV_BLTIN | BLT_ENV | BLT_SPC | BLT_DCL, bltin(typeset)},
	{"test", NV_BLTIN | BLT_ENV, bltin(test)},
	{"[", NV_BLTIN | BLT_ENV, bltin(test)},
	{"let", NV_BLTIN | BLT_ENV, bltin(let)},
	{"export", NV_BLTIN | BLT_ENV | BLT_SPC | BLT_DCL, bltin(readonly)},
	{".", NV_BLTIN | BLT_ENV | BLT_SPC, bltin(source)},
	{"return", NV_BLTIN | BLT_ENV | BLT_SPC, bltin(return )},
	{"enum", NV_BLTIN | BLT_ENV | BLT_SPC | BLT_DCL, bltin(enum)},
	#if SHOPT_BASH
	{"declare", NV_BLTIN | BLT_ENV | BLT_SPC | BLT_DCL, bltin(typeset)},
	{"local", NV_BLTIN | BLT_ENV | BLT_DCL, bltin(typeset)},
	#endif // SHOPT_BASH

SYSLOCAL is only used two places. It's not clear what the ramifications are of this bug.