Improve shell builtin exit codes

[tucak]

I noticed that some builtins behave weirdly when failing, this should make the situation a bit more consistent.

[mgree]

Can you elaborate on 'weirdly'? It's tough to see what the concrete changes are just from the PR diff.

[tucak]

Of course. I noticed two classes of errors:

1. Some builtins wouldn't set their own exit code at all, they are "leaking" the previous exit code.
2. Some bulltins in certain cases would always exit with 0, even if there was an I/O error (e.g., echo This will fail. > /dev/full), they "silently fail".

I've included a test in the commit, tests/shell/builtin.exitcode, that tests these problems, although it is not exhaustive. Without the patch, it fails and the output is:

Leaking commands:
export
readonly
local
times
set
trap
hash
type
command
Silently failing commands:
pwd
echo
umask
history
alias
alias foo
alias foo baz
kill -l
kill -l 1
help builtins
help version
help trace

With the patch, the output is:

Leaking commands:
Silently failing commands:

dash passes this test too, but other shells have some builtins that silently fail. mksh seems to be particularly careless.

[mgree]

fc probably shouldn't be in this list, as it only really has meaning in an interactive shell. (See https://pubs.opengroup.org/onlinepubs/9699919799/utilities/fc.html) It's harmless to have it for now, though.

[mgree]

The good news is that this all looks fine. The bad news is that it isn't passing the POSIX test suite. The still worse news is that neither are old, known-good builds.

There's some bug that's breaking my setup. I'd wager it's in the Docker toolchain or the Debian 9 image I'm using.

In any case, I'm going to try to debug the issue before I merge this patch (or #15). Sorry for the delay, and I'm very glad you submitted these... thank you!

[tucak]

I noticed that tests/shell/builtin.exitcode.test is really slow. Turns out it is because help trace concatenates lots of strings using Lem's string concatenation routine, which recursively concatenates, allocating a new string in every call, only to throw it away in the next. With OCaml's String.concat it is almost instantaneous. Would you accept a change that tells Lem to use the OCaml native concat, placed in smoosh_prelude.lem, or is it too much of a hack?

[mgree]

That's probably too much of a hack for me... but the Lem library has a concat function in string.lem. There's no extraction hints, but they'd be easy enough to add there.

Another option would be to put the help trace very early in your tests, so the trace is shorter. :)

[tucak]

I added a commit that makes command -v and -V check if the command file actually exists. It does not check if it is actually executable (bash does, dash does not). Without this smoosh will happily announce that a command specified by an absolute path was found, even if it does not exists.

I couldn't find anything in the standard that would make this required or forbid it, but Modernish uses it and it seems useful.

[mgree]

Oh, I think it's implicit in "found using the PATH variable" in https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html. [https://github.com/mgree/smoosh/blob/master/BUG.command.-v.builtin.path.test] is about this behavior.

[tucak]

Right, but this is for when it is given as an absolute path. Example:

$ command -V /does/not/exist
/does/not/exist is /does/not/exist

now becomes

$ command -V /does/not/exist
/does/not/exist: not found

[mgree]

Ahhh. I see. The command -v entry is clear about "command_names including a character" (which covers both explicitly relative paths like ./ and ../ as well as absolute paths) but the command -V entry doesn't mention it.

I imagine that -v and -V are almost always implemented with a single resolution phase and two different rendering phases, and so -V will inherit the same treatment of paths. Here's what I get on the shells I have locally:

~/smoosh/tests/cmp$ ./test.sh 'command -V /none/such'
      smoosh: OUT [/none/such is /none/such] ERR [] EC [0]
  bash-posix: OUT [] ERR [bash: line 0: command: /none/such: not found] EC [1]
        bash: OUT [] ERR [bash: line 0: command: /none/such: not found] EC [1]
        dash: OUT [/none/such: not found] ERR [] EC [127]
   zsh-posix: OUT [/none/such not found] ERR [] EC [1]
         zsh: OUT [/none/such not found] ERR [] EC [1]
         osh: OUT [] ERR [  command -V /none/such
          ^~
[ -c flag ]:1: 'command' doesn't accept flag '-V'] EC [2]
        mksh: OUT [/none/such not found] ERR [] EC [1]
         ksh: OUT [] ERR [ksh: command: /none/such: not found] EC [1]
  yash-posix: OUT [] ERR [command: no such command `/none/such'] EC [1]
        yash: OUT [] ERR [command: no such command `/none/such'] EC [1]

[tucak]

Another small change for command, as the standard says that the paths "shall be written as absolute pathnames".