llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) <details> <summary>Changes</summary> Previously, completion behavior was inconsistent, sometimes including the partial token or removing existing user text. Since LLDB completions includes the partial token by default, we now strip it before sending to the client. The completion heuristic: 1. Strip the commandEscapePrefix 2. Request completions from the debugger 3. Get the line at cursor position 4. Calculate the length of any partial token 5. Offset each completion by the partial token length In all cases it completion starts from the cursor position. then offsets by `Length` and inserts the completion. Examples (single quotes show whitespace and are not part of the input): ```md | Input | Partial Token | Length | Completion | |------------|---------------|--------|---------------| | m | m | 1 | memory | | `m | m | 1 | memory | | myfoo. | myfoo. | 6 | myfoo.method( | | myfoo.fi | myfoo.fi | 7 | myfoo.field | | myfoo. b | b | 1 | myfoo. bar | | 'memory ' | | 0 | memory read | | memory | memory | 6 | memory | | settings s | s | 1 | settings show | ``` Fixes #<!-- -->176424 --- Patch is 20.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/177151.diff 3 Files Affected: - (modified) lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py (+219-120) - (modified) lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp (+12-9) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.h (+4-4) ``````````diff diff --git a/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py b/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py index 1792ff9953efe..937904d682bb0 100644 --- a/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py +++ b/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py @@ -2,47 +2,91 @@ Test lldb-dap completions request """ +# FIXME: remove when LLDB_MINIMUM_PYTHON_VERSION > 3.8 +from __future__ import annotations + +import json +from typing import Optional import lldbdap_testcase -import dap_server -from lldbsuite.test import lldbutil +from dataclasses import dataclass, replace, asdict from lldbsuite.test.decorators import skipIf from lldbsuite.test.lldbtest import line_number -session_completion = { - "text": "session", - "label": "session", - "detail": "Commands controlling LLDB session.", -} -settings_completion = { - "text": "settings", - "label": "settings", - "detail": "Commands for managing LLDB settings.", -} -memory_completion = { - "text": "memory", - "label": "memory", - "detail": "Commands for operating on memory in the current target process.", -} -command_var_completion = { - "text": "var", - "label": "var", - "detail": "Show variables for the current stack frame. Defaults to all arguments and local variables in scope. Names of argument, local, file static and file global variables can be specified.", -} -variable_var_completion = {"text": "var", "label": "var", "detail": "vector<baz> &"} -variable_var1_completion = {"text": "var1", "label": "var1", "detail": "int &"} -variable_var2_completion = {"text": "var2", "label": "var2", "detail": "int &"} + +@dataclass(frozen=True) +class CompletionItem: + label: str + text: Optional[str] = None + detail: Optional[str] = None + start: Optional[int] = None + length: int = 0 + + def __repr__(self): + # use json as it easier to see the diff on failure. + return json.dumps(asdict(self), indent=4) + + def clone(self, **kwargs) -> CompletionItem: + """Creates a copy of this CompletionItem with specified fields modified.""" + return replace(self, **kwargs) + + +@dataclass +class TestCase: + input: str + expected: set[CompletionItem] + not_expected: Optional[set[CompletionItem]] = None + + +session_completion = CompletionItem( + label="session", + detail="Commands controlling LLDB session.", +) +settings_completion = CompletionItem( + label="settings", + detail="Commands for managing LLDB settings.", +) +memory_completion = CompletionItem( + label="memory", + detail="Commands for operating on memory in the current target process.", +) +command_var_completion = CompletionItem( + label="var", + detail="Show variables for the current stack frame. Defaults to all arguments and local variables in scope. Names of argument, local, file static and file global variables can be specified.", + length=3, +) +variable_var_completion = CompletionItem(label="var", detail="vector<baz> &", length=3) +variable_var1_completion = CompletionItem(label="var1", detail="int &") +variable_var2_completion = CompletionItem(label="var2", detail="int &") + +str1_completion = CompletionItem( + label="str1", + detail="std::string &", +) # Older version of libcxx produce slightly different typename strings for # templates like vector. @skipIf(compiler="clang", compiler_version=["<", "16.0"]) class TestDAP_completions(lldbdap_testcase.DAPTestCaseBase): - def verify_completions(self, actual_list, expected_list, not_expected_list=[]): - for expected_item in expected_list: - self.assertIn(expected_item, actual_list) - - for not_expected_item in not_expected_list: - self.assertNotIn(not_expected_item, actual_list) + def verify_completions(self, case: TestCase): + completions = { + CompletionItem(**comp) + for comp in self.dap_server.get_completions(case.input) + } + + # handle expected completions + expected_completions = case.expected + for exp_comp in expected_completions: + # with self.subTest(f"Expected completion : {exp_comp}"): + self.assertIn( + exp_comp, completions, f"\nCompletion for input: {case.input}" + ) + + # unexpected completions + not_expected_label = case.not_expected or set() + for not_exp_comp in not_expected_label: + with self.subTest(f"Not expected completion : {not_exp_comp}"): + self.assertNotIn(not_exp_comp, completions) def setup_debuggee(self): program = self.getBuildArtifact("a.out") @@ -70,72 +114,96 @@ def test_command_completions(self): # Provides completion for top-level commands self.verify_completions( - self.dap_server.get_completions("se"), - [session_completion, settings_completion], + TestCase( + input="se", + expected={ + session_completion.clone(length=2), + settings_completion.clone(length=2), + }, + ) ) - # Provides completions for sub-commands self.verify_completions( - self.dap_server.get_completions("memory "), - [ - { - "text": "read", - "label": "read", - "detail": "Read from the memory of the current target process.", - }, - { - "text": "region", - "label": "region", - "detail": "Get information on the memory region containing an address in the current target process.", + TestCase( + input="memory ", + expected={ + CompletionItem( + label="read", + detail="Read from the memory of the current target process.", + ), + CompletionItem( + label="region", + detail="Get information on the memory region containing an address in the current target process.", + ), }, - ], + ) ) # Provides completions for parameter values of commands self.verify_completions( - self.dap_server.get_completions("`log enable "), - [{"text": "gdb-remote", "label": "gdb-remote"}], + TestCase( + input="`log enable ", expected={CompletionItem(label="gdb-remote")} + ) ) # Also works if the escape prefix is used self.verify_completions( - self.dap_server.get_completions("`mem"), [memory_completion] + TestCase(input="`mem", expected={memory_completion.clone(length=3)}) ) self.verify_completions( - self.dap_server.get_completions("`"), - [session_completion, settings_completion, memory_completion], + TestCase( + input="`", + expected={ + session_completion.clone(), + settings_completion.clone(), + memory_completion.clone(), + }, + ) ) # Completes an incomplete quoted token self.verify_completions( - self.dap_server.get_completions('setting "se'), - [ - { - "text": "set", - "label": "set", - "detail": "Set the value of the specified debugger setting.", - } - ], + TestCase( + input='setting "se', + expected={ + CompletionItem( + label="set", + detail="Set the value of the specified debugger setting.", + length=3, + ) + }, + ) ) # Completes an incomplete quoted token self.verify_completions( - self.dap_server.get_completions("'mem"), - [memory_completion], + TestCase(input="'mem", expected={memory_completion.clone(length=4)}) ) # Completes expressions with quotes inside self.verify_completions( - self.dap_server.get_completions('expr " "; typed'), - [{"text": "typedef", "label": "typedef"}], + TestCase( + input='expr " "; typed', + expected={CompletionItem(label="typedef", length=5)}, + ) ) # Provides completions for commands, but not variables self.verify_completions( - self.dap_server.get_completions("var"), - [command_var_completion], - [variable_var_completion], + TestCase( + input="var", + expected={command_var_completion.clone()}, + not_expected={variable_var_completion.clone()}, + ) + ) + + # Completes partial completion + self.verify_completions( + TestCase( + input="plugin list ar", + expected={CompletionItem(label="architecture", length=2)}, + ) ) def test_variable_completions(self): @@ -152,26 +220,32 @@ def test_variable_completions(self): # Provides completions for varibles, but not command self.verify_completions( - self.dap_server.get_completions("var"), - [variable_var_completion], - [command_var_completion], + TestCase( + input="var", + expected={variable_var_completion.clone()}, + not_expected={command_var_completion.clone()}, + ) ) # We stopped inside `fun`, so we shouldn't see variables from main self.verify_completions( - self.dap_server.get_completions("var"), - [variable_var_completion], - [ - variable_var1_completion, - variable_var2_completion, - ], + TestCase( + input="var", + expected={variable_var_completion}, + not_expected={ + variable_var1_completion.clone(length=3), + variable_var2_completion.clone(length=3), + }, + ) ) # We should see global keywords but not variables inside main self.verify_completions( - self.dap_server.get_completions("str"), - [{"text": "struct", "label": "struct"}], - [{"text": "str1", "label": "str1", "detail": "std::string &"}], + TestCase( + input="str", + expected={CompletionItem(label="struct", length=3)}, + not_expected={str1_completion.clone(length=3)}, + ) ) self.continue_to_next_stop() @@ -179,65 +253,86 @@ def test_variable_completions(self): # We stopped in `main`, so we should see variables from main but # not from the other function self.verify_completions( - self.dap_server.get_completions("var"), - [ - variable_var1_completion, - variable_var2_completion, - ], - [ - variable_var_completion, - ], + TestCase( + input="var", + expected={ + variable_var1_completion.clone(length=3), + variable_var2_completion.clone(length=3), + }, + not_expected={ + variable_var_completion.clone(length=3), + }, + ) ) self.verify_completions( - self.dap_server.get_completions("str"), - [ - {"text": "struct", "label": "struct"}, - {"text": "str1", "label": "str1", "detail": "std::string &"}, - ], + TestCase( + input="str", + expected={ + CompletionItem(label="struct", length=3), + str1_completion.clone(length=3), + }, + ) ) self.assertIsNotNone(self.dap_server.get_completions("ƒ")) # Test utf8 after ascii. + # TODO self.dap_server.get_completions("mƒ") # Completion also works for more complex expressions self.verify_completions( - self.dap_server.get_completions("foo1.v"), - [{"text": "var1", "label": "foo1.var1", "detail": "int"}], + TestCase( + input="foo1.v", + expected={CompletionItem(label="foo1.var1", detail="int", length=6)}, + ) ) self.verify_completions( - self.dap_server.get_completions("foo1.my_bar_object.v"), - [{"text": "var1", "label": "foo1.my_bar_object.var1", "detail": "int"}], + TestCase( + input="foo1.my_bar_object.v", + expected={ + CompletionItem( + label="foo1.my_bar_object.var1", detail="int", length=20 + ) + }, + ) ) self.verify_completions( - self.dap_server.get_completions("foo1.var1 + foo1.v"), - [{"text": "var1", "label": "foo1.var1", "detail": "int"}], + TestCase( + input="foo1.var1 + foo1.v", + expected={CompletionItem(label="foo1.var1", detail="int", length=6)}, + ) ) self.verify_completions( - self.dap_server.get_completions("foo1.var1 + v"), - [{"text": "var1", "label": "var1", "detail": "int &"}], + TestCase( + input="foo1.var1 + v", + expected={CompletionItem(label="var1", detail="int &", length=1)}, + ) ) # should correctly handle spaces between objects and member operators self.verify_completions( - self.dap_server.get_completions("foo1 .v"), - [{"text": "var1", "label": ".var1", "detail": "int"}], - [{"text": "var2", "label": ".var2", "detail": "int"}], + TestCase( + input="foo1 .v", + expected={CompletionItem(label=".var1", detail="int", length=2)}, + not_expected={CompletionItem(label=".var2", detail="int", length=2)}, + ) ) self.verify_completions( - self.dap_server.get_completions("foo1 . v"), - [{"text": "var1", "label": "var1", "detail": "int"}], - [{"text": "var2", "label": "var2", "detail": "int"}], + TestCase( + input="foo1 . v", + expected={CompletionItem(label="var1", detail="int", length=1)}, + not_expected={CompletionItem(label="var2", detail="int", length=1)}, + ) ) # Even in variable mode, we can still use the escape prefix self.verify_completions( - self.dap_server.get_completions("`mem"), [memory_completion] + TestCase(input="`mem", expected={memory_completion.clone(length=3)}) ) def test_auto_completions(self): @@ -261,24 +356,28 @@ def test_auto_completions(self): # We are stopped inside `main`. Variables `var1` and `var2` are in scope. # Make sure, we offer all completions self.verify_completions( - self.dap_server.get_completions("va"), - [ - command_var_completion, - variable_var1_completion, - variable_var2_completion, - ], + TestCase( + input="va", + expected={ + command_var_completion.clone(length=2), + variable_var1_completion.clone(length=2), + variable_var2_completion.clone(length=2), + }, + ) ) # If we are using the escape prefix, only commands are suggested, but no variables self.verify_completions( - self.dap_server.get_completions("`va"), - [ - command_var_completion, - ], - [ - variable_var1_completion, - variable_var2_completion, - ], + TestCase( + input="`va", + expected={ + command_var_completion.clone(length=2), + }, + not_expected={ + variable_var1_completion.clone(length=2), + variable_var2_completion.clone(length=2), + }, + ) ) # TODO: Note we are not checking the result because the `expression --` command adds an extra character diff --git a/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp b/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp index 9a3973190f7e7..671d894611db8 100644 --- a/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp @@ -72,6 +72,14 @@ static std::optional<size_t> GetCursorPos(StringRef text, uint32_t line, return cursor_pos; } +static size_t GetTokenLengthAtCursor(StringRef line, size_t cursor_pos) { + line = line.substr(0, cursor_pos); + const size_t idx = line.rfind(' '); + if (idx == StringRef::npos) + return line.size(); + return line.size() - (idx + 1); +} + /// Returns a list of possible completions for a given caret position and text. /// /// Clients should only call this request if the corresponding capability @@ -109,6 +117,7 @@ CompletionsRequestHandler::Run(const CompletionsArguments &args) const { cursor_pos -= escape_prefix.size(); } + const size_t partial_token_len = GetTokenLengthAtCursor(text, cursor_pos); // While the user is typing then we likely have an incomplete input and cannot // reliably determine the precise intent (command vs variable), try completing // the text as both a command and variable expression, if applicable. @@ -138,19 +147,13 @@ CompletionsRequestHandler::Run(const CompletionsArguments &args) const { const StringRef match = matches.GetStringAtIndex(i); const StringRef description = descriptions.GetStringAtIndex(i); - StringRef match_ref = match; - for (const StringRef commit_point : {".", "->"}) { - if (const size_t pos = match_ref.rfind(commit_point); - pos != StringRef::npos) { - match_ref = match_ref.substr(pos + commit_point.size()); - } - } - CompletionItem item; - item.text = match_ref; item.label = match; if (!description.empty()) item.detail = description; + // lldb returned completions includes the partial typed token + // overwrite it. + item.length = partial_token_len; targets.emplace_back(std::move(item)); } diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h index 3433dc74d5b31..71046d24c9787 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h @@ -168,24 +168,24 @@ struct CompletionItem { /// whether it is 0- or 1-based. If the start position is omitted the text /// is added at the location specified by the `column` attribute of the /// `completions` request. - int64_t start = 0; + uint64_t start = 0; /// Length determines how many characters are overwritten by the completion /// text and it is measured in UTF-16 code units. If missing the value 0 is /// assumed which results in the completion text being inserted. - int64_t length = 0; + uint64_t length = 0; /// Determines the start of the new selection after the text has been /// inserted (or replaced). `selectionStart` is measured in UTF-16 code /// units and must be in the range 0 and length of the completion text. If /// omitted the selection starts at the end of the completion text. - int64_t selectionStart = 0; + uint64_t selectionStart = 0; /// Determines the length of the new selection after t... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/177151 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
