Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write "the ultimate guide to errexit" #709

Closed
andychu opened this issue Apr 15, 2020 · 18 comments
Closed

Write "the ultimate guide to errexit" #709

andychu opened this issue Apr 15, 2020 · 18 comments

Comments

@andychu
Copy link
Contributor

andychu commented Apr 15, 2020

This is going to take a lot of time, but I think it should be done because a lot of these threads are like "groundhog day"

https://lobste.rs/s/ajoaje/first_two_statements_your_bash_script

e.g. discovering -e for the first time, then seeing all the problems in the Wooledge Wiki, etc.

https://mywiki.wooledge.org/BashFAQ/105

Threads:

https://oilshell.zulipchat.com/#narrow/stream/121540-oil-discuss/topic/Things.20that.20surprised.20me.20about.20shell

https://oilshell.zulipchat.com/#narrow/stream/121540-oil-discuss/topic/more.20errexit.20links

related: #476

@andychu
Copy link
Contributor Author

andychu commented Apr 15, 2020

Off the top of my head, here are the problems:

  1. The confusing rule that errexit is disabled with if, || && ! (it's in POSIX and all shells implement it)
  2. The local x=$(false) vs. x=$(false) problem
    • more_errexit in Oil fixes this
  3. The bash specific command sub problem. x=$(false). I think inherit_errexit fixes this.

Related: set -o pipefail, SIGPIPE, etc.

@andychu
Copy link
Contributor Author

andychu commented Apr 15, 2020

A very related problem is that the $? exit status is pretty confusing itself.

e.g. for command subs and process subs.


Repeating crazily comprehensive links from Zulip:

https://www.fvue.nl/wiki/Bash:_Error_handling

https://www.in-ulm.de/~mascheck/various/set-e/

andychu pushed a commit that referenced this issue Apr 15, 2020
- singleton command sub
- command sub is in another process.  (Is there a way around this?)
  - I think we could detect it statically, not dynamically?

Related to issue #476.  And #709.

Unrelated: Restore assertions in glob code
@andychu
Copy link
Contributor Author

andychu commented Apr 16, 2020

#474 is also very related.

  • grep can return 0 1 or 2. ditto for test
  • SIGPIPE is an issue

andychu pushed a commit that referenced this issue Apr 16, 2020
There are 4 major problems, which are mostly addressed.  And a few minor
ones.

Addresses issue #709.
@Crestwave
Copy link
Contributor

I'm not sure if this is really problematic, but one interesting gotcha I encountered today was something to the effect of:

set -e
f() { (( var )) && echo foo; }
f
echo bar

This doesn't print anything if var evaluates to 0 because even though errexit was disabled for (( var )), it defined the function's return code since it was the last command executed. And since it is enabled in the caller, it then aborts.

@o11c
Copy link

o11c commented May 10, 2020

Note also that set -E (errtrace) is still completely unknown to osh. This forced me to add some || is-oil to get my script to run at all (and unsafely at that).

Whereas trap 'blah' ERR is just a warning.

@andychu
Copy link
Contributor Author

andychu commented May 10, 2020

OK I haven't used errtrace, but it sounds like it's in scope. Do you have an example of your usage?

Generally speaking Oil development is very test driven, and I accept PRs with failing spec tests if they pass with other shells (including bash).

Example for ble.sh features:

https://www.oilshell.org/release/0.8.pre4/test/spec.wwz/ble-idioms.html

Either way some examples would help. I generally prioritize features based on "real" usage, since there is a deep well of bash features to go through...

@andychu
Copy link
Contributor Author

andychu commented May 10, 2020

@Crestwave

Sorry for the late response. I think there are two problems there, and I think we should address them. I mentioned (( )) here, and the && problem is well known.

https://oilshell.zulipchat.com/#narrow/stream/121540-oil-discuss/topic/Ultimate.20errexit.20guide/near/194360428

The exit code of (( )) is problematic and I'm not sure it can be fixed... It is another case of confusing booleans and success/fail, e.g. (( a = 0 )) vs. if (( a == b )).

Oil has a different expression language that doesn't have that confusion and has more types besides integers. But I think && will be fixed by unconditionally causing a fatal runtime error when it's confusing, so maybe that will include (( )) on the left.

@Crestwave
Copy link
Contributor

Crestwave commented May 10, 2020

The main thing I found interesting was that it exited even though && disabled it because it defined the function's return code. Which makes sense given the mechanics of errexit, but I just didn't consider it before.

@andychu
Copy link
Contributor Author

andychu commented May 10, 2020

Yeah I have run into this before and been confused, and it's mentioned on Wooledge's wiki. It happens on code as simple as:

set -e

f()  {
  test -f foo && echo "foo exists"
}

f   # fails here!
echo "I expected to get here, but no"

That is better written:

set -e

f() {
  test -f foo
  echo "foo exists"
}

Or use if test -f foo.

Basically && is useless and confusing when set -e is on. So I think that shopt -s strict_errexit should simply disallow && that's not in a condition?

if test -f foo && test -f bar; then ...     # allowed

test -f foo && ...  # not allowed

@andychu
Copy link
Contributor Author

andychu commented May 10, 2020

FWIW I changed my style to always use if statements because of this (because I always have set -e on.)

I don't use the test x && foo idiom at all. But we shouldn't have that "footgun" there waiting for other people to step on it...

@o11c
Copy link

o11c commented May 10, 2020

Frankly, I don't understand how my code works anymore. I do know that:

  • every line was mandatory at some point during testing
  • if I mess up anywhere else in the script, I get useful info, and the script stops.

My startup code looks like:

set -e
set -E

trap 'echo "$BASH_SOURCE:$LINENO: error: failure during early startup! Details unavailable."' ERR

magic_exitvalue=$(($(kill -l CONT)+128))

backtrace()
{
    {
        local status=$?
        if [ "$status" -eq "$magic_exitvalue" ]
        then
            echo '(omit backtrace)'
            exit "$magic_exitvalue"
        fi
        local max file line func argc argvi i j
        echo
        echo 'Panic! Something failed unexpectedly.' "(status $status)"
        echo 'While executing' "$BASH_COMMAND"
        echo
        echo Backtrace:
        echo
        max=${#BASH_LINENO[@]}
        let max-- # The top-most frame is "special".
        argvi=${BASH_ARGC[0]}
        for ((i=1;i<max;++i))
        do
            file=${BASH_SOURCE[i]}
            line=${BASH_LINENO[i-1]}
            func=${FUNCNAME[i]}
            argc=${BASH_ARGC[i]}
            printf '%s:%d: ... in %q' "$file" "$line" "$func"
            # BASH_ARGV: ... bar foo ...
            # argvi          ^
            # argvi+argc             ^
            for ((j=argc-1; j>=0; --j))
            do
                printf ' %q' ${BASH_ARGV[argvi+j]}
            done
            let argvi+=argc || true
            printf '\n'
        done

        if true
        then
            file=${BASH_SOURCE[i]}
            line=${BASH_LINENO[i-1]}
            printf '%s:%d: ... at top level\n' "$file" "$line"
        fi
    } >&2
    exit "$magic_exitvalue"
    unreachable
}
shopt -s extdebug
trap 'backtrace' ERR

# main code, involving many functions, follows.

@andychu
Copy link
Contributor Author

andychu commented May 10, 2020

OK thanks, this looks a lot like #708 . It's probably in scope but I don't know all the details of how it works either....

@andychu
Copy link
Contributor Author

andychu commented Oct 10, 2020

Comment listing all the problems concisely: https://news.ycombinator.com/item?id=24740842

This issue is still very important! I have some ideas for it -- I think we need a builtin called catch for error checking functions. (Formerly called invoke).

@andychu
Copy link
Contributor Author

andychu commented Oct 10, 2020

Quote from HN:

Wait until you realize

  set -e; (false && true); echo hi

does nothing, but

  set -e;  false && true ; echo hi

prints hi.

This is the same problem as #2, the f() { test -d /tmp && echo "exists" } problem, except with subshells, not with functions.

Good point! This is not solved by catch / invoke... So we still need a solution for this. I thought we would discourage && because it's not necessary.

However something like if test -d / && test -d /tmp still makes sense

@andychu
Copy link
Contributor Author

andychu commented Oct 10, 2020

Refreshing my memory on what we have:

  1. inherit_errexit from bash 4.4. Oil also implements it.

  2. Oil's more_errexit to fix the local problem

  3. Oil's strict_errexit to detect if myfunc or ! myfunc dynamically, and force you to write it as if catch myfunc or ! catch myfunc.

Although I wasn't entirely happy with the dynamic detection here. It still needs another iteration. Problem: one line functions could still be allowed?

The catch (invoke) builtin complements this, but isn't implemented yet. (Note that it can't be implemented as a shell function!)

Now what's the solution to the "&& at the end of function" problem?

I think the best answer so far was that when strict_errexit is on, it will disallow

foo && bar

You can just write it

foo
bar

But it will allow:

if foo && bar {
  echo boolean context
}

I think that makes sense, but we'll have to test it out more to be sure.

Comments appreciated!

@andychu
Copy link
Contributor Author

andychu commented Oct 11, 2020

Important example:

https://news.ycombinator.com/edit?id=24744146

(edit: removed incorrect code)

@andychu andychu changed the title Write "the ultimate guide to errexit" (and fix problems) Write "the ultimate guide to errexit" Jan 11, 2021
@andychu
Copy link
Contributor Author

andychu commented Jan 11, 2021

OK I think everything here is settled, but we just need to document it ...

@andychu andychu removed the essential label Sep 2, 2021
@andychu
Copy link
Contributor Author

andychu commented May 7, 2022

@andychu andychu closed this as completed May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants