Temporary env bindings are not visible during function calls.

That is, 'FOO=bar myfunc' now works.  This bug was tickled by Aboriginal
Linux.

Prior to this change, I constructed an 'environ' dict from the env
bindings, and only passed that to external processes.

Now there is a read-only _StackFrame() on the variable stack.

It can be (1) read from and (2) set with the new scope_e.TempEnv mode.
LocalOnly and Dynamic scope skip it.  The 'read' builtin uses LocalOnly
so the binding persists.

In the spec tests: a very good exploration of temporary env semantics.
dash, bash, and mksh all behave differently!

No spec tests regressions.

Also:

- Basic xtrace tracing for Assignment.  In OSH, assignments are
  different than commands.

core/cmd_exec.py

@@ -459,31 +459,6 @@ def _EvalRedirects(self, node):
       redirects.append(r)
     return redirects
 
-  def _EvalEnv(self, node_env, out_env):
-    """Evaluate environment variable bindings.
-
-    Args:
-      node_env: list of ast.env_pair
-      out_env: mutated.
-    """
-    # NOTE: Env evaluation is done in new scope so it doesn't persist.  It also
-    # pushes argv.  Don't need that?
-    self.mem.PushTemp()
-    for env_pair in node_env:
-      name = env_pair.name
-      rhs = env_pair.val
-
-      # Could pass extra bindings like out_env here?  But PushTemp should work?
-      val = self.word_ev.EvalWordToString(rhs)
-
-      # Set each var so the next one can reference it.  Example:
-      # FOO=1 BAR=$FOO ls /
-      # TODO: Could add spid to LhsName.
-      self.mem.SetVar(ast.LhsName(name), val, (), scope_e.LocalOnly)
-
-      out_env[name] = val.s
-    self.mem.PopTemp()
-
   def _MakeProcess(self, node, job_state=None):
     """
     Assume we will run the node in another process.  Return a process.
@@ -509,7 +484,7 @@ def _MakeProcess(self, node, job_state=None):
     p = process.Process(thunk, job_state=job_state)
     return p
 
-  def _RunSimpleCommand(self, argv, environ, fork_external):
+  def _RunSimpleCommand(self, argv, fork_external):
     # This happens when you write "$@" but have no arguments.
     if not argv:
       return 0  # status 0, or skip it?
@@ -544,6 +519,8 @@ def _RunSimpleCommand(self, argv, environ, fork_external):
         status = 2  # consistent error code for usage error
       return status
 
+    environ = self.mem.GetExported()  # Include temporary variables
+
     if fork_external:
       thunk = process.ExternalThunk(argv, environ)
       p = process.Process(thunk)
@@ -612,6 +589,14 @@ def _RunJobInBackground(self, node):
       log('Started background job with pid %d', pid)
     return 0
 
+  def _SetSourceLocation(self, span_id):
+    # TODO: This API should be simplified
+    line_span = self.arena.GetLineSpan(span_id)
+    line_id = line_span.line_id
+    line = self.arena.GetLine(line_id)
+    source_name, line_num = self.arena.GetDebugInfo(line_id)
+    self.mem.SetSourceLocation(source_name, line_num)
+
   def _Dispatch(self, node, fork_external):
     argv0 = None  # for error message
     check_errexit = False  # for errexit
@@ -632,11 +617,6 @@ def _Dispatch(self, node, fork_external):
 
       words = braces.BraceExpandWords(node.words)
       argv = self.word_ev.EvalWordSequence(words)
-      if argv:
-        argv0 = argv[0]
-
-      environ = self.mem.GetExported()
-      self._EvalEnv(node.more_env, environ)
 
       # This is a very basic implementation for PS4='+$SOURCE_NAME:$LINENO:'
 
@@ -660,18 +640,30 @@ def _Dispatch(self, node, fork_external):
       if found:
         # NOTE: This is what we want to expose as variables for PS4.
         #ui.PrintFilenameAndLine(span_id, self.arena)
-
-        line_span = self.arena.GetLineSpan(span_id)
-        line_id = line_span.line_id
-        line = self.arena.GetLine(line_id)
-        source_name, line_num = self.arena.GetDebugInfo(line_id)
-        self.mem.SetSourceLocation(source_name, line_num)
+        self._SetSourceLocation(span_id)
       else:
         self.mem.SetSourceLocation('<unknown>', -1)
 
+      # This comes before evaluating env, in case there are problems evaluating
+      # it.  We could trace the env separately?  Also trace unevaluated code
+      # with set-o verbose?
       self.tracer.OnSimpleCommand(argv)
 
-      status = self._RunSimpleCommand(argv, environ, fork_external)
+      if node.more_env:
+        self.mem.PushTemp()
+      try:
+        for env_pair in node.more_env:
+          val = self.word_ev.EvalWordToString(env_pair.val)
+          # Set each var so the next one can reference it.  Example:
+          # FOO=1 BAR=$FOO ls /
+          self.mem.SetVar(ast.LhsName(env_pair.name), val,
+                          (var_flags_e.Exported,), scope_e.TempEnv)
+
+        # NOTE: This might never return!  In the case of fork_external=False.
+        status = self._RunSimpleCommand(argv, fork_external)
+      finally:
+        if node.more_env:
+          self.mem.PopTemp()
 
       if self.exec_opts.xtrace:
         #log('+ %s -> %d', argv, status)
@@ -775,6 +767,15 @@ def _Dispatch(self, node, fork_external):
 
         self.mem.SetVar(lval, val, flags, lookup_mode)
 
+        # Assignment always appears to have a spid.
+        if node.spids:
+          self._SetSourceLocation(node.spids[0])
+        else:
+          # TODO: when does this happen?  Warn.
+          #log('Warning: assignment has no location information: %s', node)
+          self.mem.SetSourceLocation('<unknown>', -1)
+        self.tracer.OnAssignment(lval, val, flags, lookup_mode)
+
       # TODO: This should be eval of RHS, unlike bash!
       status = 0
 
@@ -1168,13 +1169,9 @@ def __init__(self, exec_opts, mem, word_ev):
     self.arena = alloc.PluginArena('<$PS4>')
     self.parse_cache = {}  # PS4 value -> CompoundWord.  PS4 is scoped.
 
-  def OnSimpleCommand(self, argv):
+  def _EvalPS4(self):
     """For set -x."""
 
-    # NOTE: I think tracing should be on by default?  For post-mortem viewing.
-    if not self.exec_opts.xtrace:
-      return
-
     val = self.mem.GetVar('PS4')
     assert val.tag == value_e.Str
 
@@ -1214,9 +1211,25 @@ def OnSimpleCommand(self, argv):
     # <ERROR: cannot evaluate PS4>
     prefix = self.word_ev.EvalWordToString(ps4_word)
 
-    print('%s%s%s' % (
-        first_char, prefix.s, ' '.join(_PrettyString(a) for a in argv)),
-        file=sys.stderr)
+    return first_char, prefix.s
+
+  def OnSimpleCommand(self, argv):
+    # NOTE: I think tracing should be on by default?  For post-mortem viewing.
+    if not self.exec_opts.xtrace:
+      return
+
+    first_char, prefix = self._EvalPS4()
+    cmd = ' '.join(_PrettyString(a) for a in argv)
+    print('%s%s%s' % (first_char, prefix, cmd), file=sys.stderr)
+
+  def OnAssignment(self, lval, val, flags, lookup_mode):
+    # NOTE: I think tracing should be on by default?  For post-mortem viewing.
+    if not self.exec_opts.xtrace:
+      return
+
+    # Now we have to get the prefix
+    first_char, prefix = self._EvalPS4()
+    print('%s%s%s = %s' % (first_char, prefix, lval, val), file=sys.stderr)
 
   def Event(self):
     """

