Skip to content

[lldb] Add test to document alias tab completion behaviour #65760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

DavidSpickett
Copy link
Collaborator

While looking at #49528 I found that, happily, aliases can now be tab completed.

However, if there is a built-in match that will always be taken. Which is a bit surprising, though logical if we don't want people really messing up their commands I guess.

Meaning "b" tab completes to our built-in breakpoint alias, before it looks at any of the aliases. "bf" doesn't match "b", so it looks through the aliases.

I didn't find any tests for this in the obvious places, so this adds some.

While looking at llvm#49528
I found that, happily, aliases can now be tab completed.

However, if there is a built-in match that will always be taken.
Which is a bit surprising, though logical if we don't want people
really messing up their commands I guess.

Meaning "b" tab completes to our built-in breakpoint alias,
before it looks at any of the aliases. "bf" doesn't match "b",
so it looks through the aliases.

I didn't find any tests for this in the obvious places,
so this adds some.
@DavidSpickett DavidSpickett requested a review from a team as a code owner September 8, 2023 13:59
@github-actions github-actions bot added the lldb label Sep 8, 2023
@DavidSpickett
Copy link
Collaborator Author

This just documents the status quo - but, I'd like to confirm this is expected and desirable behaviour, hence the review.

Seems fine to me as it does prevent the user causing some kinds of chaos. breakpoint is perhaps just a bad example because it has the single letter alias b that gets in the way of almost any alias name you'd choose.

@jimingham
Copy link
Collaborator

The test is okay, but this seems like confusing behavior to me. Most people who use lldb don't set up command aliases and aren't really aware of which commands are aliases and which are built-in commands. So treating them differently seems wrong to me. If you have a br alias then br should win over breakpoint because it is an exact match. If you have a brk alias, then I would expect br to be ambiguous.

@DavidSpickett
Copy link
Collaborator Author

That was my first assumption, that they were all in one pool of commands for completion. Then I thought maybe it had been setup this way to protect the built-ins somehow. Given it wasn't tested it may not have been intentionally implemented that way.

If people agree that tab completion should look at commands and aliases at once I can fix that in a follow up. I agree it's what a user would assume.

@JDevlieghere
Copy link
Member

+1. As Jim pointed out, we use them ourselves internally, so to me the distinction sounds like an implementation detail and should be transparent for completion.

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We can update the test when we fix the behavior.

@DavidSpickett DavidSpickett merged commit 0f1a018 into llvm:main Sep 8, 2023
@jimingham
Copy link
Collaborator

Just to make sure there's no confusion... The reason that b completes to __regex_break is that we always prefer exact matches in completion. Since we did:

command alias b __regex_break

then b<TAB> should only complete to the b alias. That makes sense, since executing b will run the __regex_break alias.

Maybe this was already clear, but it sounded from David's original comment that b selecting the __regex_break alias was because aliases & built-in commands are being treated differently, but in fact that one should still be unambiguous because exact matches always win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants