[lldb] Rewrite Swift completion logic in REPL to not crash [WIP]

lldb/include/lldb/Expression/REPL.h

@@ -130,8 +130,8 @@ class REPL : public IOHandlerDelegate {
                                 lldb::ValueObjectSP &valobj_sp,
                                 ExpressionVariable *var = nullptr) = 0;
 
-  virtual int CompleteCode(const std::string &current_code,
-                           StringList &matches) = 0;
+  virtual void CompleteCode(const std::string &current_code,
+                            CompletionRequest &request) = 0;
 
   OptionGroupFormat m_format_options = OptionGroupFormat(lldb::eFormatDefault);
   OptionGroupValueObjectDisplay m_varobj_options;

lldb/include/lldb/Utility/CompletionRequest.h

@@ -104,7 +104,7 @@ class CompletionRequest {
 
   llvm::StringRef GetRawLine() const { return m_command; }
   llvm::StringRef GetRawLineUntilCursor() const {
-    return m_command.substr(0, m_cursor_index);
+    return m_command.substr(0, m_raw_cursor_pos);
   }
 
   unsigned GetRawCursorPos() const { return m_raw_cursor_pos; }

lldb/packages/Python/lldbsuite/test/lang/swift/completion/TestSwiftREPLCompletion.py

@@ -1,22 +1,42 @@
-from __future__ import print_function
-import pexpect
-import os
+
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
+from lldbsuite.test.lldbpexpect import PExpectTest
 
+class SwiftCompletionTest(PExpectTest):
 
-class TestSwiftREPLCompletion(TestBase):
     mydir = TestBase.compute_mydir(__file__)
 
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
     @skipUnlessDarwin
-    def test_repl_completion(self):
-        prompt = "Welcome to"
-        child = pexpect.spawn('%s --repl' % (lldbtest_config.lldbExec))
-        # Assign to make sure the sessions are killed during teardown
-        self.child = child
-        # Send a <TAB> and make sure we don't crash.
-        child.sendline("import Foundatio\t")
-        child.sendline("print(NSString(\"patatino\"))")
-        child.expect("patatino")
+    def test_basic_completion(self):
+
+        self.launch(extra_args=["--repl"], executable=None, dimensions=(100,500))
+
+        # Wait on the first prompt
+        self.child.expect_exact("1>")
+        # Press tab a few times which should do nothing.
+        # Note that we don't get any indentation whitespace as
+        # pexpect is not recognized as a interactive terminal by pexpect it seems.
+        self.child.send("\t\t\t")
+
+        # Try completing something that only has one result "Hasabl" -> "Hashable".
+        self.child.send("Hashabl\t")
+        self.child.expect_exact("Hashable")
+        self.child.sendline("")
+
+        # Try completing something that has multiple completions.
+        self.child.send("Hash\t")
+        self.child.expect_exact("Available completions:")
+        self.child.expect_exact("Hashable")
+        self.child.expect_exact("Hasher")
+        self.child.sendline("")
+
+    def setUpCommands(self):
+        return [] # REPL doesn't take any setup commands.
+
+    def expect_prompt(self):
+        pass # No constant prompt on the REPL.

lldb/packages/Python/lldbsuite/test/lldbpexpect.py

@@ -2,6 +2,7 @@
 from __future__ import absolute_import
 
 # System modules
+import os
 import sys
 
 # Third-party modules
@@ -30,16 +31,21 @@ def expect_prompt(self):
         def launch(self, executable=None, extra_args=None, timeout=30, dimensions=None):
             logfile = getattr(sys.stdout, 'buffer',
                               sys.stdout) if self.TraceOn() else None
+
             args = ['--no-lldbinit', '--no-use-colors']
             for cmd in self.setUpCommands():
                 args += ['-O', cmd]
             if executable is not None:
                 args += ['--file', executable]
             if extra_args is not None:
                 args.extend(extra_args)
+
+            env = dict(os.environ)
+            env["TERM"]="vt100"
+
             self.child = pexpect.spawn(
                     lldbtest_config.lldbExec, args=args, logfile=logfile,
-                    timeout=timeout, dimensions=dimensions)
+                    timeout=timeout, dimensions=dimensions, env=env)
             self.expect_prompt()
             for cmd in self.setUpCommands():
                 self.child.expect_exact(cmd)

lldb/source/Expression/REPL.cpp

