[lldb] Treat user aliases the same as built-ins when tab completing

[DavidSpickett]

Previously we would check all built-ins first for suggestions,
then check built-ins and aliases. This meant that if you had
an alias brkpt -> breakpoint, "br" would complete to "breakpoint".

Instead of giving you the choice of "brkpt" or "breakpoint".

[DavidSpickett]

This is a PR "stacked" on #65973, so the first 2 commits are from that and the original one. Review with the commits tab instead of "files changed".

[llvmbot]

@llvm/pr-subscribers-lldb

Changes

Previously we would check all built-ins first for suggestions,
then check built-ins and aliases. This meant that if you had
an alias brkpt -> breakpoint, "br" would complete to "breakpoint".

Instead of giving you the choice of "brkpt" or "breakpoint".

Full diff: https://github.com/llvm/llvm-project/pull/65974.diff

9 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/lldbtest.py (+12-18)
• (modified) lldb/source/Interpreter/CommandInterpreter.cpp (+5-30)
• (modified) lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp (+1-1)
• (modified) lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp (+1-1)
• (modified) lldb/source/Target/Process.cpp (+2-2)
• (modified) lldb/test/API/commands/expression/completion/TestExprCompletion.py (+46-46)
• (modified) lldb/test/API/functionalities/abbreviation/TestAbbreviations.py (+1-1)
• (modified) lldb/test/API/functionalities/completion/TestCompletion.py (+11-21)
• (modified) lldb/test/API/functionalities/load_unload/TestLoadUnload.py (+9)

diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 49355d61911837f..4f8bdcd7263dc35 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2223,10 +2223,7 @@ def check_completion_with_desc(
                 )
             self.assertFalse(got_failure, error_msg)
 
-    def complete_exactly(self, str_input, patterns):
-        self.complete_from_to(str_input, patterns, True)
-
-    def complete_from_to(self, str_input, patterns, turn_off_re_match=False):
+    def complete_from_to(self, str_input, patterns):
         """Test that the completion mechanism completes str_input to patterns,
         where patterns could be a pattern-string or a list of pattern-strings"""
         # Patterns should not be None in order to proceed.
@@ -2254,21 +2251,18 @@ def complete_from_to(self, str_input, patterns, turn_off_re_match=False):
                 for idx in range(1, num_matches + 1):
                     compare_string += match_strings.GetStringAtIndex(idx) + "\n"
 
+        # If the singular pattern is the same as the input, assume that we
+        # shouldn't have any completions.
+        if len(patterns) == 1 and str_input == patterns[0] and num_matches:
+            self.fail("Expected no completions but got:\n" + compare_string)
+
         for p in patterns:
-            if turn_off_re_match:
-                self.expect(
-                    compare_string,
-                    msg=COMPLETION_MSG(str_input, p, match_strings),
-                    exe=False,
-                    substrs=[p],
-                )
-            else:
-                self.expect(
-                    compare_string,
-                    msg=COMPLETION_MSG(str_input, p, match_strings),
-                    exe=False,
-                    patterns=[p],
-                )
+            self.expect(
+                compare_string,
+                msg=COMPLETION_MSG(str_input, p, match_strings),
+                exe=False,
+                substrs=[p],
+            )
 
     def completions_match(self, command, completions):
         """Checks that the completions for the given command are equal to the
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index 6d1ad799f2d10fb..fc94bf6fbfe117c 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1227,36 +1227,11 @@ CommandObject *
 CommandInterpreter::GetCommandObject(llvm::StringRef cmd_str,
                                      StringList *matches,
                                      StringList *descriptions) const {
-  CommandObject *command_obj =
-      GetCommandSP(cmd_str, false, true, matches, descriptions).get();
-
-  // If we didn't find an exact match to the command string in the commands,
-  // look in the aliases.
-
-  if (command_obj)
-    return command_obj;
-
-  command_obj = GetCommandSP(cmd_str, true, true, matches, descriptions).get();
-
-  if (command_obj)
-    return command_obj;
-
-  // If there wasn't an exact match then look for an inexact one in just the
-  // commands
-  command_obj = GetCommandSP(cmd_str, false, false, nullptr).get();
-
-  // Finally, if there wasn't an inexact match among the commands, look for an
-  // inexact match in both the commands and aliases.
-
-  if (command_obj) {
-    if (matches)
-      matches->AppendString(command_obj->GetCommandName());
-    if (descriptions)
-      descriptions->AppendString(command_obj->GetHelp());
-    return command_obj;
-  }
-
-  return GetCommandSP(cmd_str, true, false, matches, descriptions).get();
+  // Try to find a match among commands and aliases. Allowing inexact matches,
+  // but perferring exact matches.
+  return GetCommandSP(cmd_str, /*include_aliases=*/true, /*exact=*/false,
+                             matches, descriptions)
+                    .get();
 }
 
 CommandObject *CommandInterpreter::GetUserCommandObject(
diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
index 1e91f2ccd198259..b4f1b76c39dbebf 100644
--- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -949,7 +949,7 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process,
 Status PlatformPOSIX::UnloadImage(lldb_private::Process *process,
                                   uint32_t image_token) {
   const addr_t image_addr = process->GetImagePtrFromToken(image_token);
-  if (image_addr == LLDB_INVALID_ADDRESS)
+  if (image_addr == LLDB_INVALID_IMAGE_TOKEN)
     return Status("Invalid image token");
 
   StreamString expr;
diff --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
index cbac14e2ccf7a92..88d543289a8470e 100644
--- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -415,7 +415,7 @@ uint32_t PlatformWindows::DoLoadImage(Process *process,
 
 Status PlatformWindows::UnloadImage(Process *process, uint32_t image_token) {
   const addr_t address = process->GetImagePtrFromToken(image_token);
-  if (address == LLDB_INVALID_ADDRESS)
+  if (address == LLDB_INVALID_IMAGE_TOKEN)
     return Status("invalid image token");
 
   StreamString expression;
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 2b0774588138881..f82ab05362fbee9 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -5911,12 +5911,12 @@ size_t Process::AddImageToken(lldb::addr_t image_ptr) {
 lldb::addr_t Process::GetImagePtrFromToken(size_t token) const {
   if (token < m_image_tokens.size())
     return m_image_tokens[token];
-  return LLDB_INVALID_ADDRESS;
+  return LLDB_INVALID_IMAGE_TOKEN;
 }
 
 void Process::ResetImageToken(size_t token) {
   if (token < m_image_tokens.size())
-    m_image_tokens[token] = LLDB_INVALID_ADDRESS;
+    m_image_tokens[token] = LLDB_INVALID_IMAGE_TOKEN;
 }
 
 Address
diff --git a/lldb/test/API/commands/expression/completion/TestExprCompletion.py b/lldb/test/API/commands/expression/completion/TestExprCompletion.py
index 3c354a3bce1a9b5..ada818989c789a1 100644
--- a/lldb/test/API/commands/expression/completion/TestExprCompletion.py
+++ b/lldb/test/API/commands/expression/completion/TestExprCompletion.py
@@ -29,34 +29,34 @@ def test_expr_completion(self):
         )
 
         # Completing member functions
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooNoArgs", "expr some_expr.FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooWithArgs", "expr some_expr.FooWithArgsBar("
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooWithMultipleArgs",
             "expr some_expr.FooWithMultipleArgsBar(",
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooUnderscore", "expr some_expr.FooUnderscoreBar_()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooNumbers", "expr some_expr.FooNumbersBar1()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.StaticMemberMethod",
             "expr some_expr.StaticMemberMethodBar()",
         )
 
         # Completing static functions
-        self.complete_exactly(
+        self.complete_from_to(
             "expr Expr::StaticMemberMethod", "expr Expr::StaticMemberMethodBar()"
         )
 
         # Completing member variables
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.MemberVariab", "expr some_expr.MemberVariableBar"
         )
 
@@ -94,43 +94,43 @@ def test_expr_completion(self):
         self.completions_contain("expr 1+", ["1+some_expr", "1+static_cast"])
 
         # Test with spaces
-        self.complete_exactly(
+        self.complete_from_to(
             "expr   some_expr .FooNoArgs", "expr   some_expr .FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr  some_expr .FooNoArgs", "expr  some_expr .FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr .FooNoArgs", "expr some_expr .FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr. FooNoArgs", "expr some_expr. FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr . FooNoArgs", "expr some_expr . FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr Expr :: StaticMemberMethod", "expr Expr :: StaticMemberMethodBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr Expr ::StaticMemberMethod", "expr Expr ::StaticMemberMethodBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr Expr:: StaticMemberMethod", "expr Expr:: StaticMemberMethodBar()"
         )
 
         # Test that string literals don't break our parsing logic.
-        self.complete_exactly(
+        self.complete_from_to(
             'expr const char *cstr = "some_e"; char c = *cst',
             'expr const char *cstr = "some_e"; char c = *cstr',
         )
-        self.complete_exactly(
+        self.complete_from_to(
             'expr const char *cstr = "some_e" ; char c = *cst',
             'expr const char *cstr = "some_e" ; char c = *cstr',
         )
         # Requesting completions inside an incomplete string doesn't provide any
         # completions.
-        self.complete_exactly(
+        self.complete_from_to(
             'expr const char *cstr = "some_e', 'expr const char *cstr = "some_e'
         )
 
@@ -139,110 +139,110 @@ def test_expr_completion(self):
         self.assume_no_completions("expr -i0 -- some_expr.", 11)
 
         # Test with expr arguments
-        self.complete_exactly(
+        self.complete_from_to(
             "expr -i0 -- some_expr .FooNoArgs", "expr -i0 -- some_expr .FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr  -i0 -- some_expr .FooNoArgs",
             "expr  -i0 -- some_expr .FooNoArgsBar()",
         )
 
         # Addrof and deref
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (*(&some_expr)).FooNoArgs", "expr (*(&some_expr)).FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (*(&some_expr)) .FooNoArgs", "expr (*(&some_expr)) .FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (* (&some_expr)) .FooNoArgs", "expr (* (&some_expr)) .FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (* (& some_expr)) .FooNoArgs",
             "expr (* (& some_expr)) .FooNoArgsBar()",
         )
 
         # Addrof and deref (part 2)
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (&some_expr)->FooNoArgs", "expr (&some_expr)->FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (&some_expr) ->FooNoArgs", "expr (&some_expr) ->FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (&some_expr) -> FooNoArgs", "expr (&some_expr) -> FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr (&some_expr)-> FooNoArgs", "expr (&some_expr)-> FooNoArgsBar()"
         )
 
         # Builtin arg
-        self.complete_exactly("expr static_ca", "expr static_cast")
+        self.complete_from_to("expr static_ca", "expr static_cast")
 
         # From other files
-        self.complete_exactly(
+        self.complete_from_to(
             "expr fwd_decl_ptr->Hidden", "expr fwd_decl_ptr->HiddenMember"
         )
 
         # Types
-        self.complete_exactly("expr LongClassNa", "expr LongClassName")
-        self.complete_exactly(
+        self.complete_from_to("expr LongClassNa", "expr LongClassName")
+        self.complete_from_to(
             "expr LongNamespaceName::NestedCla", "expr LongNamespaceName::NestedClass"
         )
 
         # Namespaces
-        self.complete_exactly("expr LongNamespaceNa", "expr LongNamespaceName::")
+        self.complete_from_to("expr LongNamespaceNa", "expr LongNamespaceName::")
 
         # Multiple arguments
-        self.complete_exactly(
+        self.complete_from_to(
             "expr &some_expr + &some_e", "expr &some_expr + &some_expr"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr SomeLongVarNameWithCapitals + SomeLongVarName",
             "expr SomeLongVarNameWithCapitals + SomeLongVarNameWithCapitals",
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr SomeIntVar + SomeIntV", "expr SomeIntVar + SomeIntVar"
         )
 
         # Multiple statements
-        self.complete_exactly(
+        self.complete_from_to(
             "expr long LocalVariable = 0; LocalVaria",
             "expr long LocalVariable = 0; LocalVariable",
         )
 
         # Custom Decls
-        self.complete_exactly(
+        self.complete_from_to(
             "expr auto l = [](int LeftHandSide, int bx){ return LeftHandS",
             "expr auto l = [](int LeftHandSide, int bx){ return LeftHandSide",
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr struct LocalStruct { long MemberName; } ; LocalStruct S; S.Mem",
             "expr struct LocalStruct { long MemberName; } ; LocalStruct S; S.MemberName",
         )
 
         # Completing function call arguments
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooWithArgsBar(some_exp",
             "expr some_expr.FooWithArgsBar(some_expr",
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooWithArgsBar(SomeIntV",
             "expr some_expr.FooWithArgsBar(SomeIntVar",
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.FooWithMultipleArgsBar(SomeIntVar, SomeIntVa",
             "expr some_expr.FooWithMultipleArgsBar(SomeIntVar, SomeIntVar",
         )
 
         # Function return values
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.Self().FooNoArgs", "expr some_expr.Self().FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.Self() .FooNoArgs", "expr some_expr.Self() .FooNoArgsBar()"
         )
-        self.complete_exactly(
+        self.complete_from_to(
             "expr some_expr.Self(). FooNoArgs", "expr some_expr.Self(). FooNoArgsBar()"
         )
 
diff --git a/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py b/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py
index 10431e41dc81a2e..cade8d87e7f76f5 100644
--- a/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py
+++ b/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py
@@ -20,7 +20,7 @@ def test_command_abbreviations_and_aliases(self):
         self.assertTrue(result.Succeeded())
         self.assertEqual("apropos script", result.GetOutput())
 
-        command_interpreter.ResolveCommand("h", result)
+        command_interpreter.ResolveCommand("he", result)
         self.assertTrue(result.Succeeded())
         self.assertEqual("help", result.GetOutput())
 
diff --git a/lldb/test/API/functionalities/completion/TestCompletion.py b/lldb/test/API/functionalities/completion/TestCompletion.py
index cc2a20dcd0dca76..fd1db08b9995c88 100644
--- a/lldb/test/API/functionalities/completion/TestCompletion.py
+++ b/lldb/test/API/functionalities/completion/TestCompletion.py
@@ -237,16 +237,13 @@ def test_log_dir(self):
         src_dir = os.path.dirname(os.path.realpath(__file__))
         self.complete_from_to(
             "log enable lldb expr -f " + src_dir,
-            [src_dir + os.sep],
-            turn_off_re_match=True,
-        )
+            [src_dir + os.sep])
 
     # 
     def test_infinite_loop_while_completing(self):
         """Test that 'process print hello\' completes to itself and does not infinite loop."""
         self.complete_from_to(
-            "process print hello\\", "process print hello\\", turn_off_re_match=True
-        )
+            "process print hello\\", "process print hello\\")
 
     def test_watchpoint_co(self):
         """Test that 'watchpoint co' completes to 'watchpoint command '."""
