[completion] Fix two sources of infinite loops.

- The basename() test in GetSpecForFirst() was broken, which caused
  an infinite loop when running ~/.local/bin/mypy.  Fixed with unit
  test.
- The 124 protocol now tests if the completion spec was changed before
  retrying.  This prevents a function that simply does 'return 124' from
  entering an infinite loop.  The unit tests have coverage for this case
  via 'return-124.bash'.

Addresses issue #208.

bin/oil.py

@@ -392,7 +392,9 @@ def ShellMain(lang, argv0, argv, login_shell):
   ex.bool_ev = bool_ev
   ex.tracer = tracer
 
-  spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter)
+  spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter,
+                                          comp_lookup)
+
   # Add some builtins that depend on the executor!
   complete_builtin = builtin_comp.Complete(spec_builder, comp_lookup)
   builtins[builtin_e.COMPLETE] = complete_builtin

core/completion.py

@@ -146,6 +146,8 @@ def __init__(self):
         '__first': _DO_NOTHING,
     }
 
+    self.commands_with_spec_changes = []  # for the 124 protocol
+
     # So you can register *.sh, unlike bash.  List of (glob, [actions]),
     # searched linearly.
     self.patterns = []
@@ -162,12 +164,21 @@ def PrintSpecs(self):
     for pat, spec in self.patterns:
       print('%s = %s' % (pat, spec))
 
+  def ClearCommandsChanged(self):
+    del self.commands_with_spec_changes[:]
+
+  def GetCommandsChanged(self):
+    return self.commands_with_spec_changes
+
   def RegisterName(self, name, base_opts, user_spec):
     """Register a completion action with a name.
     Used by the 'complete' builtin.
     """
     self.lookup[name] = (base_opts, user_spec)
 
+    if name not in ('__fallback', '__first'):
+      self.commands_with_spec_changes.append(name)
+
   def RegisterGlob(self, glob_pat, base_opts, user_spec):
     self.patterns.append((glob_pat, base_opts, user_spec))
 
@@ -181,7 +192,7 @@ def GetSpecForName(self, argv0):
       return pair
 
     key = os_path.basename(argv0)
-    actions = self.lookup.get(key)
+    pair = self.lookup.get(key)
     if pair:
       return pair
 
@@ -353,9 +364,15 @@ def Matches(self, comp):
 class ShellFuncAction(CompletionAction):
   """Call a user-defined function using bash's completion protocol."""
 
-  def __init__(self, ex, func):
+  def __init__(self, ex, func, comp_lookup):
+    """
+    Args:
+      comp_lookup: For the 124 protocol: test if the user-defined function
+      registered a new UserSpec.
+    """
     self.ex = ex
     self.func = func
+    self.comp_lookup = comp_lookup
 
   def __repr__(self):
     # TODO: Add file and line number here!
@@ -392,10 +409,24 @@ def Matches(self, comp):
     self.log('Running completion function %r with arguments %s',
              self.func.name, argv)
 
+    self.comp_lookup.ClearCommandsChanged()
     status = self.ex.RunFuncForCompletion(self.func, argv)
+    commands_changed = self.comp_lookup.GetCommandsChanged()
+
+    self.log('comp.first %s, commands_changed: %s', comp.first,
+             commands_changed)
+
     if status == 124:
-      self.log('Got status 124 from %r', self.func.name)
-      raise _RetryCompletion()
+      cmd = os_path.basename(comp.first) 
+      if cmd in commands_changed:
+        self.log('Got status 124 from %r and %s commands changed',
+                 self.func.name, commands_changed)
+        raise _RetryCompletion()
+      else:
+        util.error(
+            "Function %r returned 124, but the completion spec for %r wasn't "
+            "changed", self.func.name, cmd)
+        return []
 
     # Read the response.  We set it above, so this error would only happen if
     # the user unset it.

core/completion_test.py

@@ -107,8 +107,12 @@ def _MakeComp(self, words, index, to_complete):
   def testLookup(self):
     c = completion.Lookup()
     c.RegisterName('grep', BASE_OPTS, U1)
-    print(c.GetSpecForName('grep'))
-    print(c.GetSpecForName('/usr/bin/grep'))
+
+    _, user_spec = c.GetSpecForName('grep')
+    self.assertEqual(1, len(user_spec.actions))
+
+    _, user_spec = c.GetSpecForName('/usr/bin/grep')
+    self.assertEqual(1, len(user_spec.actions))
 
     c.RegisterGlob('*.py', BASE_OPTS, U1)
     base_opts, comp = c.GetSpecForName('/usr/bin/foo.py')
@@ -189,7 +193,8 @@ def testShellFuncExecution(self):
 
     ex = test_lib.InitExecutor(arena=arena)
 
