Fix precedence of chains of && and ||.

In command mode, they have EQUAL precedence, unlike inside [[. This bug is tickled by Aboriginal Linux. - Add a gold test and spec test. - Reorganize test/gold.sh a little.
oilshell · Dec 30, 2017 · 100d2e6756dfcce6bfe886a86c8e8cbfdcaa7a02 · 100d2e6
1 parent 77562b4
commit 100d2e6756dfcce6bfe886a86c8e8cbfdcaa7a02
Unified Split

Showing with 122 additions and 66 deletions.

+32 −15 core/cmd_exec.py

+27 −0 gold/and-or.sh

+25 −20 osh/cmd_parse.py

+1 −2 osh/osh.asdl

+17 −0 spec/smoke.test.sh

+19 −28 test/gold.sh

+1 −1 test/spec.sh
diff --git a/core/cmd_exec.py b/core/cmd_exec.py
@@ -815,29 +815,46 @@ def _Dispatch(self, node, fork_external):
      check_errexit = False

    elif node.tag == command_e.AndOr:
-      # TODO: We have to fix && || precedence.  See case #13 in
-      # dbracket.test.sh.
+      # NOTE: && and || have EQUAL precedence in command mode.  See case #13
+      # in dbracket.test.sh.

-      #print(node.children)
-      left, right = node.children
+      left = node.children[0]

-      # This is everything except the last one.
+      # Suppress failure for every child except the last one.
      self._PushErrExit()
      try:
        status = self._Execute(left)
      finally:
        self._PopErrExit()

-      if node.op_id == Id.Op_DPipe:
-        if status != 0:
-          status = self._Execute(right)
-          check_errexit = True  # only check last condition
-      elif node.op_id == Id.Op_DAmp:
-        if status == 0:
-          status = self._Execute(right)
-          check_errexit = True  # only check last condition
-      else:
-        raise AssertionError
+      i = 1
+      n = len(node.children)
+      while i < n:
+        #log('i %d status %d', i, status)
+        child = node.children[i]
+        op_id = node.ops[i-1]
+
+        #log('child %s op_id %s', child, op_id)
+
+        if op_id == Id.Op_DPipe and status == 0:
+          i += 1
+          continue  # short circuit
+
+        elif op_id == Id.Op_DAmp and status != 0:
+          i += 1
+          continue  # short circuit
+
+        if i == n - 1:  # errexit handled differently for last child
+          status = self._Execute(child)
+          check_errexit = True
+        else:
+          self._PushErrExit()
+          try:
+            status = self._Execute(child)
+          finally:
+            self._PopErrExit()
+
+        i += 1

    elif node.tag in (command_e.While, command_e.Until):
      # TODO: Compile this out?
diff --git a/gold/and-or.sh b/gold/and-or.sh
@@ -0,0 +1,27 @@
+#!/bin/bash
+#
+# Test chained and/or.
+
+# From Aboriginal sources/download_functions.sh.
+noversion()
+{
+  LOGRUS='s/-*\(\([0-9\.]\)*\([_-]rc\)*\(-pre\)*\([0-9][a-zA-Z]\)*\)*\(\.tar\(\..z2*\)*\)$'
+  [ -z "$2" ] && LOGRUS="$LOGRUS//" || LOGRUS="$LOGRUS/$2\\6/"
+
+  echo "$1" | sed -e "$LOGRUS"
+}
+
+
+# Simplified version.
+simple() {
+  [ -z "$1" ] && LOGRUS="yes" || LOGRUS="no"
+}
+
+test-simple() {
+  simple 1
+  echo $LOGRUS
+  simple ''
+  echo $LOGRUS
+}
+
+"$@"
diff --git a/osh/cmd_parse.py b/osh/cmd_parse.py
@@ -1329,35 +1329,40 @@ def ParsePipeline(self):

  def ParseAndOr(self):
    """
-    and_or           : pipeline (( AND_IF | OR_IF ) newline_ok pipeline)* ;
-
-    Left recursive / associative:
-
    and_or           : and_or ( AND_IF | OR_IF ) newline_ok pipeline
                     | pipeline

-    TODO: Make it left recursive -- results are wrong otherwise.  I guess you
-    have to do it iteratively, and add operators?  It's similar to the  | and
-    |& issue.
+    Note that it is left recursive and left associative.  We parse it
+    iteratively with a token of lookahead.
    """
-    left = self.ParsePipeline()
-    if not left: return None
+    child = self.ParsePipeline()
+    if not child: return None

-    if not self._Peek(): return None  # because ParsePipeline need not _Peek()
+    if not self._Peek(): return None
    if self.c_id not in (Id.Op_DPipe, Id.Op_DAmp):
-      return left
-    op = self.c_id
-    self._Next()  # Skip past operator
+      return child

-    if not self._NewlineOk(): return None
+    ops = []
+    children = [child]
+
+    while True:
+      ops.append(self.c_id)
+
+      self._Next()  # skip past || &&

-    right = self.ParseAndOr()
-    if not right: return None
+      if not self._NewlineOk():
+        return None
+
+      child = self.ParsePipeline()
+      if not child: return None
+
+      children.append(child)
+
+      if not self._Peek(): return None
+      if self.c_id not in (Id.Op_DPipe, Id.Op_DAmp):
+        break

-    node = ast.AndOr()
-    node.children = [left, right]
-    #node.ops = [op]
-    node.op_id = op
+    node = ast.AndOr(ops, children)
    return node

  def ParseCommandLine(self):
diff --git a/osh/osh.asdl b/osh/osh.asdl
@@ -167,8 +167,7 @@ module osh
  | Assignment(id keyword, string* flags, assign_pair* pairs)
  | ControlFlow(token token, word? arg_word)
  | Pipeline(command* children, bool negated, int* stderr_indices)
-    -- TODO: Should be left and right
-  | AndOr(command* children, id op_id)
+  | AndOr(id* ops, command* children)
    -- Part of for/while/until.  Can have one or more children.
  | DoGroup(command* children, redir* redirects)
    -- A brace group is a compound command, with redirects.  Can have one or
diff --git a/spec/smoke.test.sh b/spec/smoke.test.sh
@@ -22,6 +22,23 @@ hostname | wc -l
 echo hi | wc -l
 # stdout: 1

+### and-or chains
+echo 1 && echo 2 || echo 3 && echo 4
+echo --
+false || echo A
+false || false || echo B
+false || false || echo C && echo D || echo E
+## STDOUT:
+1
+2
+4
+--
+A
+B
+C
+D
+## END
+
 ### here doc with var
 v=one
 tac <<EOF
diff --git a/test/gold.sh b/test/gold.sh
@@ -61,14 +61,6 @@ html-summary() {
  _compare test/spec-runner.sh html-summary
 }

-configure() {
-  _compare ./configure
-}
-
-no-op() {
-  _compare scripts/count.sh
-}
-
 gen-module-init() {
  local modules='time datetime'
  _compare build/actions.sh gen-module-init $modules
@@ -87,24 +79,18 @@ startup-benchmark() {
  _compare benchmarks/startup.sh compare-strace
 }

-glob() {
-  _compare gold/glob.sh
-}
-
-nix() {
-  _compare gold/nix.sh isElfSimpleWithStdin
-}
-
-readonly_() {
-  _compare gold/readonly.sh
-}
-
-complex-here-docs() {
-  _compare gold/complex-here-docs.sh
-}
+configure() { _compare ./configure; }
+nix() { _compare gold/nix.sh isElfSimpleWithStdin; }
+and-or() { _compare gold/and-or.sh test-simple; }

 comments() { _compare gold/comments.sh; }
+readonly_() { _compare gold/readonly.sh; }
 export() { _compare gold/export.sh; }
+glob() { _compare gold/glob.sh; }
+no-op() { _compare scripts/count.sh; }
+complex-here-docs() { _compare gold/complex-here-docs.sh; }
+
+
 # Not implemented in osh.
 dollar-sq() { _compare gold/dollar-sq.sh; }

@@ -119,19 +105,24 @@ readonly -a PASSING=(
  # FLAKY: This one differs by timestamp
  #version-text

+  configure
+  nix
+  and-or
+
  comments
  readonly_
+  export
+  glob
+  no-op
+  complex-here-docs
+
  count
  one-spec-test
  html-summary
-  configure
-  no-op
  gen-module-init
-  glob
-  complex-here-docs

  # This one takes a little long, but it's realistic.
-  wild
+  #wild

  # There are slight differences in the number of syscalls reported.  Not sure
  # of the cause.
diff --git a/test/spec.sh b/test/spec.sh
@@ -474,7 +474,7 @@ assoc-zsh() {
 # NOTE: zsh passes about half and fails about half.  It supports a subset of [[
 # I guess.
 dbracket() {
-  sh-spec spec/dbracket.test.sh --osh-failures-allowed 5 \
+  sh-spec spec/dbracket.test.sh --osh-failures-allowed 4 \
    $BASH $MKSH $OSH "$@"
  #sh-spec spec/dbracket.test.sh $BASH $MKSH $OSH $ZSH "$@"
 }