@@ -501,14 +501,7 @@ void REPL::IOHandlerComplete(IOHandler &io_handler,
   current_code.append("\n");
   current_code += request.GetRawLineUntilCursor();
 
-  StringList matches;
-  int result = CompleteCode(current_code, matches);
-  if (result == -2) {
-    assert(matches.GetSize() == 1);
-    request.AddCompletion(matches.GetStringAtIndex(0), "",
-                          CompletionMode::RewriteLine);
-  } else
-    request.AddCompletions(matches);
+  CompleteCode(current_code, request);
 }
 
 bool QuitCommandOverrideCallback(void *baton, const char **argv) {

lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp

@@ -533,8 +533,8 @@ bool SwiftREPL::PrintOneVariable(Debugger &debugger, StreamFileSP &output_sp,
   return handled;
 }
 
-int SwiftREPL::CompleteCode(const std::string &current_code,
-                            lldb_private::StringList &matches) {
+void SwiftREPL::CompleteCode(const std::string &current_code,
+                             CompletionRequest &request) {
   //----------------------------------------------------------------------g
   // If we use the target's SwiftASTContext for completion, it reaaallly
   // slows down subsequent expressions. The compiler team doesn't have time
@@ -546,7 +546,7 @@ int SwiftREPL::CompleteCode(const std::string &current_code,
     auto type_system_or_err = m_target.GetScratchTypeSystemForLanguage(eLanguageTypeSwift);
     if (!type_system_or_err) {
       llvm::consumeError(type_system_or_err.takeError());
-      return 0;
+      return;
     }
 
     auto *target_swift_ast =
@@ -579,54 +579,56 @@ int SwiftREPL::CompleteCode(const std::string &current_code,
       swift::SourceFile &repl_source_file =
           repl_module->getMainSourceFile(swift::SourceFileKind::REPL);
 
+      // Swift likes to give us strings to append to the current token but
+      // the CompletionRequest requires a replacement for the full current
+      // token. Fix this by getting the current token here and we attach
+      // the suffix we get from Swift.
+      std::string prefix = request.GetCursorArgumentPrefix();
       llvm::StringRef current_code_ref(current_code);
       completions.populate(repl_source_file, current_code_ref);
+
+      // The root is the unique completion we need to use, so let's add it
+      // to the completion list. As the completion is unique we can stop here.
       llvm::StringRef root = completions.getRoot();
       if (!root.empty()) {
-        matches.AppendString(root.data(), root.size());
-        return 1;
+        request.AddCompletion(prefix + root.str(), "", CompletionMode::Partial);
+        return;
       }
+
       // Otherwise, advance through the completion state machine.
       const swift::CompletionState completion_state = completions.getState();
       switch (completion_state) {
       case swift::CompletionState::CompletedRoot: {
-        // We completed the root. Next step is to display the completion list.
-        matches.AppendString(""); // Empty string to indicate no completion,
-        // just display other strings that come after it
+        // Display the completion list.
         llvm::ArrayRef<llvm::StringRef> llvm_matches =
             completions.getCompletionList();
         for (const auto &llvm_match : llvm_matches) {
+          // The completions here aren't really useful for actually completing
+          // the token but are more descriptive hints for the user
+          // (e.g. "isMultiple(of: Int) -> Bool"). They aren't useful for
+          // actually completing anything so let's use the current token as
+          // a placeholder that is always valid.
           if (!llvm_match.empty())
-            matches.AppendString(llvm_match.data(), llvm_match.size());
+            request.AddCompletion(prefix, llvm_match);
         }
-        // Don't include the empty string we appended above or we will display
-        // one
-        // too many we need to return the magical value of one less than our
-        // actual matches.
-        // TODO: modify all IOHandlerDelegate::IOHandlerComplete() to use a
-        // CompletionMatches
-        // class that wraps up the "StringList matches;" along with other smarts
-        // so we don't
-        // have to return magic values and incorrect sizes.
-        return matches.GetSize() - 1;
       } break;
 
       case swift::CompletionState::DisplayedCompletionList: {
         // Complete the next completion stem in the cycle.
-        llvm::StringRef stem = completions.getPreviousStem().InsertableString;
-        matches.AppendString(stem.data(), stem.size());
+        request.AddCompletion(prefix + completions.getPreviousStem().InsertableString.str());
       } break;
 
       case swift::CompletionState::Empty:
-      case swift::CompletionState::Unique:
-        // We already provided a definitive completion--nothing else to do.
-        break;
+      case swift::CompletionState::Unique: {
+        llvm::StringRef root = completions.getRoot();
+
+        if (!root.empty())
+          request.AddCompletion(prefix + root.str());
+      } break;
 
       case swift::CompletionState::Invalid:
         llvm_unreachable("got an invalid completion set?!");
       }
     }
   }
-
-  return matches.GetSize();
 }

lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.h

@@ -68,8 +68,8 @@ class SwiftREPL : public REPL {
                         lldb::ValueObjectSP &valobj_sp,
                         ExpressionVariable *var = nullptr) override;
 
-  int CompleteCode(const std::string &current_code,
-                   StringList &matches) override;
+  void CompleteCode(const std::string &current_code,
+                    CompletionRequest &request) override;
 
 public:
   static bool classof(const REPL *repl) {

lldb/unittests/Utility/CompletionRequestTest.cpp

@@ -26,6 +26,7 @@ TEST(CompletionRequest, Constructor) {
   EXPECT_EQ(request.GetRawCursorPos(), cursor_pos);
   EXPECT_EQ(request.GetCursorIndex(), arg_index);
   EXPECT_EQ(request.GetCursorCharPosition(), arg_cursor_pos);
+  EXPECT_EQ(request.GetRawLineUntilCursor(), "a b");
 
   EXPECT_EQ(request.GetPartialParsedLine().GetArgumentCount(), 2u);
   EXPECT_STREQ(request.GetPartialParsedLine().GetArgumentAtIndex(1), "b");