Enhance runtime error messages based on running git-completion.bash.

- a[x]=y when a is not an array
- Invalid PatSub (still need to figure out why this is happening)

Also:

- Catch exceptions from completion functions, and display errors.
- Simplify the ASDL representation of PatSub.

core/cmd_exec.py

@@ -395,7 +395,9 @@ def _EvalLhs(self, node, spid):
 
     if node.tag == lhs_expr_e.LhsIndexedName:  # a[1+2]=x
       i = self.arith_ev.Eval(node.index)
-      return runtime.LhsIndexedName(node.name, i)
+      runtime_node = runtime.LhsIndexedName(node.name, i)
+      runtime_node.spids.append(node.spids[0])  # copy left-most token over
+      return runtime_node
 
     raise AssertionError(node.tag)
 
@@ -1333,33 +1335,11 @@ def RunProcessSub(self, node, op_id):
     else:
       raise AssertionError
 
-    # TODO: Generalize process sub?
-    #
-    # - Make it work first, bare minimum.
-    # - Then Make something like Pipeline()?
-    #   - you add all the argument processes
-    #   - then you add the main processes, with those as args
-    #   - then p.Wait()
-    #     - get status for all of them?
-    #
-    # Problem is that you don't see this until word_eval?
-    # You can scan a simple command for these though.
-
-    # TODO:
-    # - Do we need to somehow register a waiter?  After SimpleCommand,
-    #   argv and redirect words need to wait?
-    # - what about for loops?  case?  ControlFlow?  temp binding,
-    #   assignments, etc. They all have words
-    #   - disallow those?
-    # I guess you need it at the end of every command sub loop?
-    # But you want to detect statically if you need to wait?
-    # Maybe just have a dirty flag?  needs_wait
-      # - Make a pipe
-      # - Start another process connected to the write end of the pipe.
-      # - Return [/dev/fd/FD] as the read end of the pipe.
-
   def RunFunc(self, func_node, argv):
-    """Used by completion engine."""
+    """Used by completion engine.
+
+    Also used to run SimpleCommand.  TODO: Need to catch FatalRuntimeError.
+    """
     # These are redirects at DEFINITION SITE.  You can also have redirects at
     # the CALLER.  For example:
 

core/completion.py

@@ -230,6 +230,7 @@ def Matches(self, words, index, prefix):
     state.SetGlobalString(self.ex.mem, 'COMP_CWORD', str(index))
 
     self.ex.RunFunc(self.func, [])  # call with no arguments
+
 
     # Should be COMP_REPLY to follow naming convention!  Lame.
     val = state.GetGlobal(self.ex.mem, 'COMPREPLY')
@@ -646,21 +647,24 @@ def Matches(self, buf, status_out):
 
     index = len(comp_words) - 1  # COMP_CWORD -1 when it's empty
     i = 0
-    for m in chain.Matches(comp_words, index, prefix):
-      # TODO: need to dedupe these
-      yield m
-      i += 1
+    try:
+      for m in chain.Matches(comp_words, index, prefix):
+        # TODO: need to dedupe these
+        yield m
+        i += 1
+        elapsed = time.time() - start_time
+        plural = '' if i == 1 else 'es'
+        status_out.Write(0,
+            '... %d match%s for %r in %.2f seconds (Ctrl-C to cancel)', i,
+            plural, buf, elapsed)
+
       elapsed = time.time() - start_time
       plural = '' if i == 1 else 'es'
       status_out.Write(0,
-          '... %d match%s for %r in %.2f seconds (Ctrl-C to cancel)', i,
+          'Found %d match%s for %r in %.2f seconds', i,
           plural, buf, elapsed)
-
-    elapsed = time.time() - start_time
-    plural = '' if i == 1 else 'es'
-    status_out.Write(0,
-        'Found %d match%s for %r in %.2f seconds', i,
-        plural, buf, elapsed)
+    except util.FatalRuntimeError as e:
+      ui.PrettyPrintError(e, self.parse_ctx.arena, sys.stderr)
 
     # TODO: Have to de-dupe and sort these?  Because 'echo' is a builtin as
     # well as a command, and we don't want to show it twice.  Although then

core/libstr.py

@@ -281,14 +281,14 @@ def __init__(self, pat, replace_str):
     self.replace_str = replace_str
 
   def Replace(self, s, op):
-    if op.do_all:
+    if op.replace_mode == Id.Lit_Slash:  # replace all
       return s.replace(self.pat, self.replace_str)
-    elif op.do_prefix:
+    elif op.replace_mode == Id.Lit_Pound:  # replace prefix
       if s.startswith(self.pat):
         return self.replace_str + s[self.pat_len:]
       else:
         return s
-    elif op.do_suffix:
+    elif op.replace_mode == Id.Lit_Percent:  # replace suffix
       if s.endswith(self.pat):
         # NOTE: This handles ${s/#/foo}.  See spec test in var-op-strip.
         i = len(s) - self.pat_len
@@ -300,22 +300,26 @@ def Replace(self, s, op):
 
 
 class _GlobReplacer(_Replacer):
