Skip to content
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.

Sign up for GitHub

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

Fix panic in menus #782

Closed
wants to merge 5 commits into from
Closed

Fix panic in menus #782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
883c48e
fix ide menu not reporting correct required_lines
maxomatic458 Apr 14, 2024
d0bd552
fix possible panic
maxomatic458 Apr 14, 2024
8fc2999
fix panic for columnar menu
maxomatic458 Apr 14, 2024
b143ae4
remove unrelated commit from this branch
maxomatic458 Apr 14, 2024
b2c43d3
fix panic with unicode chars
maxomatic458 Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/menu/columnar_menu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,22 @@ impl ColumnarMenu {
use_ansi_coloring: bool,
) -> String {
if use_ansi_coloring {
let match_len = self.working_details.shortest_base_string.len();
let match_len = self
.working_details
.shortest_base_string
.trim_end()
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to trim the string and thus change the length here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shortest_base_string can include trailing whitespace from the buffer, because the completer will ignore whitespace, when making a completion. "test " -> will complete the same as "test" but will have a different span. so we need to trim it, so 4 chars match and not 5.

.chars()
.count();

// Split string so the match text can be styled
let (match_str, remaining_str) = suggestion.value.split_at(match_len);
let (match_str, remaining_str) = if match_len <= suggestion.value.chars().count() {
Copy link
Member

Choose a reason for hiding this comment

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

As the self.working_details.shortest_base_string succeeded to be generated in self.update_values() (the range coming from Completer::complete_with_base_ranges did not violate char boundaries which would panic the SliceIndex)

reedline/src/menu/columnar_menu.rs

Lines 497 to 504 in 46f410b

let (values, base_ranges) = completer.complete_with_base_ranges(&input, pos);
self.values = values;
self.working_details.shortest_base_string = base_ranges
.iter()
.map(|range| editor.get_buffer()[range.clone()].to_string())
.min_by_key(|s| s.len())
.unwrap_or_default();

having to do char based indexing, indicates to me that shortest_base_string seems to be not a strict prefix of suggestion.value.
Don't know if that is really the intended semantics to then split here based on length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ranges from Completer::complete_with_base_ranges (and pretty much all buffer related stuff) are byte based not character based, to support unicode we need to do char indexing here.

shortest_base_string seems to be not a strict prefix of suggestion.value

thats actually somewhat true if you use other completion methods (instead of the default starts_with)
if thats the case if match_len <= suggestion.value.chars().count() will most likely fail and not style the matching string at all.

Copy link
Member

Choose a reason for hiding this comment

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

If the byte indxes are wrong it should already fail on the index in L502, if I remember the slice index semantics of str correctly.

So this whole section doing the match highlighting either needs to be aware of which matching mode was used, get the correct coordinates to highlight or perform some alignment to check which characters in the query match to the picked Suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im pretty sure the indexing in L502 wont fail because the range is directly coming from the completer (doesnt matter which completion mode was used)

if we wanted to add support for other completion methods then we would need to be aware of that, but currently those cases are just ignored (so if the check failes, the text remains unstyled)

Im not planning to add support for other completion methods in this, i just want to make them not crash the styling system. (and support unicode)

full support for other completion methods would be something for another PR

Copy link
Member

Choose a reason for hiding this comment

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

Im pretty sure the indexing in L502 wont fail because the range is directly coming from the completer (doesnt matter which completion mode was used)

The Completer implementer can deliver invalid (i.e. not byte-encoding aware or out-of bounds) ranges in its span (or completely override complete_with_base_ranges).
But this would panic L502 as it tries to index the buffer &str. Indexing outside the encoded chars/bounds of a str is a panic
So the culprit for what you a trying to fix is not a malformed span from the Completer (which should be fixed at that source first and then hardened here in a way that still informs implementers).

if you use other completion methods (instead of the default starts_with)
if thats the case if match_len <= suggestion.value.chars().count() will most likely fail and not style the matching string at all.

We shouldn't accept a highlighting system that only stops to highlight mismatching prompted text and suggestion under the circumstance that something else is fishy.

The highlighting should mark the text that is equal/equivalent between the user query and the suggestions.
And the assumption that the user query is a prefix of all suggestions is not true.

(I don't have the time to do a full archaeology on #730 or before if the shortest_base_string is the right proxy to reflect the matching text itself)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think ill close this PR for now and make another one where i will implement your suggestions. (so add proper support for different completion methods and figure out a safer way to get the base ranges)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on your input @maxomatic458, I'm going to close this. I look forward to seeing your next one. Thanks!

(
suggestion.value.chars().take(match_len).collect::<String>(),
suggestion.value.chars().skip(match_len).collect::<String>(),
)
} else {
("".to_string(), suggestion.value.to_string())
};

let suggestion_style_prefix = suggestion
.style
Expand Down
16 changes: 14 additions & 2 deletions src/menu/ide_menu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,10 +512,22 @@ impl IdeMenu {
};

if use_ansi_coloring {
let match_len = self.working_details.shortest_base_string.len();
let match_len = self
.working_details
.shortest_base_string
.trim_end()
.chars()
.count();

// Split string so the match text can be styled
let (match_str, remaining_str) = string.split_at(match_len);
let (match_str, remaining_str) = if match_len <= string.chars().count() {
(
string.chars().take(match_len).collect::<String>(),
string.chars().skip(match_len).collect::<String>(),
)
} else {
("".to_string(), string.to_string())
};

let suggestion_style_prefix = suggestion
.style
Expand Down
Loading