Skip to content

Fix panic when using fuzzy completion in a folder containing files starting with a multibyte unicode char#918

Merged
ysthakur merged 8 commits into
nushell:mainfrom
dburburan:main
Jun 14, 2025
Merged

Fix panic when using fuzzy completion in a folder containing files starting with a multibyte unicode char#918
ysthakur merged 8 commits into
nushell:mainfrom
dburburan:main

Conversation

@dburburan

Copy link
Copy Markdown
Contributor

This fixes #919 see the issue for a full description

I see a lot of discussion about newer code coming related to fuzzy matching, so this is just to temporarily fix the immediate issue of the panic. I think the most useful thing here is the addition of the test case to catch it.

I wasn't sure if I should handle the match_position = 0 case specially, or modify match_len to mean characters, not bytes. I went with the latter as I thought it looked cleaner, but there's definitely an argument for keeping the old code path for the majority of cases.

This is my first PR, very open to any feedback. Did my best but there's a lot of guidelines to absorb and I don't know most of the code base.

@ysthakur

ysthakur commented Jun 11, 2025

Copy link
Copy Markdown
Member

Hi, thanks for the PR! Haven't tested it out yet, but code looks good to me.

I wasn't sure if I should handle the match_position = 0 case specially, or modify match_len to mean characters, not bytes. I went with the latter as I thought it looked cleaner, but there's definitely an argument for keeping the old code path for the majority of cases.

I don't think only handling the match_position = 0 case would be enough, so the way you did it is fine. There's some weirdness here, like the fact that we find the shortest base string by number of bytes but then use the number of characters for match_len, but this is more of a band-aid until we get highlighting for fuzzy matching working properly, so it's fine.

The IDE menu has the same problem. Could you duplicate the logic (and test) there? In the future, we can reduce the duplication a bit, but it's tolerable for now.

@ysthakur ysthakur added the A-Completions Area: Tab completion and inline hint completions label Jun 11, 2025
@dburburan

Copy link
Copy Markdown
Contributor Author

I fixed the panic and added the test case to IDE menu.

Also, I found an additional panic that only happens in the IDE menu case because that one truncates the displayed string, which can then be shorter than match_position. I've fixed it by putting in a check for that case which just goes back to the 0 default.

Added test cases for this new panic in both IDE and columnar menu, even though columnar doesn't have this issue right now.

Handling unicode lengths overall isn't quite right e.g. the borders -
20250611 201800-testdir

But I want to keep this PR focused on just fixing panics, and at least these ones shouldn't happen now.

Comment thread src/menu/columnar_menu.rs
Comment thread src/menu/ide_menu.rs Outdated
Comment thread src/menu/ide_menu.rs Outdated
Comment thread src/menu/ide_menu.rs Outdated
Comment thread src/menu/columnar_menu.rs
Comment thread src/menu/ide_menu.rs Outdated
@ysthakur

Copy link
Copy Markdown
Member

LGTM! Thanks for working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Completions Area: Tab completion and inline hint completions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic when using fuzzy completion in a folder containing files starting with a multibyte unicode char

2 participants