From bd31559bc71b9a89201803e4962fab3be8b7f7bd Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Fri, 20 Dec 2019 15:11:49 +0100 Subject: [PATCH 1/4] [lldb/pexpect] Force-set the TERM environment variable In some environments (typically, buildbots), this variable may not be available. This can cause tests to behave differently. Explicitly set the variable to "vt100" to ensure consistent test behavior. It should not matter that we do not inherit the process TERM variable, as the child process runs in a new virtual terminal anyway. --- lldb/packages/Python/lldbsuite/test/lldbpexpect.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lldb/packages/Python/lldbsuite/test/lldbpexpect.py b/lldb/packages/Python/lldbsuite/test/lldbpexpect.py index 402148a9534fd1..29a04ea947d2dd 100644 --- a/lldb/packages/Python/lldbsuite/test/lldbpexpect.py +++ b/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,6 +31,7 @@ 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] @@ -37,9 +39,13 @@ def launch(self, executable=None, extra_args=None, timeout=30, dimensions=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) From 0f32e4dadd9e44b2c648f8dd97d72f72eec676a2 Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Mon, 27 Jan 2020 21:19:33 +0100 Subject: [PATCH 2/4] [lldb] Fix that CompletionRequestGetRawLineUntilCursor returns the wrong result By using m_cursor_index and not m_raw_cursor_pos in this function we return the characters until the argument index (not the character index). This means that for "foo" (cursor at the end of the string) this returns "" instead of "foo" (as the argument index is 0 for the first argument and not 4 which is the character index at the end). This lead to a crash in the REPL completion where it would cause that we would complete the string "ABC" to " " as we think the line is actually empty and we want to provide the indentation string. Note that the function has already been deleted upstream so this bug doesn't exist there. --- lldb/include/lldb/Utility/CompletionRequest.h | 2 +- lldb/unittests/Utility/CompletionRequestTest.cpp | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lldb/include/lldb/Utility/CompletionRequest.h b/lldb/include/lldb/Utility/CompletionRequest.h index 28ac78a875659a..ca045139946e06 100644 --- a/lldb/include/lldb/Utility/CompletionRequest.h +++ b/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; } diff --git a/lldb/unittests/Utility/CompletionRequestTest.cpp b/lldb/unittests/Utility/CompletionRequestTest.cpp index 54bd3429921941..afeba875f40f45 100644 --- a/lldb/unittests/Utility/CompletionRequestTest.cpp +++ b/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"); From b8ea404bb28a3c096fd6ec80c2b06d641d77bdcf Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Mon, 27 Jan 2020 21:24:39 +0100 Subject: [PATCH 3/4] [lldb] Move REPL::CompleteCode interface to use CompletionRequest --- lldb/include/lldb/Expression/REPL.h | 4 ++-- lldb/source/Expression/REPL.cpp | 9 +-------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/lldb/include/lldb/Expression/REPL.h b/lldb/include/lldb/Expression/REPL.h index d34a792f58f1cc..ba5655a541130f 100644 --- a/lldb/include/lldb/Expression/REPL.h +++ b/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 ¤t_code, - StringList &matches) = 0; + virtual void CompleteCode(const std::string ¤t_code, + CompletionRequest &request) = 0; OptionGroupFormat m_format_options = OptionGroupFormat(lldb::eFormatDefault); OptionGroupValueObjectDisplay m_varobj_options; diff --git a/lldb/source/Expression/REPL.cpp b/lldb/source/Expression/REPL.cpp index 751fdb9665d950..9518590d90ca49 100644 --- a/lldb/source/Expression/REPL.cpp +++ b/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) { From 28f284d23dcf73b0e5eb2df83b62fd9608cc3030 Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Mon, 27 Jan 2020 21:25:15 +0100 Subject: [PATCH 4/4] [lldb] Rewrite Swift REPL completion to use CompletionRequest This removes all the workaround and problematic code that was needed for the old interface. With the CompletionRequest we no longer need to calculate the common prefix ourselves and we don't need to calculate the magic return value. On the other hand we can no longer provide 'appendix' completions (E.g. completing "Suff" with "ix", instead we now need to provide the completion "Suffix"), so in one case we now need to prefix the completion with the existing prefix from the CompletionRequest (as Swift only returns the string to append and not the whole token). Also rewrites the TestSwiftREPLCompletion.py to test all the problematic cases and some more. --- .../completion/TestSwiftREPLCompletion.py | 48 ++++++++++++----- .../ExpressionParser/Swift/SwiftREPL.cpp | 54 ++++++++++--------- .../ExpressionParser/Swift/SwiftREPL.h | 4 +- 3 files changed, 64 insertions(+), 42 deletions(-) diff --git a/lldb/packages/Python/lldbsuite/test/lang/swift/completion/TestSwiftREPLCompletion.py b/lldb/packages/Python/lldbsuite/test/lang/swift/completion/TestSwiftREPLCompletion.py index 66024815cd0dcc..eb776440fbb6db 100644 --- a/lldb/packages/Python/lldbsuite/test/lang/swift/completion/TestSwiftREPLCompletion.py +++ b/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 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. diff --git a/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp b/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp index 3e13ae24bbebee..00566e55ecb34b 100644 --- a/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp +++ b/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 ¤t_code, - lldb_private::StringList &matches) { +void SwiftREPL::CompleteCode(const std::string ¤t_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 ¤t_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 ¤t_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_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(); } diff --git a/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.h b/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.h index 034366a806ce51..9422ed24cffeee 100644 --- a/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.h +++ b/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 ¤t_code, - StringList &matches) override; + void CompleteCode(const std::string ¤t_code, + CompletionRequest &request) override; public: static bool classof(const REPL *repl) {