From 78f4541116e366c65343de881ed23d438b7c0fe5 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Thu, 22 Jan 2026 14:53:02 +0100 Subject: [PATCH] reader: fix try_apply_edit_to_autosuggestion false positive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Given command line ": i" and suggestion ": İnstall" whose lowercase mapping is ": i\u{307}nstall", most of our code assumes that typing "n" invalidates the autosuggestion. This doesn't happen because try_apply_edit_to_autosuggestion thinks that "i" has fully matched the suggestion's "İ". Fix this inconsistency by recording the exact number of lowercase characters already matched in the suggestion; then we only need to compare the rest. This allows us to restore an important invariant; this reverts 1d2a5997cc4 (Remove broken assert, 2026-01-21). --- crates/wcstringutil/src/lib.rs | 24 +++++-- src/history/history.rs | 4 ++ src/reader/reader.rs | 90 +++++++++++++++++++-------- tests/checks/tmux-autosuggestion.fish | 9 +++ 4 files changed, 95 insertions(+), 32 deletions(-) diff --git a/crates/wcstringutil/src/lib.rs b/crates/wcstringutil/src/lib.rs index 1ca7d8757..7f1c1d622 100644 --- a/crates/wcstringutil/src/lib.rs +++ b/crates/wcstringutil/src/lib.rs @@ -18,14 +18,24 @@ pub fn count_newlines(s: &wstr) -> usize { count } -fn is_prefix(mut lhs: impl Iterator, mut rhs: impl Iterator) -> bool { +#[derive(Eq, PartialEq)] +pub enum IsPrefix { + Prefix, + Equal, +} +pub fn is_prefix( + mut lhs: impl Iterator, + mut rhs: impl Iterator, +) -> Option { + use IsPrefix::*; loop { match (lhs.next(), rhs.next()) { - (None, _) => return true, - (Some(_), None) => return false, + (None, None) => return Some(Equal), + (None, Some(_)) => return Some(Prefix), + (Some(_), None) => return None, (Some(lhs), Some(rhs)) => { if lhs != rhs { - return false; + return None; } } } @@ -36,7 +46,7 @@ fn is_prefix(mut lhs: impl Iterator, mut rhs: impl Iterator bool { let proposed_prefix = lowercase(proposed_prefix.chars()); let value = lowercase(value.chars()); - is_prefix(proposed_prefix, value) + is_prefix(proposed_prefix, value).is_some() } pub fn string_prefixes_string_maybe_case_insensitive( @@ -63,7 +73,7 @@ pub fn strip_executable_suffix(path: &wstr) -> Option<&wstr> { pub fn string_suffixes_string_case_insensitive(proposed_suffix: &wstr, value: &wstr) -> bool { let proposed_suffix = lowercase_rev(proposed_suffix.chars()); let value = lowercase_rev(value.chars()); - is_prefix(proposed_suffix, value) + is_prefix(proposed_suffix, value).is_some() } /// Test if a string prefixes another. Returns true if a is a prefix of b. @@ -588,6 +598,8 @@ macro_rules! validate { validate!("i", "İ", true); validate!("gs", "gs_", true); validate!("gs_", "gs", false); + assert_eq!("İn".to_lowercase().as_str(), "i\u{307}n"); + validate!("echo in", "echo İnstall", false); } #[test] diff --git a/src/history/history.rs b/src/history/history.rs index b01fead9d..4ebabbf40 100644 --- a/src/history/history.rs +++ b/src/history/history.rs @@ -1640,6 +1640,10 @@ pub fn current_item(&self) -> &HistoryItem { self.current_item.as_ref().expect("No current item") } + pub fn canon_term(&self) -> &wstr { + &self.canon_term + } + /// Returns the current search result item contents. /// /// # Panics diff --git a/src/reader/reader.rs b/src/reader/reader.rs index 7a571703e..3a7e97aa2 100644 --- a/src/reader/reader.rs +++ b/src/reader/reader.rs @@ -19,6 +19,8 @@ use crate::portable_atomic::AtomicU64; use fish_common::{UTF8_BOM_WCHAR, help_section}; +use fish_fallback::lowercase; +use fish_wcstringutil::{IsPrefix, is_prefix}; use libc::{ _POSIX_VDISABLE, ECHO, EINTR, EIO, EISDIR, ENOTTY, EPERM, ESRCH, FLUSHO, ICANON, ICRNL, IEXTEN, INLCR, IXOFF, IXON, O_NONBLOCK, O_RDONLY, ONLCR, OPOST, SIGINT, SIGTTIN, STDERR_FILENO, @@ -2012,26 +2014,40 @@ fn try_apply_edit_to_autosuggestion( // This is also the main mechanism by which readline commands that don't change the command line // text avoid recomputing the autosuggestion. assert!(string_prefixes_string_maybe_case_insensitive( - autosuggestion.icase, + autosuggestion.icase_matched_codepoints.is_some(), &command_line_text[autosuggestion.search_string_range.clone()], &autosuggestion.text )); let search_string_range = autosuggestion.search_string_range.clone(); // This is a heuristic with false negatives but that seems fine. - let Some(offset) = edit.range.start.checked_sub(search_string_range.start) else { - return false; - }; - let Some(remaining) = autosuggestion.text.get(offset..) else { - return false; - }; - if edit.range.end != search_string_range.end - || !string_prefixes_string_maybe_case_insensitive( - autosuggestion.icase, - &edit.replacement, - remaining, - ) - || edit.replacement.len() == remaining.len() + if edit.range.start < search_string_range.start + || edit.range.end != search_string_range.end + || { + let suggestion = autosuggestion.text.chars(); + let replacement = edit.replacement.chars(); + let unchanged_prefix = autosuggestion.icase_matched_codepoints.map_or( + search_string_range + .len() + .checked_sub(edit.range.len()) + .unwrap(), + |matched_codepoints| { + matched_codepoints + .checked_sub( + lowercase(command_line_text[edit.range.clone()].chars()).count(), + ) + .unwrap() + }, + ); + if autosuggestion.icase_matched_codepoints.is_some() { + is_prefix( + lowercase(replacement), + lowercase(suggestion).skip(unchanged_prefix), + ) + } else { + is_prefix(replacement, suggestion.skip(unchanged_prefix)) + } + } != Some(IsPrefix::Prefix) { return false; } @@ -2050,6 +2066,7 @@ fn push_edit_internal(&mut self, elt: EditableLineTag, edit: Edit, allow_coalesc self.command_line.text(), &edit, ); + if preserves_autosuggestion { autosuggestion_update = AutosuggestionUpdate::Preserve } else if !self.autosuggestion.is_empty() @@ -5146,8 +5163,9 @@ pub(super) struct Autosuggestion { /// Always at least whole line. search_string_range: Range, - /// Whether the autosuggestion should be case insensitive. - icase: bool, + /// If the autosuggestion is a case insensitive (prefix) match, this indicates the number + /// of code points we matched in the lowercase mapping of the suggestion.. + icase_matched_codepoints: Option, /// Whether the autosuggestion is a whole match from history. is_whole_item_from_history: bool, @@ -5190,14 +5208,14 @@ fn new( command_line: WString, search_string_range: Range, text: WString, - icase: bool, + icase_matched_codepoints: Option, is_whole_item_from_history: bool, ) -> Self { Self { autosuggestion: Autosuggestion { text, search_string_range, - icase, + icase_matched_codepoints, is_whole_item_from_history, }, command_line, @@ -5333,7 +5351,7 @@ fn get_autosuggestion_performer( command_line.clone(), range.clone(), full[suggested_range].into(), - icase, + icase.then(|| searcher.canon_term().char_count()), is_whole, ); if icase { @@ -5403,11 +5421,12 @@ fn get_autosuggestion_performer( ); line_at_cursor(&full_line, would_be_cursor).to_owned() }; + let lowercase_char_count = lowercase(command_line[line_range.clone()].chars()).count(); let mut result = AutosuggestionResult::new( command_line, line_range, suggestion, - true, // normal completions are case-insensitive + Some(lowercase_char_count), // normal completions are case-insensitive /*is_whole_item_from_history=*/ false, ); result.needs_load = needs_load; @@ -5468,7 +5487,7 @@ fn autosuggest_completed(&mut self, result: AutosuggestionResult) { } else if !result.is_empty() && self.can_autosuggest() && string_prefixes_string_maybe_case_insensitive( - result.icase, + result.icase_matched_codepoints.is_some(), result.search_string(), &result.text, ) @@ -5491,6 +5510,12 @@ fn update_autosuggestion(&mut self) { let el = &self.data.command_line; if self.is_at_line_with_autosuggestion() { + let autosuggestion = &self.autosuggestion; + assert!(string_prefixes_string_maybe_case_insensitive( + autosuggestion.icase_matched_codepoints.is_some(), + &el.text()[autosuggestion.search_string_range.clone()], + &autosuggestion.text + )); return; } @@ -7369,7 +7394,7 @@ macro_rules! validate { Autosuggestion { text: L!("echo hest").to_owned(), search_string_range: 0..4, - icase: false, + icase_matched_codepoints: None, is_whole_item_from_history: true, }, "echo", @@ -7377,7 +7402,7 @@ macro_rules! validate { Some(Autosuggestion { text: L!("echo hest").to_owned(), search_string_range: 0..5, - icase: false, + icase_matched_codepoints: None, is_whole_item_from_history: true, }) ); @@ -7387,7 +7412,7 @@ macro_rules! validate { Autosuggestion { text: L!("echo hest").to_owned(), search_string_range: 0..4, - icase: false, + icase_matched_codepoints: None, is_whole_item_from_history: true, }, "echo", @@ -7400,7 +7425,7 @@ macro_rules! validate { Autosuggestion { text: L!("echo hest").to_owned(), search_string_range: 0..4, - icase: true, + icase_matched_codepoints: Some(4), is_whole_item_from_history: true, }, "echo", @@ -7408,9 +7433,22 @@ macro_rules! validate { Some(Autosuggestion { text: L!("echo hest").to_owned(), search_string_range: 0..6, - icase: true, + icase_matched_codepoints: Some(4), is_whole_item_from_history: true, }) ); + + validate!( + "Lowercase mapping is only partially matched", + Autosuggestion { + text: L!("echo İnstall").to_owned(), + search_string_range: 0..6, + icase_matched_codepoints: Some(6), + is_whole_item_from_history: true, + }, + "echo i", + Edit::new(6..6, L!("n").to_owned()), + None, + ); } } diff --git a/tests/checks/tmux-autosuggestion.fish b/tests/checks/tmux-autosuggestion.fish index e433715e5..b7a46e048 100644 --- a/tests/checks/tmux-autosuggestion.fish +++ b/tests/checks/tmux-autosuggestion.fish @@ -50,3 +50,12 @@ isolated-tmux send-keys C-u 'echo İ___' Enter C-l 'echo i' tmux-sleep isolated-tmux capture-pane -p # CHECK: prompt {{\d+}}> echo İ___ + +isolated-tmux send-keys C-u 'echo İnstall' Enter C-l 'echo i' +tmux-sleep +isolated-tmux capture-pane -p +# CHECK: prompt {{\d+}}> echo İnstall +isolated-tmux send-keys n +tmux-sleep +isolated-tmux capture-pane -p +# CHECK: prompt {{\d+}}> echo in