disallowing redirects on control flow too strict by default (Run ble.sh)

[danyspin97]

https://github.com/akinomyoga/ble.sh

The following line is not supported by osh: https://github.com/akinomyoga/ble.sh/blob/58e1be46bea16acb487327f61d9cb44fd67651b8/ble.pp#L95

    return 1 2>/dev/null || exit 1
    ^~~~~~
ble.sh:4: Control flow shouldn't have redirects

[andychu]

Ah geez, OK I think bash might spew on return, but Oil doesn't?

I disallowed that because it seems nonsensical -- why would you want to redirect the output of return?

But if bash prints some spew to stderr, that's valid.

So I might need to introduce an option strict_redirect or something like that ... gah. Or just allow it.

Thanks for the report!

[akinomyoga]

But if bash prints some spew to stderr, that's valid.

Bash does print error messages when return is used outside of shell functions or sourced scripts. In such cases, return fails and nothing happens on the control path. Try the following command:

$ bash -c return
bash: line 0: return: can only `return' from a function or sourced script

The line return 1 2>/dev/null || exit 1 in ble.sh is used to perform return when the script is used as a source script and perform exit when the script is directly executed as an independent shell script.

[andychu]

Yes, I remember seeing that, there is a test case for it somewhere. Oil follows other shells and allows return to exit a main program.

So unfortunately Oil may have to provide compatibility with the bash workarounds that aren't necessary in Oil ...

[akinomyoga]

I assume the following sentence in the article "Why Create a New Unix Shell?" is still valid for the current Oil. (Does the "shell script" in the following sentence include (some subset of) Bash scripts?)

The goal is to run existing shell scripts.

I don't think ble.sh could finally be run on Oil (osh) because it is written for Bash and relies on the detailed behavior of Bash. It requires too much efforts to port it because it has more than 24k lines, and I don't think it's worth doing it. I don't know, but nevertheless ble.sh might be some testing example for the compatibility of Oil with Bash.

I have also played with osh. Can I write some points that I noticed in a quick check?

return is builtin in POSIX/bash

Oil seems to handle return in the syntax level because Oil outputs the error messages even if the return is not in the executed control path. This means that return is treated as not a builtin but a keyword in Oil which may conflict with the POSIX requirements.

Even though, Oil reports builtin for type -t return:

$ bin/osh -c 'type -t return'
builtin

Another interesting observation is that return cannot be overridden in Oil as it is handled as a keyword. For example, the result of the following command differs between osh and bash.

$ bash -c 'function return { echo hello; }; return; echo world'
hello
world
$ bin/osh -c 'function return { echo hello; }; return; echo world'
$

Another builtin echo can be overridden by function in Oil. It is not consistent that some builtins are allowed to be overridden, and the others are not (as far as return is considered as a builtin as specified in POSIX). Note that POSIX does not allow neither the overrides of builtins nor the function definition with the same name with a builtin. While, Bash allows both in non-POSIX mode. The script ble.sh, which is written for interactive Bash, overrides several builtin functions to modify their behavior.

builtin return

Although builtin is implemented in Oil, and type indicates that return is a builtin, builtin return fails in Oil. The following error messages explicitly contradicts with POSIX and also the result of type return.

$ bin/osh -c 'builtin echo hello'
hello
$ bin/osh -c 'builtin return'
  builtin return
          ^~~~~~
[ -c flag ]:1: 'return' isn't a shell builtin
$

exit is also builtin

Also I would like to point out that Bash can output error messages even for exit when some background jobs are remaining in interactive sessions. The script ble.sh also uses redirects for the exit builtin for this reason.

[andychu]

Yes that's documented here:

https://www.oilshell.org/release/0.8.pre2/doc/known-differences.html#break-continue-return-are-keywords-not-builtins

I guess I should change type -- it's a bit misleading now.

A few bugs with this label here:

https://github.com/oilshell/oil/labels/divergence

If you see anything else, let me know!

Also related: https://github.com/oilshell/oil/wiki/What-Is-Expected-to-Run-Under-OSH

I haven't looked at ble.sh yet ... 24K lines is very big :) But Oil does run thousands of lines of unmodified shell. Sometimes small patches are required.

A goal is to allow running under both bash and OSH, after slight patches. That is, you might have to modify a few things, but it should run under both interpreters. If that's awkward for any reason, I'm interested. But yes as mentioned interactive scripts are more likely to run into unimplemented features (bind, etc.)

[akinomyoga]

Thank you for the pointers! Actually ble.sh belongs to exactly the second kind of scripts "Interactive ones". For example, it makes abuse of bind that you just mentioned. It's almost impossible to port ble.sh. Maybe only syntax analysis by osh -n on ble.sh and related scripts could give some insights on Oil here. Thank you again!

[andychu]

Wow I'm looking through the ble.sh source and this is very impressive. I will have to try it out because it looks like it does some things with terminals I'm curious about for Oil.

I tried this command to parse all shell files in ble:

for name in */*.sh; do echo $name; osh -n $name | wc -l ;  done

I think it may have found a bug?

keymap/vi.sh
      ((index=bol+ARG-1,index>eol?(index=eol)))
                                             ^
keymap/vi.sh:3815: Parser expected Id.Arith_Colon, got Id.Arith_RParen

That doesn't appear to be valid bash syntax, bash will complain too:

$ bash -c '((index=bol+ARG-1,index>eol?(index=eol)))'
bash: ((: index=bol+ARG-1,index>eol?(index=eol): `:' expected for conditional expression (error token is ")")

