[errexit] Fix crash due to confusion between shopt and POSIX disabling

Found by *A Tour of Oil*

Now we use a "parallel stack representation" for
self.disabled_errexit_spid and the errexit option stack

core/executor.py

@@ -209,14 +209,15 @@ def RunSimpleCommand(self, cmd_val, do_fork, call_procs=True):
     if call_procs:
       proc_node = self.procs.get(arg0)
       if proc_node is not None:
-        if (self.exec_opts.strict_errexit() and 
-            self.mutable_opts.ErrExitIsDisabled()):
-          self.errfmt.Print_('errexit was disabled for this construct',
-                             span_id=self.mutable_opts.ErrExitSpanId())
-          self.errfmt.StderrLine('')
-          e_die("Can't run a proc while errexit is disabled. "
-                "Use 'run' or wrap it in a process with $0 myproc",
-                span_id=span_id)
+        if self.exec_opts.strict_errexit():
+          disabled_spid = self.mutable_opts.ErrExitDisabledSpanId()
+          if disabled_spid != runtime.NO_SPID:
+            self.errfmt.Print_('errexit was disabled for this construct',
+                               span_id=disabled_spid)
+            self.errfmt.StderrLine('')
+            e_die("Can't run a proc while errexit is disabled. "
+                  "Use 'try' or wrap it in a process with $0 myproc",
+                  span_id=span_id)
 
         with dev.ctx_Tracer(self.tracer, 'proc', argv):
           # NOTE: Functions could call 'exit 42' directly, etc.

core/oven.py

@@ -525,16 +525,19 @@ def RunSimpleCommand(self, cmd_val, do_fork, call_procs=True):
     if call_procs:
       proc_node = self.procs.get(arg0)
       if proc_node is not None:
-        if (self.exec_opts.strict_errexit() and 
-            self.mutable_opts.ErrExitIsDisabled()):
-          # TODO: make errfmt a member
-          #self.errfmt.Print_('errexit was disabled for this construct',
-          #                   span_id=self.mutable_opts.errexit.spid_stack[0])
-          #stderr_line('')
-          e_die("Can't run a proc while errexit is disabled. "
-                "Use 'catch' or wrap it in a process with $0 myproc",
-                span_id=span_id)
-
+        if self.exec_opts.strict_errexit():
+          disabled_spid = self.mutable_opts.ErrExitDisabledSpanId()
+          if disabled_spid != runtime.NO_SPID:
+            # TODO: make errfmt a member
+            #self.errfmt.Print_('errexit was disabled for this construct',
+            #                   span_id=disabled_spid)
+            #self.errfmt.StderrLine('')
+            e_die("Can't run a proc while errexit is disabled. "
+                  "Use 'try' or wrap it in a process with $0 myproc",
+                  span_id=span_id)
+
+        # TODO: make tracer a member
+        #with dev.ctx_Tracer(self.tracer, 'proc', argv):
         # NOTE: Functions could call 'exit 42' directly, etc.
         status = self.cmd_ev.RunProc(proc_node, argv[1:])
         return status

core/state.py

@@ -160,6 +160,9 @@ def __init__(self, mutable_opts, opt_nums, b):
     # type: (MutableOpts, List[int], bool) -> None
     for opt_num in opt_nums:
       mutable_opts.Push(opt_num, b)
+      if opt_num == option_i.errexit:
+        mutable_opts.errexit_disabled_spid.append(runtime.NO_SPID)  # it wasn't disabled
+
     self.mutable_opts = mutable_opts
     self.opt_nums = opt_nums
 
@@ -170,6 +173,8 @@ def __enter__(self):
   def __exit__(self, type, value, traceback):
     # type: (Any, Any, Any) -> None
     for opt_num in self.opt_nums:  # don't bother to do it in reverse order
+      if opt_num == option_i.errexit:
+        self.mutable_opts.errexit_disabled_spid.pop()
       self.mutable_opts.Pop(opt_num)
 
 
