[completion] Three improvements for bash_completion.

- Show warning locations for arithmetic errors.  This required threading
  an arena through.

- This led me to notice a bug in [ evaluation, which is now fixed with a
  spec test.  We weren't passing exec_opts in the [ case.
  - It shows that [ is stricter than [[ !

- bash-completion uses printf -v %s and %q, so I wrote some spec tests
  for these features.  However I just replaced these invocations with
  something like:

    eval "$3=$ref"

  So for now OSH doesn't need prinf -v.

bin/oil.py

@@ -230,7 +230,7 @@ def OshMain(argv0, argv, login_shell):
     # also run functions... it gets the Executor through Executor._Complete.
     if HAVE_READLINE:
       splitter = legacy.SplitContext(mem)  # TODO: share with executor.
-      ev = word_eval.CompletionWordEvaluator(mem, exec_opts, splitter)
+      ev = word_eval.CompletionWordEvaluator(mem, exec_opts, splitter, arena)
       progress_f = ui.StatusLine()
       var_action = completion.VariablesActionInternal(ex.mem)
       root_comp = completion.RootCompleter(ev, comp_lookup, var_action,

core/cmd_exec.py

@@ -132,9 +132,12 @@ def __init__(self, mem, fd_state, funcs, comp_lookup, exec_opts, parse_ctx,
 
     self.splitter = legacy.SplitContext(self.mem)
     self.word_ev = word_eval.NormalWordEvaluator(mem, exec_opts, self.splitter,
-                                                 self)
-    self.arith_ev = expr_eval.ArithEvaluator(mem, exec_opts, self.word_ev)
-    self.bool_ev = expr_eval.BoolEvaluator(mem, exec_opts, self.word_ev)
+                                                 self.arena, self)
+    self.arith_ev = expr_eval.ArithEvaluator(mem, exec_opts, self.word_ev,
+                                             self.arena)
+
+    self.bool_ev = expr_eval.BoolEvaluator(mem, exec_opts, self.word_ev,
+                                           self.arena)
 
     self.traps = {}  # signal/hook name -> callable
     self.nodes_to_run = []  # list of nodes, appended to by signal handlers

core/cmd_exec_test.py

@@ -52,14 +52,10 @@ def InitExecutor(arena=None):
 
 
 def InitEvaluator():
-  mem = state.Mem('', [], {}, None)
-  state.SetLocalString(mem, 'x', 'xxx')
-  state.SetLocalString(mem, 'y', 'yyy')
-
-  exec_opts = state.ExecOpts(mem, None)
-  # Don't need side effects for most things
-  splitter = legacy.SplitContext(mem)
-  return word_eval.CompletionWordEvaluator(mem, exec_opts, splitter)
+  word_ev = test_lib.MakeTestEvaluator()
+  state.SetLocalString(word_ev.mem, 'x', 'xxx')
+  state.SetLocalString(word_ev.mem, 'y', 'yyy')
+  return word_ev
 
 
 class ExpansionTest(unittest.TestCase):

core/expr_eval.py

@@ -17,11 +17,12 @@
 except ImportError:
   from benchmarks import fake_libc as libc
 
+from asdl import const
+from core import dev
 from core import util
 from core import state
-from osh.meta import BOOL_ARG_TYPES, Id, types
-from osh.meta import runtime
-from osh.meta import ast
+from core import ui
+from osh.meta import ast, runtime, types, BOOL_ARG_TYPES, Id
 
 log = util.log
 warn = util.warn
@@ -108,16 +109,22 @@ def _StringToInteger(s, word=None):
 
 
 class _ExprEvaluator(object):
-  """
-  For now the arith and bool evaluators share some logic.
+  """Shared between arith and bool evaluators.
+
+  They both:
+
+  1. Convert strings to integers, respecting set -o strict_arith.
+  2. Look up variables and evaluate words.
   """
 
-  def __init__(self, mem, exec_opts, word_ev):
+  def __init__(self, mem, exec_opts, word_ev, arena):
     self.mem = mem
     self.exec_opts = exec_opts
     self.word_ev = word_ev  # type: word_eval.WordEvaluator
+    self.arena = arena
 
   def _StringToIntegerOrError(self, s, word=None):
+    """Used by both [[ $x -gt 3 ]] and (( $x ))."""
     try:
       i = _StringToInteger(s, word=word)
     except util.FatalRuntimeError as e:
@@ -178,7 +185,7 @@ def EvalLhs(node, arith_ev, mem, exec_opts):
       # It would make more sense for 'nounset' to control this, but bash
       # doesn't work that way.
       #if self.exec_opts.strict_arith:
-      #  e_die('Undefined array %r', node.name)  # TODO: error location
+      #  e_die('Undefined array %r', node.name)
       val = runtime.Str('')
 
     elif val.tag == value_e.StrArray:
@@ -247,6 +254,13 @@ def _ValToArithOrError(self, val, int_coerce=True, word=None):
         raise
       else:
         i = 0
+
+        span_id = dev.SpanIdFromError(e)
+        if self.arena:  # BoolEvaluator for test builtin doesn't have it.
+          if span_id != const.NO_INTEGER:
+            ui.PrintFilenameAndLine(span_id, self.arena)
+          else:
+            log('*** Warning has no location info ***')
         warn(e.UserErrorString())
     return i
 
@@ -263,7 +277,9 @@ def _EvalLhsToArith(self, node):
     if val.tag == value_e.StrArray:
       e_die("Can't use assignment like ++ or += on arrays")
 
-    i = self._ValToArithOrError(val)
+    # TODO: attribute a word here.  It really should be a span ID?
+    # We have a few nodes here like UnaryAssign and BinaryAssign.
+    i = self._ValToArithOrError(val, word=None)
     return i, lval
 
   def _Store(self, lval, new_int):

core/test_builtin.py

@@ -171,11 +171,18 @@ def Test(argv, need_right_bracket):
   # mem: Don't need it for BASH_REMATCH?  Or I guess you could support it
   # exec_opts: don't need it, but might need it later
 
-  mem = None
-  exec_opts = None
+  mem = None  # Not necessary
   word_ev = _WordEvaluator()
+  arena = None
 
-  bool_ev = expr_eval.BoolEvaluator(mem, exec_opts, word_ev)
+  # We want [ a -eq a ] to always be an error, unlike [[ a -eq a ]].  This is a
+  # weird case of [[ being less strict.
+  class _DummyExecOpts():
+    def __init__(self):
+      self.strict_arith = True
+  exec_opts = _DummyExecOpts()
+
+  bool_ev = expr_eval.BoolEvaluator(mem, exec_opts, word_ev, arena)
   try:
     b = bool_ev.Eval(bool_node)
   except util.FatalRuntimeError as e:

core/test_lib.py

@@ -88,6 +88,6 @@ def MakeTestEvaluator():
   mem = state.Mem('', [], {}, arena)
   exec_opts = state.ExecOpts(mem, None)
   splitter = legacy.SplitContext(mem)
-  ev = word_eval.CompletionWordEvaluator(mem, exec_opts, splitter)
+  ev = word_eval.CompletionWordEvaluator(mem, exec_opts, splitter, arena)
   return ev
 

core/word_eval.py

@@ -133,13 +133,13 @@ class _WordEvaluator(object):
     EvalRhsWord
     EvalWordSequence
   """
-  def __init__(self, mem, exec_opts, splitter):
+  def __init__(self, mem, exec_opts, splitter, arena):
     self.mem = mem  # for $HOME, $1, etc.
     self.exec_opts = exec_opts  # for nounset
     self.splitter = splitter
     self.globber = glob_.Globber(exec_opts)
     # NOTE: Executor also instantiates one.
-    self.arith_ev = expr_eval.ArithEvaluator(mem, exec_opts, self)
+    self.arith_ev = expr_eval.ArithEvaluator(mem, exec_opts, self, arena)
 
   def _EvalCommandSub(self, part, quoted):
     """Abstract since it has a side effect.
@@ -1039,8 +1039,8 @@ def EvalWordSequence(self, words):
 
 class NormalWordEvaluator(_WordEvaluator):
 
-  def __init__(self, mem, exec_opts, splitter, ex):
-    _WordEvaluator.__init__(self, mem, exec_opts, splitter)
+  def __init__(self, mem, exec_opts, splitter, arena, ex):
+    _WordEvaluator.__init__(self, mem, exec_opts, splitter, arena)
     self.ex = ex
 
   def _EvalCommandSub(self, node, quoted):
@@ -1057,9 +1057,6 @@ class CompletionWordEvaluator(_WordEvaluator):
   Difference from NormalWordEvaluator: No access to executor!  But they both
   have a splitter.
   """
-  def __init__(self, mem, exec_opts, splitter):
-    _WordEvaluator.__init__(self, mem, exec_opts, splitter)
-
   def _EvalCommandSub(self, node, quoted):
     return runtime.StringPartValue('__COMMAND_SUB_NOT_EXECUTED__', not quoted)
 

doc/osh-quick-ref-toc.txt

@@ -138,7 +138,9 @@ X [Call Stack]    @BASH_SOURCE   @FUNCNAME   @BASH_LINENO
 X [Process Stack] BASH_SUBSHELL   SHLVL
 X [Shell State]   BASH_CMDS   @DIRSTACK
   [Completion]    @COMP_WORDS   COMP_CWORD   COMP_WORDBREAKS
-                  @COMPREPLY
+                  @COMPREPLY   X COMP_KEY   X COMP_LINE   X COMP_POINT
+                  X COMP_TYPE
+
   [cd]            PWD   OLDPWD   X CDPATH
   [getopts]       OPTIND   OPTARG   X OPTERR
   [read]          REPLY   IFS

osh/arith_parse_test.py

@@ -33,12 +33,12 @@ def ParseAndEval(code_str):
   anode = w_parser._ReadArithExpr()  # need the right lex state?
   print('node:', anode)
 
-  mem = state.Mem('', [], {}, None)
+  mem = state.Mem('', [], {}, arena)
   exec_opts = state.ExecOpts(mem, None)
   splitter = legacy.SplitContext(mem)
-  ev = word_eval.CompletionWordEvaluator(mem, exec_opts, splitter)
+  ev = word_eval.CompletionWordEvaluator(mem, exec_opts, splitter, arena)
 
-  arith_ev = expr_eval.ArithEvaluator(mem, exec_opts, ev)
+  arith_ev = expr_eval.ArithEvaluator(mem, exec_opts, ev, arena)
   value = arith_ev.Eval(anode)
   return value
 

scripts/complete.sh

@@ -7,7 +7,7 @@ set -o nounset
 set -o pipefail
 set -o errexit
 
-readonly BASH_COMP=/usr/share/bash-completion/bash_completion.osh
+readonly BASH_COMP=../bash-completion/bash_completion
 
 # This version is too new to run on my Ubuntu machine!  Uses git --list-cmds.
 #readonly GIT_COMP=testdata/completion/git-completion.bash
@@ -44,11 +44,15 @@ audit-git() {
 #        local -A VERBS=(
 #               [STANDALONE]='status list-locales list-keymaps'
 
-audit-distro() {
-  local path=/usr/share/bash-completion/bash_completion
+audit-bashcomp() {
+  local path=$BASH_COMP
+
   audit $path
 
-  find /usr/share/bash-completion/ -type f | xargs grep -E --color ']\+?='
+  # Some of these are not cash variables.
+  grep -E -o 'COMP_[A-Z]+' $path | hist
+
+  #find /usr/share/bash-completion/ -type f | xargs grep -E --color ']\+?='
 }
 
 # Git completion is very big!
@@ -72,7 +76,7 @@ fresh-osh-with-dump() {
 osh-trace() {
   # $FUNCNAME displays the whole stack in osh (unlike bash), but ${FUNCNAME[0]}
   # displays the top.
-  env -i OSH_CRASH_DUMP_DIR=_tmp PS4='+[${LINENO}:${FUNCNAME}] ' \
+  env -i OSH_CRASH_DUMP_DIR=_tmp PS4='+[${LINENO}:${FUNCNAME[0]}] ' \
     bin/osh -x --debug-file _tmp/debug --xtrace-to-debug-file "$@"
 }
 

spec/builtin-printf.test.sh

@@ -0,0 +1,78 @@
+#!/usr/bin/env bash
+#
+# printf
+# bash-completion uses this odd printf -v construction.  It seems to mostly use
+# %s and %q though.
+#
+# %s should just be
+# declare $var='val'
+#
+# NOTE: 
+# /usr/bin/printf %q "'" seems wrong.
+# $ /usr/bin/printf  %q "'"
+# ''\'''
+#
+# I suppose it is technically correct, but it looks very ugly.
+
+#### printf -v %s
+var=foo
+printf -v $var %s 'hello there'
+argv "$foo" 
+## STDOUT:
+['hello there']
+## END
+
+#### printf -v %q
+var=foo
+printf -v $var %q '"quoted" with spaces and \'
+argv "$foo" 
+## STDOUT:
+['\\"quoted\\"\\ with\\ spaces\\ and\\ \\\\']
+## END
+
+#### declare instead of %s
+var=foo
+declare $var='hello there'
+argv "$foo" 
+## STDOUT:
+['hello there']
+## END
+
+#### declare instead of %q
+var=foo
+val='"quoted" with spaces and \'
+# I think this is bash 4.4 only.
+declare $var="${val@Q}"
+argv "$foo" 
+## STDOUT:
+['hello there']
+## END
+
+#### printf -v dynamic scope
+# OK so printf is like assigning to a var.
+# printf -v foo %q "$bar" is like
+# foo=${bar@Q}
+dollar='dollar'
+f() {
+  local mylocal=foo
+  printf -v dollar %q '$'  # assign foo to a quoted dollar
+  printf -v mylocal %q 'mylocal'
+  echo dollar=$dollar
+  echo mylocal=$mylocal
+}
+echo dollar=$dollar
+echo --
+f
+echo --
+echo dollar=$dollar
+echo mylocal=$mylocal
+## STDOUT:
+dollar=dollar
+--
+dollar=\$
+mylocal=mylocal
+--
+dollar=\$
+mylocal=
+## END
+

spec/builtin-test.test.sh

@@ -261,3 +261,14 @@ not newer
 not older than itself
 not newer than itself
 ## END
+
+#### [ a -eq b ]
+[ a -eq a ]
+echo status=$?
+## STDOUT:
+status=2
+## END
+## BUG mksh STDOUT:
+status=0
+## END
+

spec/dbracket.test.sh

@@ -105,7 +105,8 @@ hex=0x0f    # = 15 (decimal)
 
 # TODO: Add tests for this
 # https://www.gnu.org/software/bash/manual/bash.html#Bash-Conditional-Expressions
-# When used with [[, the ‘<’ and ‘>’ operators sort lexicographically using the current locale. The test command uses ASCII ordering.
+# When used with [[, the ‘<’ and ‘>’ operators sort lexicographically using the
+# current locale. The test command uses ASCII ordering.
 
 #### > on strings
 # NOTE: < doesn't need space, even though == does?  That's silly.

test/spec-runner.sh

@@ -25,6 +25,9 @@ _spec-manifest() {
     # Nothing passing here
     if (name == "extended-glob") next;
 
+    # For testing printf -v, which I do not want to implement.
+    if (name == "builtin-printf") next;
+
     # This was meant for ANTLR.
     if (name == "shell-grammar") next;
 

test/spec.sh

@@ -300,6 +300,10 @@ builtin-io() {
     ${REF_SHELLS[@]} $ZSH $BUSYBOX_ASH $OSH_LIST "$@"
 }
 
+builtin-printf() {
+  sh-spec spec/builtin-printf.test.sh $BASH $OSH_LIST "$@"
+}
+
 builtins2() {
   sh-spec spec/builtins2.test.sh ${REF_SHELLS[@]} $ZSH $OSH_LIST "$@"
 }