-    a = completion.ShellFuncAction(ex, func_node)
+    comp_lookup = completion.Lookup()
+    a = completion.ShellFuncAction(ex, func_node, comp_lookup)
     comp = self._MakeComp(['f'], 0, 'f')
     matches = list(a.Matches(comp))
     self.assertEqual(['f1', 'f2'], matches)
@@ -489,6 +494,30 @@ def testCompletesAliases(self):
     m = list(r.Matches(MockApi('ll_own_completion ')))
     self.assertEqual(['own ', 'words '], sorted(m))
 
+  def testNoInfiniteLoop(self):
+    # This was ONE place where we got an infinite loop.
+
+    with open('testdata/completion/return-124.bash') as f:
+      code_str = f.read()
+    trail = parse_lib.Trail()
+    arena = alloc.SideArena('<completion_test.py>')
+    parse_ctx = parse_lib.ParseContext(arena, {}, trail=trail)
+    comp_lookup = completion.Lookup()
+    ex = test_lib.EvalCode(code_str, parse_ctx, comp_lookup=comp_lookup)
+
+    r = _MakeRootCompleter(parse_ctx=parse_ctx, comp_lookup=comp_lookup)
+
+    m = list(r.Matches(MockApi('bad ')))
+    self.assertEqual([], sorted(m))
+
+    # Error: spec not changed
+    m = list(r.Matches(MockApi('both ')))
+    self.assertEqual([], sorted(m))
+
+    # Redefines completions
+    m = list(r.Matches(MockApi('both2 ')))
+    self.assertEqual(['b1 ', 'b2 '], sorted(m))
+
   def testCompletesAssignment(self):
     # OSH doesn't do this.  Here is noticed about bash --norc (which is
     # undoubtedly different from bash_completion):

core/test_lib.py

@@ -170,7 +170,8 @@ def InitExecutor(parse_ctx=None, comp_lookup=None, arena=None, mem=None):
   ex = cmd_exec.Executor(mem, fd_state, funcs, builtins, exec_opts,
                          parse_ctx, exec_deps)
 
-  spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter)
+  spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter,
+                                          comp_lookup)
   # Add some builtins that depend on the executor!
   complete_builtin = builtin_comp.Complete(spec_builder, comp_lookup)  # used later
   builtins[builtin_e.COMPLETE] = complete_builtin

osh/builtin_comp.py

@@ -91,7 +91,7 @@ def Matches(self, comp):
 
 class SpecBuilder(object):
 
-  def __init__(self, ex, parse_ctx, word_ev, splitter):
+  def __init__(self, ex, parse_ctx, word_ev, splitter, comp_lookup):
     """
     Args:
       ex: Executor for compgen -F
@@ -101,6 +101,7 @@ def __init__(self, ex, parse_ctx, word_ev, splitter):
     self.parse_ctx = parse_ctx
     self.word_ev = word_ev
     self.splitter = splitter
+    self.comp_lookup = comp_lookup
 
   def Build(self, argv, arg, base_opts):
     """Given flags to complete/compgen, return a UserSpec."""
@@ -115,7 +116,7 @@ def Build(self, argv, arg, base_opts):
       func = ex.funcs.get(func_name)
       if func is None:
         raise args.UsageError('Function %r not found' % func_name)
-      actions.append(completion.ShellFuncAction(ex, func))
+      actions.append(completion.ShellFuncAction(ex, func, self.comp_lookup))
 
     # NOTE: We need completion for -A action itself!!!  bash seems to have it.
     for name in arg.actions:

testdata/completion/return-124.bash

@@ -0,0 +1,40 @@
+_bad() {
+  #argv "$@"
+
+  echo '_bad returning 124'
+
+  # This caused an infinite loop in OSH, but not in bash.  We have to test if
+  # the return value is 124 AND the compspec was updated.
+  #
+  # In bash, it seems like you EITHER set COMPREPLY or return 124, not BOTH!
+  # If it sees 124, it doesn't process the completions (unlike OSH at the
+  # moment).
+
+  #COMPREPLY=(x y)
+
+  return 124
+}
+complete -F _bad bad
+
+_both() {
+  #echo '_both setting COMPREPLY and returning 124'
+  COMPREPLY=(x y)
+  return 124
+}
+complete -F _both both
+
+_both2() {
+  #echo '_both setting COMPREPLY and returning 124'
+  COMPREPLY=(x y)
+  complete -W 'b1 b2' both2
+  return 124
+}
+complete -F _both2 both2
+
+_default() {
+  echo '_default returning 124 without changing completion spec'
+  # We're supposed to source something here, but we didn't
+  return 124
+}
+
+complete -F _default -D