[lldb/Interpreter] Fix ambiguous partial command resolution

[medismailben]

This patch is a follow-up to #97263 that fix ambigous abbreviated
command resolution.

When multiple commands are resolved, instead of failing to pick a command to
run, this patch changes to resolution logic to check if there is a single
alias match and if so, it will run the alias instead of the other matches.

This has as a side-effect that we don't need to make aliases for every
substring of aliases to support abbrivated alias resolution.

Signed-off-by: Med Ismail Bennani [email protected]

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Med Ismail Bennani (medismailben)

Changes

This patch is a follow-up to #97263 that fix ambigous abbreviated command resolution.

When multiple commands are resolved, instead of failing to pick a command to run, this patch changes to resolution logic to check if there is either a single user command match or a single alias match and if so, it will run that command instead of the others.

This has as a side-effect that we don't need to make aliases for every substring of aliases to support abbrivated alias resolution.

---

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

2 Files Affected:

• (modified) lldb/include/lldb/Interpreter/CommandInterpreter.h (+4)
• (modified) lldb/source/Interpreter/CommandInterpreter.cpp (+68-19)

diff --git a/lldb/include/lldb/Interpreter/CommandInterpreter.h b/lldb/include/lldb/Interpreter/CommandInterpreter.h
index 48f6618ab0e39..2bafc30cc8e23 100644
--- a/lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ b/lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -295,6 +295,10 @@ class CommandInterpreter : public Broadcaster,
                                       StringList *matches = nullptr,
                                       StringList *descriptions = nullptr) const;
 