@@ -624,19 +621,15 @@ def test_command_unalias(self):
 
     def test_command_aliases(self):
         self.runCmd("command alias brkpt breakpoint")
-        # If there is an unambiguous completion from the built-in commands,
-        # we choose that.
-        self.complete_from_to("br", "breakpoint")
-        # Only if there is not, do we then look for an unambiguous completion
-        # from the user defined aliases.
+        # Exact matches are chosen if possible, even if there are longer
+        # completions we could use.
+        self.complete_from_to("b", "b ")
+        # Aliases are included in possible completions.
+        self.complete_from_to("br", ["breakpoint", "brkpt"])
+        # An alias can be the chosen completion.
         self.complete_from_to("brk", "brkpt")
 
-        # Aliases are included when there's no exact match.
-        self.runCmd("command alias play breakpoint")
-        self.complete_from_to("pl", ["plugin", "platform", "play"])
-
-        # That list can also contain only aliases if there's no built-ins to
-        # match.
+        # The list can contain only aliases if there's no built-ins to match.
         self.runCmd("command alias test_1 breakpoint")
         self.runCmd("command alias test_2 breakpoint")
         self.complete_from_to("test_", ["test_1", "test_2"])
@@ -727,9 +720,7 @@ def test_symbol_name(self):
         self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
         self.complete_from_to(
             "breakpoint set -n Fo",
-            "breakpoint set -n Foo::Bar(int,\\ int)",
-            turn_off_re_match=True,
-        )
+            "breakpoint set -n Foo::Bar(int,\\ int)")
         # No completion for Qu because the candidate is
         # (anonymous namespace)::Quux().
         self.complete_from_to("breakpoint set -n Qu", "")
