[errors] Improve error message for strict_errexit

And point to a more precise location.

This affects the error in issue #2118.

[doc] Fix for CI

build/doc.sh

@@ -465,7 +465,7 @@ proc p3 {
 EOF
 
   cat >$work_dir/demo.py <<EOF
-#!/usr/bin/env python3
+#!/usr/bin/env python
 
 print("hi")
 EOF

display/ui.py

@@ -53,8 +53,8 @@ def CommandType(cmd):
     # type: (command_t) -> str
     """For displaying commands in the UI."""
 
-    # Displays 'command.Simple' for now, maybe change it.
-    return command_str(cmd.tag())
+    # Displays 'Simple', 'BraceGroup', etc.
+    return command_str(cmd.tag(), dot=False)
 
 
 def PrettyId(id_):

doc/error-catalog.md

@@ -414,6 +414,29 @@ Or:
 
 <!-- TODO -->
 
+## Runtime Errors: `strict:all`
+
+### OILS-ERR-300
+
+```
+  if ! ls | wc -l; then echo failed; fi
+          ^
+[ -c flag ]:1: fatal: Command conditionals should only have one status, not Pipeline (strict_errexit, OILS-ERR-300)
+```
+
+Compound commands can't be used as conditionals because it's ambiguous.
+
+It confuses true/false with pass/fail.  What if part of the pipeline fails?
+What if `ls` doesn't exist?
+
+This YSH idiom is more explicit:
+
+    try {
+      ls | wc -l
+    }
+    if (_error.code !== 0) {
+      echo failed
+    }
 
 ## Appendix
 

osh/cmd_eval.py

@@ -118,6 +118,8 @@
 NoDebugTrap = 1 << 4
 NoErrTrap = 1 << 5
 
+_STRICT_ERREXIT_COND_MSG = "Command conditionals should only have one status, not %s (strict_errexit, OILS-ERR-300)"
+
 
 def MakeBuiltinArgv(argv1):
     # type: (List[str]) -> cmd_value.Argv
@@ -137,7 +139,7 @@ def __init__(self):
 
 
 def _HasManyStatuses(node):
-    # type: (command_t) -> bool
+    # type: (command_t) -> Optional[command_t]
     """Code patterns that are bad for POSIX errexit.  For YSH strict_errexit.
 
     Note: strict_errexit also uses
@@ -148,7 +150,7 @@ def _HasManyStatuses(node):
         # Atoms.
         # TODO: Do we need YSH atoms here?
         if case(command_e.Simple, command_e.DBracket, command_e.DParen):
-            return False
+            return None
 
         elif case(command_e.Redirect):
             node = cast(command.Redirect, UP_node)
@@ -167,23 +169,22 @@ def _HasManyStatuses(node):
                 return _HasManyStatuses(node.children[0])
             else:
                 # Multiple parts like 'ls | wc' is disallowed
-                return True
+                return node
 
         elif case(command_e.AndOr):
             node = cast(command.AndOr, UP_node)
             for c in node.children:
                 if _HasManyStatuses(c):
-                    return True
-            return False  # otherwise allow 'if true && true; ...'
+                    return c
+            return None  # otherwise allow 'if true && true; ...'
 
         # - ShAssignment could be allowed, though its exit code will always be
         #   0 without command subs
         # - Naively, (non-singleton) pipelines could be allowed because pipefail.
         #   BUT could be a proc executed inside a child process, which causes a
         #   problem: the strict_errexit check has to occur at runtime and there's
         #   no way to signal it ot the parent.
-
-    return True
+    return node
 
 
 def PlusEquals(old_val, val):
@@ -570,11 +571,10 @@ def _StrictErrExit(self, node):
         if not (self.exec_opts.errexit() and self.exec_opts.strict_errexit()):
             return
 
-        if _HasManyStatuses(node):
-            node_str = ui.CommandType(node)
-            e_die(
-                "strict_errexit only allows simple commands in conditionals (got %s). "
-                % node_str, loc.Command(node))
+        bad_node = _HasManyStatuses(node)
+        if bad_node:
+            node_str = ui.CommandType(bad_node)
+            e_die(_STRICT_ERREXIT_COND_MSG % node_str, loc.Command(bad_node))
 
     def _StrictErrExitList(self, node_list):
         # type: (List[command_t]) -> None
@@ -592,12 +592,10 @@ def _StrictErrExitList(self, node_list):
 
         assert len(node_list) > 0
         node = node_list[0]
-        if _HasManyStatuses(node):
-            # TODO: consolidate error message with above
-            node_str = ui.CommandType(node)
-            e_die(
-                "strict_errexit only allows simple commands in conditionals (got %s). "
-                % node_str, loc.Command(node))
+        bad_node = _HasManyStatuses(node)
+        if bad_node:
+            node_str = ui.CommandType(bad_node)
+            e_die(_STRICT_ERREXIT_COND_MSG % node_str, loc.Command(bad_node))
 
     def _EvalCondition(self, cond, blame_tok):
         # type: (condition_t, Token) -> bool

test/runtime-errors.sh

@@ -285,27 +285,30 @@ test-errexit-multiple-processes() {
 _strict-errexit-case() {
   local code=$1
 
-  case-banner "[strict_errexit] $code"
+  #case-banner "[strict_errexit] $code"
 
   _osh-error-1 \
     "set -o errexit; shopt -s strict_errexit; $code"
   echo
 }
 
-test-strict_errexit_1() {
+test-strict-errexit-1() {
   # Test out all the location info
 
   _strict-errexit-case '! { echo 1; echo 2; }'
 
   _strict-errexit-case '{ echo 1; echo 2; } && true'
   _strict-errexit-case '{ echo 1; echo 2; } || true'
+  _strict-errexit-case '{ echo 1; echo 2; } >/dev/null || true'
 
   # More chains
   _strict-errexit-case '{ echo 1; echo 2; } && true && true'
   _strict-errexit-case 'true && { echo 1; echo 2; } || true || true'
   _strict-errexit-case 'true && true && { echo 1; echo 2; } || true || true'
 
   _strict-errexit-case 'if { echo 1; echo 2; }; then echo IF; fi'
+  _strict-errexit-case 'if { echo 1; echo 2; } >/dev/null; 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'
 
@@ -315,7 +318,7 @@ test-strict_errexit_1() {
                         if p { echo hi }'
 }
 
-test-strict_errexit_conditionals() {
+test-strict-errexit-conditionals() {
   # this works, even though this is a subshell
   _strict-errexit-case '
 myfunc() { return 1; }
@@ -392,7 +395,7 @@ test-strict-errexit-old() {
 
   # command.Pipeline.
   _strict-errexit-case 'if ls | wc -l; then echo Pipeline; fi'
-  _strict-errexit-case 'if ! ls | wc -l; then echo Pipeline; fi'
+  _strict-errexit-case 'if ! ls | wc -l; then echo failed; fi'
 
   # This one is ALLOWED
   #_strict-errexit-case 'if ! ls; then echo Pipeline; fi'