-  def __init__(self, regex, replace_str):
+  def __init__(self, regex, replace_str, slash_spid):
     # TODO: It would be nice to cache the compilation of the regex here,
     # instead of just the string.  That would require more sophisticated use of
     # the Python/C API in libc.c, which we might want to avoid.
     self.regex = regex
     self.replace_str = replace_str
+    self.slash_spid = slash_spid
 
   def Replace(self, s, op):
     regex = '(%s)' % self.regex  # make it a group
 
-    if op.do_all:
-      return _PatSubAll(s, regex, self.replace_str)  # loop over matches
+    if op.replace_mode == Id.Lit_Slash:
+      try:
+        return _PatSubAll(s, regex, self.replace_str)  # loop over matches
+      except RuntimeError as e:
+        e_die('Error matching regex %r: %s', regex, e, span_id=self.slash_spid)
 
-    if op.do_prefix:
+    if op.replace_mode == Id.Lit_Pound:
       regex = '^' + regex
-    elif op.do_suffix:
+    elif op.replace_mode == Id.Lit_Percent:
       regex = regex + '$'
 
     m = libc.regex_first_group_match(regex, s, 0)
@@ -326,7 +330,7 @@ def Replace(self, s, op):
     return s[:start] + self.replace_str + s[end:]
 
 
-def MakeReplacer(pat, replace_str):
+def MakeReplacer(pat, replace_str, slash_spid):
   """Helper for ${x/pat/replace}
 
   Parses 'pat' and returns either a _GlobReplacer or a _ConstStringReplacer.
@@ -341,4 +345,4 @@ def MakeReplacer(pat, replace_str):
   if regex is None:
     return _ConstStringReplacer(pat, replace_str)
   else:
-    return _GlobReplacer(regex, replace_str)
+    return _GlobReplacer(regex, replace_str, slash_spid)

core/state.py

@@ -13,13 +13,11 @@
 import cStringIO
 import os
 
+from asdl import const
 from core import args
 from core import legacy
-from osh.meta import runtime
 from core import util
-from osh.meta import Id
-
-from osh.meta import ast
+from osh.meta import ast, runtime, Id
 
 part_value_e = runtime.part_value_e
 value_e = runtime.value_e
@@ -676,19 +674,28 @@ def SetVar(self, lval, value, new_flags, lookup_mode):
         e_die("Can't export array")  # TODO: error context
 
     elif lval.tag == lvalue_e.LhsIndexedName:
+      # TODO: All paths should have this?  We can get here by a[x]=1 or
+      # (( a[ x ] = 1 )).  Maybe we should make them different?
+      if lval.spids:
+        left_spid = lval.spids[0]
+      else:
+        left_spid = const.NO_INTEGER
+
+      # TODO: This is a parse error!
       # a[1]=(1 2 3)
       if value.tag == value_e.StrArray:
-        e_die("Can't assign array to array member")  # TODO: error context
+        e_die("Can't assign array to array member", span_id=left_spid)
 
       cell, namespace = self._FindCellAndNamespace(lval.name, lookup_mode)
       if cell:
         if cell.val.tag != value_e.StrArray:
           # s=x
-          # s[1]=y
-          e_die("Can't index non-array")  # TODO: error context
+          # s[1]=y  # invalid
+          e_die("Entries in value of type %s can't be assigned to",
+                cell.val.__class__.__name__, span_id=left_spid)
 
         if cell.readonly:
-          e_die("Can't assign to readonly value")
+          e_die("Can't assign to readonly value", span_id=left_spid)
 
         strs = cell.val.strs
         try:

core/state_test.py

@@ -179,23 +179,19 @@ def testSetVarClearFlag(self):
     mem.ClearFlag('PYTHONPATH', var_flags_e.Exported, scope_e.Dynamic)
     self.assertEqual(False, mem.var_stack[0].vars['PYTHONPATH'].exported)
 
+    lhs = runtime.LhsIndexedName('a', 1)
+    lhs.spids.append(0)
     # a[1]=2
-    mem.SetVar(
-        runtime.LhsIndexedName('a', 1), runtime.Str('2'), (),
-        scope_e.Dynamic)
+    mem.SetVar(lhs, runtime.Str('2'), (), scope_e.Dynamic)
     self.assertEqual([None, '2'], mem.var_stack[0].vars['a'].val.strs)
 
     # a[1]=3
-    mem.SetVar(
-        runtime.LhsIndexedName('a', 1), runtime.Str('3'), (),
-        scope_e.Dynamic)
+    mem.SetVar(lhs, runtime.Str('3'), (), scope_e.Dynamic)
     self.assertEqual([None, '3'], mem.var_stack[0].vars['a'].val.strs)
 
     # a[1]=(x y z)  # illegal
     try:
-      mem.SetVar(
-          runtime.LhsIndexedName('a', 1), runtime.StrArray(['x', 'y', 'z']),
-          (), scope_e.Dynamic)
+      mem.SetVar(lhs, runtime.StrArray(['x', 'y', 'z']), (), scope_e.Dynamic)
     except util.FatalRuntimeError as e:
       pass
     else:
@@ -208,10 +204,8 @@ def testSetVarClearFlag(self):
     self.assertEqual(True, mem.var_stack[0].vars['a'].readonly)
 
     try:
-      # a[2]=3
-      mem.SetVar(
-          runtime.LhsIndexedName('a', 1), runtime.Str('3'), (),
-          scope_e.Dynamic)
+      # a[1]=3
+      mem.SetVar(lhs, runtime.Str('3'), (), scope_e.Dynamic)
     except util.FatalRuntimeError as e:
       pass
     else:

core/word_eval.py

@@ -585,7 +585,7 @@ def _EvalBracedVarSub(self, part, part_vals, quoted):
           replace_str = ''
 
         # Either GlobReplacer or ConstStringReplacer
-        replacer = libstr.MakeReplacer(pat_val.s, replace_str)
+        replacer = libstr.MakeReplacer(pat_val.s, replace_str, op.spids[0])
 
         if val.tag == value_e.Str:
           s = replacer.Replace(val.s, op)

doc/architecture-notes.md

@@ -48,8 +48,16 @@ This isn't re-parsing, but it's re-reading.
 
 - source and eval
 - trap
+  - exit
+  - debug
+  - err
+  - signals
 - PS1 and PS4 (WordParser is used)
+
+### Function Callbacks
+
 - completion hooks registered by `complete -F ls_complete_func ls`
+- bash has a `command_not_found` hook; osh doesn't yet
 
 ## Parse errors at runtime (need line numbers)
 

osh/cmd_parse.py

@@ -155,6 +155,7 @@ def _MakeAssignPair(parse_ctx, preparsed):
     expr = a_parser.Parse()  # raises util.ParseError
                              # TODO: It reports from the wrong arena!
     lhs_expr = ast.LhsIndexedName(var_name, expr)
+    lhs_expr.spids.append(left_token.span_id)
 
   else:
     raise AssertionError

osh/osh.asdl

@@ -51,7 +51,8 @@ module osh
 
   suffix_op = 
     StringUnary(id op_id, word arg_word)  -- e.g. ${v:-default}
-  | PatSub(word pat, word? replace, bool do_all, bool do_prefix, bool do_suffix)
+    -- TODO: token for / to attribute errors
+  | PatSub(word pat, word? replace, id replace_mode)
   -- begin is optional with ${array::1}
   | Slice(arith_expr? begin, arith_expr? length)
 

osh/word_parse.py

@@ -162,10 +162,6 @@ def _ReadPatSubVarOp(self, lex_mode):
     VarSub    = ...
               | VarOf '/' Match '/' WORD
     """