The difference is that Oil statically parses arithmetic, whereas in bash does it at runtime (which is also documented in the doc, and I wrote about it on the blog)

[andychu]

Oil doesn't like this part:

        ((_ble_builtin_history_rskip_dict[\$file]+=$2))
                                          ^
src/history.sh:701: Unexpected token in arithmetic context

maybe that can be worked around with dict['$file'] if that's what you meant? Not sure.

The other errors are because Oil doesn't like assignments where the var name is dynamic:

src/canvas.sh
    ((${_prefix}x=_pos[0]))
                 ^
src/canvas.sh:1521: Left-hand side of this assignment is invalid

That could possibly be relaxed since Oil does dynamic assignment for local / declare now, etc.

[andychu]

Anyway I will try it out more later! Looks very cool though. I'm surprised you can do it all in bash :)

[akinomyoga]

I think it may have found a bug?

Thank you very much! This is a shameful bug... It should have been && instead of ?. I fixed it in local and will push it later.

maybe that can be worked around with dict['$file'] if that's what you meant?

Yes. Both \$file and '$file' works the same in this context.

I think I need to check ble.sh with osh -n in more details. Thank you!

[andychu]

I'm surprised there's only 1 bug in 30K+ lines of shell! :) I think this is one of the biggest shell programs I've ever seen, and I've looked for big ones for a number of years, e.g.

https://www.oilshell.org/release/0.8.pre2/test/wild.wwz/

(I guess there are a lot of comments though.)

BTW this parsing feature was one of the first things I wrote about in the blog. However back then Oil probably couldn't understand ble.sh; now it appears to do a decent job.

http://www.oilshell.org/blog/2016/10/13.html

---

I added some failing test cases for dynamic assignment here inspired by ble.sh:

8fdb25d

It's not that big a change to support, but I'm not sure when/if I'll get to it.

I still need to try out ble.sh to see how it works :)

[andychu]

OK back to the original issue, I allowed return 2>&1 by default. shopt -u parse_ignored (which is in the oil:basic group among others) will turn it back into a syntax error.

[andychu]

Note that POSIX does not allow neither the overrides of builtins nor the function definition with the same name with a builtin. While, Bash allows both in non-POSIX mode. The script ble.sh, which is written for interactive Bash, overrides several builtin functions to modify their behavior.

Also I'm interested in any such differences that matter for ble.sh, other than break / continue / return / exit. Those are the only builtins that are turned into keywords as far as I remember.

In other words is there anything else to document here?

https://www.oilshell.org/release/0.8.pre2/doc/known-differences.html#break-continue-return-are-keywords-not-builtins

[akinomyoga]

I'm surprised there's only 1 bug in 30K+ lines of shell!

Thank you. Oh, 24k was my mistake that I forgot to include ble.sh itself when running commands. It was 44k / 32k lines including / excluding comments. Actually I found another bug of ble.sh using osh -n, which was recently added typo. osh -n only reports the first syntax error, so I repeatedly applied osh -n by commenting the reported lines one by one to find all the syntax errors. But it seems those two bugs are everything that can be found by osh -n for the current ble.sh.

By the way, I found several things in which Bash and Oil behaves differently. I'm not sure whether all of them are intended design of Oil or not but seems a little bit long to describe them here. Should I create independent Issues or can I discuss them here?

[andychu]

Yes let's discuss in a new bug! The differences should be documented, or if there are unintended ones they can be fixed.

(we also have a chat at https://oilshell.zulipchat.com/ , but Github is fine too)

[akinomyoga]

Thank you! I pushed fixes of ble.sh found by osh -n (akinomyoga/ble.sh@da6cc47) to the master branch.

Overridable/protected builtins

Also I'm interested in any such differences that matter for ble.sh, other than break / continue / return / exit. Those are the only builtins that are turned into keywords as far as I remember.

ble.sh overrides the builtins bind, exit, read and trap. I found that trap cannot be overridden as well in Oil. I checked which Bash builtin equivalent can be overridden in Oil with the following script:

for b in $(enable | awk '$0=$2'); do
  echo "=== $b ==="
  bin/osh -c "function $b { echo hello; }; $b"
done

I found that, in addition to control flow keywords (exit, break, continue and return), the declare family (declare, export, local, readonly, typeset) and unset, parameter manipulation (set and shift) and other builtins builtin, eval, exec, times, trap, . and : are not allowed to be overridden. I haven't tested Oil specific builtins.

It is interesting to observe that one can override command even though one cannot override builtin. One can override source even though one cannot override .. Is the list of overridable builtins and protected builtins documented somewhere?

mapfile/readarray

By the way, ble.sh uses the builtins mapfile/readarray for efficiency reason if it is available (i.e., ble.sh runs in recent Bash versions). I found they are marked as X in documents, but I think it's worth implementing unless X is a philosophical decision of Oil.

[andychu]

OK I made a new issue for the builtin shadowing. Although I think some of it may relate to "special builtins" as defined by POSIX.

mapfile / readarray are in scope -- most things in bash are. #146 covers that. If it will let us run a new program under Oil I prioritize it:

https://github.com/oilshell/oil/wiki/Shell-Programs-That-Run-Under-OSH

[andychu]

Fixed: http://www.oilshell.org/blog/2020/03/release-0.8.pre3.html