@@ -205,7 +210,6 @@ class ctx_ErrExit(object):
   """
   def __init__(self, mutable_opts, errexit_val, span_id):
     # type: (MutableOpts, bool, int) -> None
-    assert span_id != runtime.NO_SPID
     mutable_opts.PushErrExit(errexit_val, span_id)
     self.mutable_opts = mutable_opts
 
@@ -304,7 +308,7 @@ def __init__(self, mem, opt0_array, opt_stacks, opt_hook):
     self.mem = mem
     self.opt0_array = opt0_array
     self.opt_stacks = opt_stacks
-    self.errexit_spid_stack = []  # type: List[int]
+    self.errexit_disabled_spid = []  # type: List[int]
 
     # Used for 'set -o vi/emacs'
     self.opt_hook = opt_hook
@@ -344,11 +348,16 @@ def Pop(self, opt_num):
   def PushErrExit(self, b, spid):
     # type: (bool, int) -> None
     self.Push(option_i.errexit, b)
-    self.errexit_spid_stack.append(spid)
+    if b:
+      assert spid == runtime.NO_SPID, spid  # was NOT disabled
+    else:
+      assert spid != runtime.NO_SPID, spid  # WAS disabled
+
+    self.errexit_disabled_spid.append(spid)
 
   def PopErrExit(self):
     # type: () -> bool
-    self.errexit_spid_stack.pop()
+    self.errexit_disabled_spid.pop()
     return self.Pop(option_i.errexit)
 
   def PushDynamicScope(self, b):
@@ -432,27 +441,34 @@ def DisableErrExit(self):
     # type: () -> None
     self._Set(option_i.errexit, False)
 
-  def ErrExitIsDisabled(self):
-    # type: () -> bool
+  def ErrExitDisabledSpanId(self):
+    # type: () -> int
+    """If errexit is disabled by POSIX rules, return span ID for construct.
+
+    e.g. the spid for 'if' or '&&' etc.
+
+    Otherwise return runtime.NO_SPID
+    """
 
     # Bug fix: The errexit disabling inherently follows a STACK DISCIPLINE.
     # But we run trap handlers in the MAIN LOOP, which break this.  So just
-    # declare that it's never disabled it in a trap.
+    # declare that it's never disabled in a trap.
     if self.Get(option_i._running_trap):
-      return False
+      return runtime.NO_SPID
 
-    # Bottom of stack: true
-    # Top of stack: false
     overlay = self.opt_stacks[option_i.errexit]
+    if 0:
+      log('overlay %s', overlay)
+      log('errexit_disabled_spid %s', self.errexit_disabled_spid)
     if overlay is None or len(overlay) == 0:
-      return False
+      return runtime.NO_SPID
     else:
-      # 'catch' will make the top of the stack true
-      return self.opt0_array[option_i.errexit] and not overlay[-1]
-
-  def ErrExitSpanId(self):
-    # type: () -> int
-    return self.errexit_spid_stack[0]
+      was_on = self.opt0_array[option_i.errexit] or (True in overlay)
+      # top of stack == False means it's disabled
+      if was_on and not overlay[-1]:
+        return self.errexit_disabled_spid[-1]
+      else:
+        return runtime.NO_SPID
 
   def _SetOption(self, opt_name, b):
     # type: (str, bool) -> None

doc/oil-language-tour.md

@@ -592,10 +592,10 @@ Some builtins take blocks as arguments:
     }
 
     # TODO: fix crash
-    #shopt --unset errexit {
-    #  mycopy x y  # ignore errors
-    #  mycopy y z  # ignore errors
-    #}
+    shopt --unset errexit {
+      mycopy x y  # ignore errors
+      mycopy y z  # ignore errors
+    }
 
 Procs can also take blocks: TODO.
 

osh/builtin_meta.py

@@ -274,9 +274,10 @@ def Run(self, cmd_val):
     # Set in the 'except' block, e.g. if 'myfunc' failed
     failure_spid = runtime.NO_SPID
     try:
-      # Temporarily turn ON errexit, and blame the 'run' spid.  Note that
-      # 'if run myproc' disables it and then enables it!
-      with state.ctx_ErrExit(self.mutable_opts, True, cmd_val.arg_spids[0]):
+      # Temporarily turn ON errexit, but don't pass a SPID because we're
+      # ENABLING and not disabling.  Note that 'if try myproc' disables it and
+      # then enables it!
+      with state.ctx_ErrExit(self.mutable_opts, True, runtime.NO_SPID):
         # Pass do_fork=True.  Slight annoyance: the real value is a field of
         # command.Simple().  See _NoForkLast() in CommandEvaluator We have an
         # extra fork (miss out on an optimization) of code like ( status ls )

osh/cmd_eval.py

@@ -502,7 +502,7 @@ def _StrictErrExit(self, node):
     if _HasManyStatuses(node):
       node_str = ui.CommandType(node)
       e_die("strict_errexit only allows simple commands (got %s). "
-            "Hint: use 'run'.",
+            "Hint: use 'try'.",
             node_str, span_id=location.SpanForCommand(node))
 
   def _StrictErrExitList(self, node_list):

spec/errexit-oil.test.sh

@@ -651,3 +651,25 @@ cat tmp
 2
 tmp_contents
 ## END
+
+#### Regression
+case $SH in (bash|dash|ash|mksh) exit ;; esac
+
+shopt --set oil:basic
+
+shopt --unset errexit {
+  echo hi
+}
+
+proc p {
+  echo p
+}
+
+shopt --unset errexit {
+  p
+}
+## STDOUT:
+hi
+p
+## END
+## N-I bash/dash/ash/mksh stdout-json: ""

test/runtime-errors.sh

@@ -159,6 +159,11 @@ strict_errexit_1() {
   _strict-errexit-case 'if { echo 1; echo 2; }; then echo IF; fi'
   _strict-errexit-case 'while { echo 1; echo 2; }; do echo WHILE; done'
   _strict-errexit-case 'until { echo 1; echo 2; }; do echo UNTIL; done'
+
+  # Must be separate lines for parsing options to take effect
+  _strict-errexit-case 'shopt -s oil:basic
+                        proc p { echo p }
+                        if p { echo hi }'
 }
 
 # OLD WAY OF BLAMING