-    do_all = False
-    do_prefix = False
-    do_suffix = False
-
     pat = self._ReadVarOpArg(lex_mode, eof_type=Id.Lit_Slash, empty_ok=False)
 
     if len(pat.parts) == 1:
@@ -180,26 +176,21 @@ def _ReadPatSubVarOp(self, lex_mode):
       p_die('Pattern in ${x/pat/replace} must not be empty',
             token=self.cur_token)
 
+    replace_mode = Id.Undefined_Tok
     # Check for / # % modifier on pattern.
     first_part = pat.parts[0]
     if first_part.tag == word_part_e.LiteralPart:
       lit_id = first_part.token.id
-      if lit_id == Id.Lit_Slash:
-        do_all = True
-        pat.parts.pop(0)
-      elif lit_id == Id.Lit_Pound:
-        do_prefix = True
-        pat.parts.pop(0)
-      elif lit_id == Id.Lit_Percent:
-        do_suffix = True
+      if lit_id in (Id.Lit_Slash, Id.Lit_Pound, Id.Lit_Percent):
         pat.parts.pop(0)
+        replace_mode = lit_id
 
     # NOTE: If there is a modifier, the pattern can be empty, e.g.
     # ${s/#/foo} and ${a/%/foo}.
 
     if self.token_type == Id.Right_VarSub:
       # e.g. ${v/a} is the same as ${v/a/}  -- empty replacement string
-      return ast.PatSub(pat, None, do_all, do_prefix, do_suffix)
+      return ast.PatSub(pat, None, replace_mode)
 
     if self.token_type == Id.Lit_Slash:
       replace = self._ReadVarOpArg(lex_mode)  # do not stop at /
@@ -212,7 +203,7 @@ def _ReadPatSubVarOp(self, lex_mode):
         p_die("Expected } after replacement string, got %s", self.cur_token,
               token=self.cur_token)
 
-      return ast.PatSub(pat, replace, do_all, do_prefix, do_suffix)
+      return ast.PatSub(pat, replace, replace_mode)
 
     # Happens with ${x//} and ${x///foo}, see test/parse-errors.sh
     p_die("Expected } after pat sub, got %r", self.cur_token.val,
@@ -298,7 +289,9 @@ def _ParseVarExpr(self, arg_lex_mode):
 
     elif op_kind == Kind.VOp2:
       if self.token_type == Id.VOp2_Slash:
+        op_spid = self.cur_token.span_id  # for attributing error to /
         op = self._ReadPatSubVarOp(arg_lex_mode)
+        op.spids.append(op_spid)
         assert op is not None
         # Checked by the method above
         assert self.token_type == Id.Right_VarSub, self.cur_token