+  CommandObject *
+  GetAliasCommandObject(llvm::StringRef cmd, StringList *matches = nullptr,
+                        StringList *descriptions = nullptr) const;
+
   /// Determine whether a root level, built-in command with this name exists.
   bool CommandExists(llvm::StringRef cmd) const;
 
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index fc07168b6c0ac..0fd3dfcb73ed1 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -520,10 +520,6 @@ void CommandInterpreter::Initialize() {
 
   cmd_obj_sp = GetCommandSPExact("scripting run");
   if (cmd_obj_sp) {
-    AddAlias("sc", cmd_obj_sp);
-    AddAlias("scr", cmd_obj_sp);
-    AddAlias("scri", cmd_obj_sp);
-    AddAlias("scrip", cmd_obj_sp);
     AddAlias("script", cmd_obj_sp);
   }
 
@@ -1302,6 +1298,36 @@ CommandObject *CommandInterpreter::GetUserCommandObject(
   return {};
 }
 
+CommandObject *CommandInterpreter::GetAliasCommandObject(
+    llvm::StringRef cmd, StringList *matches, StringList *descriptions) const {
+  std::string cmd_str(cmd);
+  auto find_exact = [&](const CommandObject::CommandMap &map) {
+    auto found_elem = map.find(std::string(cmd));
+    if (found_elem == map.end())
+      return (CommandObject *)nullptr;
+    CommandObject *exact_cmd = found_elem->second.get();
+    if (exact_cmd) {
+      if (matches)
+        matches->AppendString(exact_cmd->GetCommandName());
+      if (descriptions)
+        descriptions->AppendString(exact_cmd->GetHelp());
+      return exact_cmd;
+    }
+    return (CommandObject *)nullptr;
+  };
+
+  CommandObject *exact_cmd = find_exact(GetAliases());
+  if (exact_cmd)
+    return exact_cmd;
+
+  // We didn't have an exact command, so now look for partial matches.
+  StringList tmp_list;
+  StringList *matches_ptr = matches ? matches : &tmp_list;
+  AddNamesMatchingPartialString(GetAliases(), cmd_str, *matches_ptr);
+
+  return {};
+}
+
 bool CommandInterpreter::CommandExists(llvm::StringRef cmd) const {
   return m_command_dict.find(std::string(cmd)) != m_command_dict.end();
 }
@@ -3421,6 +3447,19 @@ CommandInterpreter::ResolveCommandImpl(std::string &command_line,
   std::string next_word;
   StringList matches;
   bool done = false;
+
+  auto build_alias_cmd = [&](std::string &full_name) {
+    revised_command_line.Clear();
+    matches.Clear();
+    std::string alias_result;
+    cmd_obj =
+        BuildAliasResult(full_name, scratch_command, alias_result, result);
+    revised_command_line.Printf("%s", alias_result.c_str());
+    if (cmd_obj) {
+      wants_raw_input = cmd_obj->WantsRawCommandString();
+    }
+  };
+
   while (!done) {
     char quote_char = '\0';
     std::string suffix;
@@ -3432,14 +3471,7 @@ CommandInterpreter::ResolveCommandImpl(std::string &command_line,
       bool is_real_command =
           (!is_alias) || (cmd_obj != nullptr && !cmd_obj->IsAlias());
       if (!is_real_command) {
-        matches.Clear();
-        std::string alias_result;
-        cmd_obj =
-            BuildAliasResult(full_name, scratch_command, alias_result, result);
-        revised_command_line.Printf("%s", alias_result.c_str());
-        if (cmd_obj) {
-          wants_raw_input = cmd_obj->WantsRawCommandString();
-        }
+        build_alias_cmd(full_name);
       } else {
         if (cmd_obj) {
           llvm::StringRef cmd_name = cmd_obj->GetCommandName();
@@ -3486,21 +3518,38 @@ CommandInterpreter::ResolveCommandImpl(std::string &command_line,
     if (cmd_obj == nullptr) {
       const size_t num_matches = matches.GetSize();
       if (matches.GetSize() > 1) {
-        StreamString error_msg;
-        error_msg.Printf("Ambiguous command '%s'. Possible matches:\n",
-                         next_word.c_str());
+        StringList user_cmd_matches;
+        GetUserCommandObject(next_word, &user_cmd_matches);
+
+        StringList alias_matches;
+        GetAliasCommandObject(next_word, &alias_matches);
+
+        if (user_cmd_matches.GetSize() == 1) {
+          cmd_obj = GetCommandObject(user_cmd_matches.GetStringAtIndex(0));
+          done = static_cast<bool>(cmd_obj);
+        } else if (alias_matches.GetSize() == 1) {
+          std::string full_name;
+          GetAliasFullName(alias_matches.GetStringAtIndex(0), full_name);
+          build_alias_cmd(full_name);
+          done = static_cast<bool>(cmd_obj);
+        } else {
+          StreamString error_msg;
+          error_msg.Printf("Ambiguous command '%s'. Possible matches:\n",
+                           next_word.c_str());
 
-        for (uint32_t i = 0; i < num_matches; ++i) {
-          error_msg.Printf("\t%s\n", matches.GetStringAtIndex(i));
+          for (uint32_t i = 0; i < num_matches; ++i) {
+            error_msg.Printf("\t%s\n", matches.GetStringAtIndex(i));
+          }
+          result.AppendRawError(error_msg.GetString());
         }
-        result.AppendRawError(error_msg.GetString());
       } else {
         // We didn't have only one match, otherwise we wouldn't get here.
         lldbassert(num_matches == 0);
         result.AppendErrorWithFormat("'%s' is not a valid command.\n",
                                      next_word.c_str());
       }
-      return nullptr;
+      if (!done)
+        return nullptr;
     }
 
     if (cmd_obj->IsMultiwordObject()) {

[DavidSpickett]

This is for this sort of lookup:

(lldb) reg read pc

Right? As opposed to tab completion.

This needs a test case or at least a clear example as a comment in the code if a test is not possible. It seems fine from how you've described it but these things come across better with a clear example.

[jimingham]

IIUC, you are adding a new rule that if an incoming command name has partial matches to one user command and one alias command, the user command is preferred over the alias command. Is that right?

You certainly should document that rule somewhere - maybe in the Tutorial section of the docs? Maybe in the help for command alias. And there should certainly be an explicit test to show this ordering - you can just use the script/scripting since that's what you added this for.

But I think I'm missing something, because I don't see how this solves the problem you set out to solve. You wanted to add the alias script to point to scripting run, and make sc, scr, etc also mean your alias. However, if I'm following your logic correctly, if I type scr then that matches the Alias command script -> scripting run and the user command scripting. By the logic in your path, it will choose the user command scripting not the alias, which I think is the opposite of what you wanted.

I must be missing something.

[medismailben]

IIUC, you are adding a new rule that if an incoming command name has partial matches to one user command and one alias command, the user command is preferred over the alias command. Is that right?

Yes.

You certainly should document that rule somewhere - maybe in the Tutorial section of the docs? Maybe in the help for command alias. And there should certainly be an explicit test to show this ordering - you can just use the script/scripting since that's what you added this for.

Indeed, I should document that.

But I think I'm missing something, because I don't see how this solves the problem you set out to solve. You wanted to add the alias script to point to scripting run, and make sc, scr, etc also mean your alias. However, if I'm following your logic correctly, if I type scr then that matches the Alias command script -> scripting run and the user command scripting. By the logic in your path, it will choose the user command scripting not the alias, which I think is the opposite of what you wanted.

I must be missing something.

Right, but I think commands added explicitly by the user, that overwrite other commands (or multiword commands in the scripting example), should have priority over the lldb defined commands and aliases.

[medismailben]

This is for this sort of lookup:

(lldb) reg read pc

Right? As opposed to tab completion.

Right, tab completion already can suggest multiple candidates, so that worked already.

This needs a test case or at least a clear example as a comment in the code if a test is not possible. It seems fine from how you've described it but these things come across better with a clear example.

Sorry if this wasn't clear, but I didn't write a test case because there was already a test case in #97263, but I guess I could write a test to test the ordering precisely (custom command, then alias, then native command)

[jimingham]

IIUC, you are adding a new rule that if an incoming command name has partial matches to one user command and one alias command, the user command is preferred over the alias command. Is that right?

Yes.

You certainly should document that rule somewhere - maybe in the Tutorial section of the docs? Maybe in the help for command alias. And there should certainly be an explicit test to show this ordering - you can just use the script/scripting since that's what you added this for.

Indeed, I should document that.

But I think I'm missing something, because I don't see how this solves the problem you set out to solve. You wanted to add the alias script to point to scripting run, and make sc, scr, etc also mean your alias. However, if I'm following your logic correctly, if I type scr then that matches the Alias command script -> scripting run and the user command scripting. By the logic in your path, it will choose the user command scripting not the alias, which I think is the opposite of what you wanted.
I must be missing something.

Right, but I think commands added explicitly by the user, that overwrite other commands (or multiword commands in the scripting example), should have priority over the lldb defined commands and aliases.

I'm not sure I agree with this. If I'm used to typing br for break, and then someone adds brogue (speaks the result of an expression in a Scottish accent?) I would be very surprised if br started meaning brogue. It would be much better to tell me there was now a conflict, and I can figure out what I want to do about it.

However, aliases are going to be the way I resolve this issue for myself (by adding an alias for br that goes to break, for instance.) So while it seems okay, if well explained, to have aliases silently win over built-in and user commands, I don't think it's a good idea for user commands to do so over built-in commands.

[medismailben]

However, aliases are going to be the way I resolve this issue for myself (by adding an alias for br that goes to break, for instance.) So while it seems okay, if well explained, to have aliases silently win over built-in and user commands, I don't think it's a good idea for user commands to do so over built-in commands.

Ok, I don't have a strong opinion about user commands so we can ditch them from this new logic.

[jimingham]

Im my mind, user commands and built-in commands are almost identical, the only difference being currently we don't allow you to delete built-in commands. So in cases like this it makes more sense to treat user & built-in commands the same way as far as lookup and parsing.

We should also document this lookup rule (in help command alias and in the Tutorial where it describes the basic command line parsing).

[medismailben]

@Michael137 @jimingham I've addressed your comments. The test is part of the #97263.

[jimingham]

It does seem worthwhile to have a test explicitly testing this ordering that exists among the tests for the command interpreter, rather than having it be a side-effect test in an unrelated test case.

Other than that, this is fine.

[jimingham]

LGTM