core/runtime.asdl

@@ -48,7 +48,7 @@ module runtime
   cell = (value val, bool exported, bool readonly)
 
   var_flags = Exported | ReadOnly
-  scope = LocalOnly | GlobalOnly | Dynamic
+  scope = TempEnv | LocalOnly | GlobalOnly | Dynamic
 
   -- For assignment, evaluated to osh_ast.lhs_expr
   lvalue = 

core/state.py

@@ -10,6 +10,7 @@
 state.py -- Interpreter state
 """
 
+import cStringIO
 import os
 
 from core import args
@@ -241,6 +242,22 @@ def SetArgv(self, argv):
     self.num_shifted = 0
 
 
+class _StackFrame(object):
+  def __init__(self, readonly=False):
+    self.vars = {}  # string -> runtime.cell
+    self.readonly = readonly
+
+  def __repr__(self):
+    f = cStringIO.StringIO()
+    f.write('<_StackFrame readonly:%s' % self.readonly)
+    for name, cell in self.vars.iteritems():
+      f.write('  %s = ' % name)
+      f.write('  %s' % cell)
+      f.write('\n')
+    f.write('>')
+    return f.getvalue()
+
+
 class DirStack(object):
   """For pushd/popd/dirs."""
 
@@ -264,6 +281,18 @@ def Iter(self):
     return reversed(self.stack)
 
 
+def _FormatStack(var_stack):
+  """Temporary debugging.
+
+  TODO: Turn this into a real JSON dump or something.
+  """
+  f = cStringIO.StringIO()
+  for i, entry in enumerate(var_stack):
+    f.write('[%d] %s' % (i, entry))
+    f.write('\n')
+  return f.getvalue()
+
+
 class Mem(object):
   """For storing variables.
 