@@ -822,8 +813,7 @@ def test_common_completion_target_stophook_ids(self):
         for subcommand in subcommands:
             self.complete_from_to(
                 "target stop-hook " + subcommand + " 1 ",
-                "target stop-hook " + subcommand + " 1 ",
-            )
+                "target stop-hook " + subcommand + " 1 ")
 
     def test_common_completion_type_language(self):
         self.complete_from_to("type category -l ", ["c"])
diff --git a/lldb/test/API/functionalities/load_unload/TestLoadUnload.py b/lldb/test/API/functionalities/load_unload/TestLoadUnload.py
index 7e8acfa3021acfc..2208e520f1d5612 100644
--- a/lldb/test/API/functionalities/load_unload/TestLoadUnload.py
+++ b/lldb/test/API/functionalities/load_unload/TestLoadUnload.py
@@ -307,6 +307,15 @@ def run_lldb_process_load_and_unload_commands(self):
             patterns=["Unloading .* with index %s.*ok" % index],
         )
 
+        # Confirm that we unloaded properly.
+        self.expect(
+            "image lookup -n a_function",
+            "a_function should not exist after unload",
+            error=True,
+            matching=False,
+            patterns=["1 match found"],
+        )
+
         self.runCmd("process continue")
 
     @expectedFailureAll(oslist=["windows"])  # breakpoint not hit

