Emulate the qurirky errexit behavior of bash and busybox ash.

This is necessary to run abuild. errexit is no respected in command subs, and "bare" assignments vs keyword assignments are treated differently. Add a 'set -o strict-errexit' option to restore the more consistent and rational OSH behavior. Add many spec test cases to show the differences. Addreses issue #61.
oilshell · Jan 9, 2018 · ba772f081056beb08fd25d8db4190076f25f20b2 · ba772f0
1 parent 7ce594a
commit ba772f081056beb08fd25d8db4190076f25f20b2
diff --git a/core/cmd_exec.py b/core/cmd_exec.py
@@ -148,6 +148,7 @@ def __init__(self, mem, status_lines, funcs, completion, comp_lookup,
    self.loop_level = 0  # for detecting bad top-level break/continue

    self.tracer = Tracer(exec_opts, mem, self.word_ev)
+    self.check_command_sub_status = False  # a hack

  def _Complete(self, argv):
    """complete builtin - register a completion function.
@@ -356,15 +357,24 @@ def _PopErrExit(self):
    self.exec_opts.errexit.Pop()

  def _CheckStatus(self, status, node, argv0=None):
+    """ErrExitFailure with location info attached."""
    if self.exec_opts.ErrExit() and status != 0:
      # Add context based on node type
      if node.tag == command_e.SimpleCommand:
        argv0 = argv0 or '<unknown>'
-        e_die('[%d] %r command exited with status %d', os.getpid(), argv0,
-              status, word=node.words[0], status=status)
+        raise util.ErrExitFailure(
+            '[%d] %r command exited with status %d', os.getpid(), argv0,
+            status, word=node.words[0], status=status)
+      elif node.tag == command_e.Assignment:
+        span_id = self._SpanIdForAssignment(node)
+        raise util.ErrExitFailure(
+            '[%d] assignment exited with status %d', os.getpid(), 
+            status, span_id=span_id, status=status)
+
      else:
-        e_die('[%d] %r exited with status %d', os.getpid(),
-              node.__class__.__name__, status, status=status)
+        raise util.ErrExitFailure(
+            '[%d] %r exited with status %d', os.getpid(),
+            node.__class__.__name__, status, status=status)

  def _EvalLhs(self, node, spid):
    """lhs_expr -> lvalue."""
@@ -462,7 +472,7 @@ def _EvalRedirects(self, node):
      redirects.append(r)
    return redirects

-  def _MakeProcess(self, node, job_state=None):
+  def _MakeProcess(self, node, job_state=None, disable_errexit=False):
    """
    Assume we will run the node in another process.  Return a process.
    """
@@ -472,8 +482,8 @@ def _MakeProcess(self, node, job_state=None):
      # - continue | less
      # - ( return )
      # NOTE: This could be done at parse time too.
-      e_die('Invalid control flow %r in pipeline / subshell / background', node.token.val,
-            token=node.token)
+      e_die('Invalid control flow %r in pipeline / subshell / background',
+            node.token.val, token=node.token)

    # NOTE: If ErrExit(), we could be verbose about subprogram errors?  This
    # only really matters when executing 'exit 42', because the child shell
@@ -483,7 +493,8 @@ def _MakeProcess(self, node, job_state=None):
    # interleaved.
    # - We could return the `exit` builtin into a FatalRuntimeError exception
    # and get this check for "free".
-    thunk = process.SubProgramThunk(self, node)
+    thunk = process.SubProgramThunk(self, node,
+                                    disable_errexit=disable_errexit)
    p = process.Process(thunk, job_state=job_state)
    return p

@@ -600,7 +611,18 @@ def _SetSourceLocation(self, span_id):
    source_name, line_num = self.arena.GetDebugInfo(line_id)
    self.mem.SetSourceLocation(source_name, line_num)

+  # TODO: Also change to BareAssign (set global or mutate local) and
+  # KeywordAssign.  The latter may have flags too.
+  def _SpanIdForAssignment(self, node):
+    # TODO: Share with tracing (SetSourceLocation) and _CheckStatus
+    return node.spids[0]
+
  def _Dispatch(self, node, fork_external):
+    # If we call RunCommandSub in a recursive call to the executor, this will
+    # be set true (if strict-errexit is false).  But it only lasts for one
+    # command.
+    self.check_command_sub_status = False
+
    argv0 = None  # for error message
    check_errexit = False  # for errexit

@@ -710,7 +732,6 @@ def _Dispatch(self, node, fork_external):
      status = 0 if i != 0 else 1

    elif node.tag == command_e.Assignment:
-      check_errexit = True
      pairs = []
      if node.keyword == Id.Assign_Local:
        lookup_mode = scope_e.LocalOnly
@@ -775,8 +796,31 @@ def _Dispatch(self, node, fork_external):
          self.mem.SetSourceLocation('<unknown>', -1)
        self.tracer.OnAssignment(lval, val, flags, lookup_mode)

-      # TODO: This should be eval of RHS, unlike bash!
-      status = 0
+      # PATCH to be compatible with existing shells: If the assignment had a
+      # command sub like:
+      #
+      # s=$(echo one; false)
+      #
+      # then its status will be in mem.last_status, and we can check it here.
+      # If there was NOT a command sub in the assignment, then we don't want to
+      # check it.
+      if node.keyword == Id.Assign_None:  # mutate existing local or global
+        # Only do this if there was a command sub?  How?  Look at node?
+        # Set a flag in mem?   self.mem.last_status or
+        if self.check_command_sub_status:
+          self._CheckStatus(self.mem.last_status, node)
+          # A global assignment shouldn't clear $?.
+          status = self.mem.last_status
+        else:
+          status = 0
+      else:
+        # To be compatible with existing shells, local assignments DO clear
+        # $?.  Even in strict mode, we don't need to bother setting
+        # check_errexit = True, because we would have already checked the
+        # command sub in RunCommandSub.
+        status = 0
+        # TODO: maybe we should have a "sane-status" that respects this:
+        # false; echo $?; local f=x; echo $?

    elif node.tag == command_e.ControlFlow:
      if node.arg_word:  # Evaluate the argument
@@ -1025,6 +1069,7 @@ def _Execute(self, node, fork_external=True):
      redirects = self._EvalRedirects(node)

    check_errexit = True
+
    if redirects is not None:
      assert isinstance(redirects, list), redirects
      if self.fd_state.Push(redirects, self.waiter):
@@ -1083,7 +1128,8 @@ def Execute(self, node, fork_external=True):
    return status

  def RunCommandSub(self, node):
-    p = self._MakeProcess(node)
+    p = self._MakeProcess(node,
+                          disable_errexit=not self.exec_opts.strict_errexit)

    r, w = os.pipe()
    p.AddStateChange(process.StdoutToPipe(r, w))
@@ -1102,10 +1148,22 @@ def RunCommandSub(self, node):

    status = p.WaitUntilDone(self.waiter)

-    # TODO: Add context
-    if self.exec_opts.ErrExit() and status != 0:
-      e_die('Command sub exited with status %d (%r)', status,
+    # OSH has the concept of aborting in the middle of a WORD.  We're not
+    # waiting until the command is over!
+    if self.exec_opts.strict_errexit:
+      if self.exec_opts.ErrExit() and status != 0:
+        raise util.ErrExitFailure(
+            'Command sub exited with status %d (%r)', status,
            node.__class__.__name__)
+    else:
+      # Set a flag so we check errexit at the same time as bash.  Example:
+      #
+      # a=$(false)
+      # echo foo  # no matter what comes here, the flag is reset
+      #
+      # Set ONLY until this command node has finished executing.
+      self.check_command_sub_status = True
+      self.mem.last_status = status

    # Runtime errors test case: # $("echo foo > $@")
    # Why rstrip()?
@@ -1157,8 +1215,6 @@ def RunProcessSub(self, node, op_id):
    # Fork, letting the child inherit the pipe file descriptors.
    pid = p.Start()

-    # TODO: Set $!
-
    # After forking, close the end of the pipe we're not using.
    if op_id == Id.Left_ProcSubIn:
      os.close(w)
@@ -1171,6 +1227,9 @@ def RunProcessSub(self, node, op_id):
    #log('Process sub started %d', pid)
    self.waiter.Register(pid, p.WhenDone)

+    # NOTE: Like bash, we never actually wait on it!
+    # TODO: At least set $! ?
+
    # Is /dev Linux-specific?
    if op_id == Id.Left_ProcSubIn:
      return '/dev/fd/%d' % r
diff --git a/core/process.py b/core/process.py
@@ -292,12 +292,15 @@ def Run(self):
 class SubProgramThunk:
  """A subprogram that can be executed in another process."""

-  def __init__(self, ex, node):
+  def __init__(self, ex, node, disable_errexit=False):
    self.ex = ex
    self.node = node
+    self.disable_errexit = disable_errexit  # for bash errexit compatibility

  def Run(self):
    # NOTE: may NOT return due to exec().
+    if self.disable_errexit:
+      self.ex.exec_opts.errexit.Disable()
    status = self.ex.Execute(self.node, fork_external=False)
    sys.exit(status)  # Must exit!

@@ -311,12 +314,6 @@ def __init__(self, w, body_str):
    self.w = w
    self.body_str = body_str

-  #def RunInParent(self):
-  #  byte_str = self.body_str.encode('utf-8')
-  #  os.write(self.w, byte_str)
-  #  # Don't bother to close, since the process will die
-  #  #os.close(self.w)
-
  def Run(self):
    """
    do_exit: For small pipelines
diff --git a/core/state.py b/core/state.py
@@ -58,12 +58,17 @@ def Pop(self):
    self.errexit = self.stack.pop()

  def Set(self, b):
+    """User code calls this."""
    if True in self.stack:  # are we in a temporary state?
      # TODO: Add error context.
      e_die("Can't set 'errexit' in a context where it's disabled "
            "(if, !, && ||, while/until conditions)")
    self.errexit = b

+  def Disable(self):
+    """For bash compatibility in command sub."""
+    self.errexit = False
+

 # Used by builtin
 SET_OPTIONS = [
@@ -79,6 +84,7 @@ def Set(self, b):
    (None, 'debug-completion'),

    (None, 'strict-control-flow'),
+    (None, 'strict-errexit'),
 ]

 _SET_OPTION_NAMES = set(name for _, name in SET_OPTIONS)
@@ -101,30 +107,46 @@ def __init__(self, mem):
    self.noglob = False  # -f
    self.noexec = False  # -n
    self.noclobber = False  # -C
-
    # We don't do anything with this yet.  But Aboriginal calls 'set +h'.
    self.hashall = True  # -h is true by default.

+    # OSH-specific options.
    self.debug_completion = False
    self.strict_control_flow = False

+    # strict_errexit makes 'local foo=$(false)' and echo $(false) fail.
+    # By default, we have mimic bash's undesirable behavior of ignoring
+    # these failures, since ash copied it, and Alpine's abuild relies on it.
+    #
+    # bash 4.4 also has shopt -s inherit_errexit, which says that command subs
+    # inherit the value of errexit.  # I don't believe it is strict enough --
+    # local still needs to fail.
+    self.strict_errexit = False
+
+    # This comes after all the 'set' options.
    shellopts = self.mem.GetVar('SHELLOPTS')
    assert shellopts.tag == value_e.Str, shellopts
    self._InitOptionsFromEnv(shellopts.s)

-    # shopt -s / -u
+    # shopt -s / -u.  NOTE: bash uses $BASHOPTS rather than $SHELLOPTS for these.
    self.nullglob = False 
    self.failglob = False 

-    # OSH-specific
+    #
+    # OSH-specific options that are not yet implemented.
+    #
+
    self.strict_arith = False  # e.g. $(( x )) where x doesn't look like integer
    self.strict_array = False  # ${a} not ${a[0]}, require double quotes, etc.
-    self.strict_command = False  # break at top level.
+                               # compare array for equality?  Or just use a
+                               # different syntax.
    self.strict_word = False  # word splitting, etc.
    self.strict_scope = False  # disable dynamic scope
-
    # TODO: strict_bool.  Some of this is covered by arithmetic, e.g. -eq.

+    # Don't need flags -e and -n.  -e is $'\n', and -n is write.
+    self.sane_echo = False
+
  def _InitOptionsFromEnv(self, shellopts):
    # e.g. errexit:nounset:pipefail
    lookup = set(shellopts.split(':'))
diff --git a/core/util.py b/core/util.py
@@ -81,6 +81,14 @@ class FatalRuntimeError(_ErrorWithLocation):
  pass


+class ErrExitFailure(FatalRuntimeError):
+  """For set -e.
+  
+  Travels between WordEvaluator and Executor.
+  """
+  pass
+
+
 def p_die(msg, *args, **kwargs):
  """Convenience wrapper for parse errors."""
  raise ParseError(msg, *args, **kwargs)
diff --git a/core/word_compile.py b/core/word_compile.py
@@ -67,12 +67,7 @@ def EvalCStringToken(id_, value):
    i = int(s, 16)
    return chr(i)

-  elif id_ == Id.Char_Unicode4:
-    s = value[2:]
-    i = int(s, 16)
-    return unichr(i)
-
-  elif id_ == Id.Char_Unicode8:
+  elif id_ in (Id.Char_Unicode4, Id.Char_Unicode8):
    s = value[2:]
    i = int(s, 16)
    return unichr(i)
diff --git a/spec/command-sub.test.sh b/spec/command-sub.test.sh
@@ -77,3 +77,30 @@ argv "$s"
 s=$(python -c 'print "ab\ncd\n "')
 argv "$s"
 # stdout: ['ab\ncd\n ']
+
+### Command Sub and exit code
+# A command resets the exit code, but an assignment doesn't.
+echo $(echo x; exit 33)
+echo $?
+x=$(echo x; exit 33)
+echo $?
+## STDOUT:
+x
+0
+33
+## END
+
+### Command Sub in local sets exit code
+# A command resets the exit code, but an assignment doesn't.
+f() {
+  echo $(echo x; exit 33)
+  echo $?
+  local x=$(echo x; exit 33)
+  echo $?
+}
+f
+## STDOUT:
+x
+0
+0
+## END