@@ -279,7 +308,7 @@ class Mem(object):
   """
 
   def __init__(self, argv0, argv, environ, arena):
-    top = {}  # string -> runtime.cell
+    top = _StackFrame()
     self.var_stack = [top]
     self.argv0 = argv0
     self.argv_stack = [_ArgFrame(argv)]
@@ -303,13 +332,12 @@ def __init__(self, argv0, argv, environ, arena):
     self._InitVarsFromEnv(environ)
     self.arena = arena
 
-
   def __repr__(self):
     parts = []
     parts.append('<Mem')
     for i, frame in enumerate(self.var_stack):
       parts.append('  -- %d --' % i)
-      for n, v in frame.iteritems():
+      for n, v in frame.vars.iteritems():
         parts.append('  %s %s' % (n, v))
     parts.append('>')
     return '\n'.join(parts) + '\n'
@@ -366,7 +394,7 @@ def PushCall(self, func_name, argv):
     # bash uses this order: top of stack first.
     self.func_name_stack.append(func_name)
 
-    self.var_stack.append({})
+    self.var_stack.append(_StackFrame())
     self.argv_stack.append(_ArgFrame(argv))
 
   def PopCall(self):
@@ -377,10 +405,12 @@ def PopCall(self):
 
   def PushTemp(self):
     """For the temporary scope in 'FOO=bar BAR=baz echo'."""
-    self.var_stack.append({})
+    # We don't want the 'read' builtin to write to this frame!
+    self.var_stack.append(_StackFrame(readonly=True))
 
   def PopTemp(self):
     self.var_stack.pop()
+    #util.log('**** PopTemp()')
 
   #
   # Argv
@@ -441,8 +471,15 @@ def GetSpecialVar(self, op_id):
   # Named Vars
   #
 
-  def _FindCellAndNamespace(self, name, lookup_mode):
-    """
+  def _FindCellAndNamespace(self, name, lookup_mode, is_read=False):
+    """Helper for getting and setting variable.
+
+    Need a mode to skip Temp scopes.  For Setting.
+
+    Args:
+      name: the variable name
+      lookup_mode: scope_e
+
     Returns:
       cell: The cell corresponding to looking up 'name' with the given mode, or
         None if it's not found.
@@ -451,18 +488,32 @@ def _FindCellAndNamespace(self, name, lookup_mode):
     if lookup_mode == scope_e.Dynamic:
       found = False
       for i in range(len(self.var_stack) - 1, -1, -1):
-        namespace = self.var_stack[i]
+        frame = self.var_stack[i]
+        if frame.readonly and not is_read:
+          continue
+        namespace = frame.vars
         if name in namespace:
           cell = namespace[name]
           return cell, namespace
-      return None, self.var_stack[0]
+      return None, self.var_stack[0].vars  # set in global namespace
 
     elif lookup_mode == scope_e.LocalOnly:
-      namespace = self.var_stack[-1]
+      frame = self.var_stack[-1]
+      if frame.readonly and not is_read:
+        frame = self.var_stack[-2]
+        # The frame below a readonly one shouldn't be readonly.
+        assert not frame.readonly, frame
+        #assert not frame.readonly, self._Format(self.var_stack)
+      namespace = frame.vars
+      return namespace.get(name), namespace
+
+    elif lookup_mode == scope_e.TempEnv:
+      frame = self.var_stack[-1]
+      namespace = frame.vars
       return namespace.get(name), namespace
 
     elif lookup_mode == scope_e.GlobalOnly:
-      namespace = self.var_stack[0]
+      namespace = self.var_stack[0].vars
       return namespace.get(name), namespace
 
     else: 
@@ -612,7 +663,7 @@ def InternalSetGlobal(self, name, new_val):
 
     Use case: SHELLOPTS.
     """
-    cell = self.var_stack[0][name]
+    cell = self.var_stack[0].vars[name]
     cell.val = new_val
 
   # NOTE: Have a default for convenience
@@ -636,7 +687,7 @@ def GetVar(self, name, lookup_mode=scope_e.Dynamic):
     if name == 'SOURCE_NAME':
       return self.source_name
 
-    cell, _ = self._FindCellAndNamespace(name, lookup_mode)
+    cell, _ = self._FindCellAndNamespace(name, lookup_mode, is_read=True)
 
     if cell:
       return cell.val
@@ -690,7 +741,7 @@ def GetExported(self):
     # Search from globals up.  Names higher on the stack will overwrite names
     # lower on the stack.
     for scope in self.var_stack:
-      for name, cell in scope.items():
+      for name, cell in scope.vars.items():
         if cell.exported and cell.val.tag == value_e.Str:
           exported[name] = cell.val.s
     return exported