[jimingham]

Why did you need to change "h" to "he" here? "help" is the only command that begins with "h" so "h" should complete to "help".

[DavidSpickett]

$ ./bin/lldb
(lldb) h
Ambiguous command 'h'. Possible matches:
        help
        history
<...>
'history' is an abbreviation for 'session history'

I figured if we're treating aliases and abreviations and the like all the same, then changing this test was the logical thing to do. Unless abbreviations should never resolve to aliases?

[DavidSpickett]

Though even if I was right, I should explain that in the test case.

[jimingham]

But there's an explicit alias from "h" to "help". So "h" shouldn't be ambiguous, and it isn't on current TOT lldb. If this change is making h ambiguous, then that's not right.

[jimingham]

Huh, I don't see where we explicitly alias "help" to "h", but in the current TOT lldb (well, as of last Friday) and in the latest Apple release, "h" is not ambiguous:

(lldb) version
lldb version 18.0.0git ([email protected]:llvm/llvm-project.git revision 04a6dc24db333ee8bfdf50791a18024f30086ff6)
  clang revision 04a6dc24db333ee8bfdf50791a18024f30086ff6
  llvm revision 04a6dc24db333ee8bfdf50791a18024f30086ff6
(lldb) h
Debugger commands:
  apropos           -- List debugger commands related to a word or subject.
  breakpoint        -- Commands for operating on breakpoints (see 'help b' for

I wonder what's different. Whatever we do, h really should mean "help" that's a far more common usage.

[DavidSpickett]

I don't see it in CommandInterpreter::Initialize with the rest but you're right there's definitely something in there, h -> help on Linux too.

[DavidSpickett]

I figured it out. Previously, when you start with "h":

• exact match commands - fails
• exact match aliases - fails
• inexact match commands - succeeds

And we would skip looking in the aliases for an inexact match. That's why "history" never shows up.

Now I'm treating aliases and commands on the same level, when the inexact match is looked for both show up.

I've added an explicit alias h -> help to fix this, and restored the test to what it was.

[DavidSpickett]

So it never was an intentional alias because it never needed aliasing, now it does.

[jimingham]

This looks right to me, but I'm worried about why you had to change the "h" completion test to "he"?

[jimingham]

Excellent, thanks for chasing that logic down. This is the correct solution, since we really do want h to mean help not just accidentally.

[jimingham]

I don't see where you approve a PR in this UI but this is approved.