From 671640b0a9bc58b10daea47f4dc49f725c47e523 Mon Sep 17 00:00:00 2001 From: Yash Thakur <45539777+ysthakur@users.noreply.github.com> Date: Fri, 22 Nov 2024 07:29:00 -0500 Subject: [PATCH] Avoid recomputing fuzzy match scores (#13700) # Description This PR makes it so that when using fuzzy matching, the score isn't recomputed when sorting. Instead, filtering and sorting suggestions is handled by a new `NuMatcher` struct. This struct accepts suggestions and, if they match the user's typed text, stores those suggestions (along with their scores and values). At the end, it returns a sorted list of suggestions. This probably won't have a noticeable impact on performance, but it might be helpful if we start using Nucleo in the future. Minor change: Makes `find_commands_by_predicate` in `StateWorkingSet` and `EngineState` take `FnMut` rather than `Fn` for the predicate. # User-Facing Changes When using case-insensitive matching, if you have two matches `FOO` and `abc`, `abc` will be shown before `FOO` rather than the other way around. I think this way makes more sense than the current behavior. When I brought this up on Discord, WindSoilder did say it would make sense to show uppercase matches first if the user typed, say, `F`. However, that would be a lot more complicated to implement. # Tests + Formatting Added a test for the changes in https://github.com/nushell/nushell/pull/13302. # After Submitting --- .../src/completions/command_completions.rs | 176 +++++++------- .../src/completions/completion_common.rs | 217 ++++++++--------- .../src/completions/completion_options.rs | 228 ++++++++++++++---- .../src/completions/custom_completions.rs | 45 +--- .../src/completions/directory_completions.rs | 15 +- .../src/completions/dotnu_completions.rs | 83 +++---- .../src/completions/file_completions.rs | 31 +-- .../src/completions/flag_completions.rs | 58 +++-- crates/nu-cli/src/completions/mod.rs | 2 +- .../src/completions/operator_completions.rs | 24 +- .../src/completions/variable_completions.rs | 156 ++++-------- crates/nu-cli/tests/completions/mod.rs | 23 +- crates/nu-protocol/src/engine/engine_state.rs | 2 +- .../src/engine/state_working_set.rs | 2 +- 14 files changed, 551 insertions(+), 511 deletions(-) diff --git a/crates/nu-cli/src/completions/command_completions.rs b/crates/nu-cli/src/completions/command_completions.rs index 0086270ac5..b12df4735a 100644 --- a/crates/nu-cli/src/completions/command_completions.rs +++ b/crates/nu-cli/src/completions/command_completions.rs @@ -1,5 +1,7 @@ +use std::collections::HashMap; + use crate::{ - completions::{Completer, CompletionOptions, MatchAlgorithm}, + completions::{Completer, CompletionOptions}, SuggestionKind, }; use nu_parser::FlatShape; @@ -9,7 +11,7 @@ use nu_protocol::{ }; use reedline::Suggestion; -use super::{completion_common::sort_suggestions, SemanticSuggestion}; +use super::{completion_options::NuMatcher, SemanticSuggestion}; pub struct CommandCompletion { flattened: Vec<(Span, FlatShape)>, @@ -33,10 +35,11 @@ impl CommandCompletion { fn external_command_completion( &self, working_set: &StateWorkingSet, - prefix: &str, - match_algorithm: MatchAlgorithm, - ) -> Vec { - let mut executables = vec![]; + sugg_span: reedline::Span, + matched_internal: impl Fn(&str) -> bool, + matcher: &mut NuMatcher, + ) -> HashMap { + let mut suggs = HashMap::new(); // os agnostic way to get the PATH env var let paths = working_set.permanent_state.get_path_env_var(); @@ -54,24 +57,38 @@ impl CommandCompletion { .completions .external .max_results - > executables.len() as i64 - && !executables.contains( - &item - .path() - .file_name() - .map(|x| x.to_string_lossy().to_string()) - .unwrap_or_default(), - ) - && matches!( - item.path().file_name().map(|x| match_algorithm - .matches_str(&x.to_string_lossy(), prefix)), - Some(true) - ) - && is_executable::is_executable(item.path()) + <= suggs.len() as i64 { - if let Ok(name) = item.file_name().into_string() { - executables.push(name); - } + break; + } + let Ok(name) = item.file_name().into_string() else { + continue; + }; + let value = if matched_internal(&name) { + format!("^{}", name) + } else { + name.clone() + }; + if suggs.contains_key(&value) { + continue; + } + if matcher.matches(&name) && is_executable::is_executable(item.path()) { + // If there's an internal command with the same name, adds ^cmd to the + // matcher so that both the internal and external command are included + matcher.add(&name, value.clone()); + suggs.insert( + value.clone(), + SemanticSuggestion { + suggestion: Suggestion { + value, + span: sugg_span, + append_whitespace: true, + ..Default::default() + }, + // TODO: is there a way to create a test? + kind: None, + }, + ); } } } @@ -79,7 +96,7 @@ impl CommandCompletion { } } - executables + suggs } fn complete_commands( @@ -88,68 +105,59 @@ impl CommandCompletion { span: Span, offset: usize, find_externals: bool, - match_algorithm: MatchAlgorithm, + options: &CompletionOptions, ) -> Vec { let partial = working_set.get_span_contents(span); + let mut matcher = NuMatcher::new(String::from_utf8_lossy(partial), options.clone()); - let filter_predicate = |command: &[u8]| match_algorithm.matches_u8(command, partial); + let sugg_span = reedline::Span::new(span.start - offset, span.end - offset); - let mut results = working_set - .find_commands_by_predicate(filter_predicate, true) - .into_iter() - .map(move |x| SemanticSuggestion { - suggestion: Suggestion { - value: String::from_utf8_lossy(&x.0).to_string(), - description: x.1, - span: reedline::Span::new(span.start - offset, span.end - offset), - append_whitespace: true, - ..Suggestion::default() - }, - kind: Some(SuggestionKind::Command(x.2)), - }) - .collect::>(); - - let partial = working_set.get_span_contents(span); - let partial = String::from_utf8_lossy(partial).to_string(); - - if find_externals { - let results_external = self - .external_command_completion(working_set, &partial, match_algorithm) - .into_iter() - .map(move |x| SemanticSuggestion { + let mut internal_suggs = HashMap::new(); + let filtered_commands = working_set.find_commands_by_predicate( + |name| { + let name = String::from_utf8_lossy(name); + matcher.add(&name, name.to_string()) + }, + true, + ); + for (name, description, typ) in filtered_commands { + let name = String::from_utf8_lossy(&name); + internal_suggs.insert( + name.to_string(), + SemanticSuggestion { suggestion: Suggestion { - value: x, - span: reedline::Span::new(span.start - offset, span.end - offset), + value: name.to_string(), + description, + span: sugg_span, append_whitespace: true, ..Suggestion::default() }, - // TODO: is there a way to create a test? - kind: None, - }); - - let results_strings: Vec = - results.iter().map(|x| x.suggestion.value.clone()).collect(); - - for external in results_external { - if results_strings.contains(&external.suggestion.value) { - results.push(SemanticSuggestion { - suggestion: Suggestion { - value: format!("^{}", external.suggestion.value), - span: external.suggestion.span, - append_whitespace: true, - ..Suggestion::default() - }, - kind: external.kind, - }) - } else { - results.push(external) - } - } - - results - } else { - results + kind: Some(SuggestionKind::Command(typ)), + }, + ); } + + let mut external_suggs = if find_externals { + self.external_command_completion( + working_set, + sugg_span, + |name| internal_suggs.contains_key(name), + &mut matcher, + ) + } else { + HashMap::new() + }; + + let mut res = Vec::new(); + for cmd_name in matcher.results() { + if let Some(sugg) = internal_suggs + .remove(&cmd_name) + .or_else(|| external_suggs.remove(&cmd_name)) + { + res.push(sugg); + } + } + res } } @@ -158,7 +166,7 @@ impl Completer for CommandCompletion { &mut self, working_set: &StateWorkingSet, _stack: &Stack, - prefix: &[u8], + _prefix: &[u8], span: Span, offset: usize, pos: usize, @@ -188,18 +196,18 @@ impl Completer for CommandCompletion { Span::new(last.0.start, pos), offset, false, - options.match_algorithm, + options, ) } else { vec![] }; if !subcommands.is_empty() { - return sort_suggestions(&String::from_utf8_lossy(prefix), subcommands, options); + return subcommands; } let config = working_set.get_config(); - let commands = if matches!(self.flat_shape, nu_parser::FlatShape::External) + if matches!(self.flat_shape, nu_parser::FlatShape::External) || matches!(self.flat_shape, nu_parser::FlatShape::InternalCall(_)) || ((span.end - span.start) == 0) || is_passthrough_command(working_set.delta.get_file_contents()) @@ -214,13 +222,11 @@ impl Completer for CommandCompletion { span, offset, config.completions.external.enable, - options.match_algorithm, + options, ) } else { vec![] - }; - - sort_suggestions(&String::from_utf8_lossy(prefix), commands, options) + } } } diff --git a/crates/nu-cli/src/completions/completion_common.rs b/crates/nu-cli/src/completions/completion_common.rs index fc7a5fcd2c..4b5ef857e1 100644 --- a/crates/nu-cli/src/completions/completion_common.rs +++ b/crates/nu-cli/src/completions/completion_common.rs @@ -1,22 +1,20 @@ -use super::MatchAlgorithm; -use crate::{ - completions::{matches, CompletionOptions}, - SemanticSuggestion, -}; -use fuzzy_matcher::{skim::SkimMatcherV2, FuzzyMatcher}; +use super::{completion_options::NuMatcher, MatchAlgorithm}; +use crate::completions::CompletionOptions; use nu_ansi_term::Style; use nu_engine::env_to_string; use nu_path::dots::expand_ndots; use nu_path::{expand_to_real_path, home_dir}; use nu_protocol::{ engine::{EngineState, Stack, StateWorkingSet}, - CompletionSort, Span, + Span, }; use nu_utils::get_ls_colors; +use nu_utils::IgnoreCaseExt; use std::path::{is_separator, Component, Path, PathBuf, MAIN_SEPARATOR as SEP}; #[derive(Clone, Default)] pub struct PathBuiltFromString { + cwd: PathBuf, parts: Vec, isdir: bool, } @@ -30,76 +28,84 @@ pub struct PathBuiltFromString { /// want_directory: Whether we want only directories as completion matches. /// Some commands like `cd` can only be run on directories whereas others /// like `ls` can be run on regular files as well. -pub fn complete_rec( +fn complete_rec( partial: &[&str], - built: &PathBuiltFromString, - cwd: &Path, + built_paths: &[PathBuiltFromString], options: &CompletionOptions, want_directory: bool, isdir: bool, ) -> Vec { - let mut completions = vec![]; - if let Some((&base, rest)) = partial.split_first() { if base.chars().all(|c| c == '.') && (isdir || !rest.is_empty()) { - let mut built = built.clone(); - built.parts.push(base.to_string()); - built.isdir = true; - return complete_rec(rest, &built, cwd, options, want_directory, isdir); - } - } - - let mut built_path = cwd.to_path_buf(); - for part in &built.parts { - built_path.push(part); - } - - let Ok(result) = built_path.read_dir() else { - return completions; - }; - - let mut entries = Vec::new(); - for entry in result.filter_map(|e| e.ok()) { - let entry_name = entry.file_name().to_string_lossy().into_owned(); - let entry_isdir = entry.path().is_dir(); - let mut built = built.clone(); - built.parts.push(entry_name.clone()); - built.isdir = entry_isdir; - - if !want_directory || entry_isdir { - entries.push((entry_name, built)); + let built_paths: Vec<_> = built_paths + .iter() + .map(|built| { + let mut built = built.clone(); + built.parts.push(base.to_string()); + built.isdir = true; + built + }) + .collect(); + return complete_rec(rest, &built_paths, options, want_directory, isdir); } } let prefix = partial.first().unwrap_or(&""); - let sorted_entries = sort_completions(prefix, entries, options, |(entry, _)| entry); + let mut matcher = NuMatcher::new(prefix, options.clone()); - for (entry_name, built) in sorted_entries { + for built in built_paths { + let mut path = built.cwd.clone(); + for part in &built.parts { + path.push(part); + } + + let Ok(result) = path.read_dir() else { + continue; + }; + + for entry in result.filter_map(|e| e.ok()) { + let entry_name = entry.file_name().to_string_lossy().into_owned(); + let entry_isdir = entry.path().is_dir(); + let mut built = built.clone(); + built.parts.push(entry_name.clone()); + built.isdir = entry_isdir; + + if !want_directory || entry_isdir { + matcher.add(entry_name.clone(), (entry_name, built)); + } + } + } + + let mut completions = vec![]; + for (entry_name, built) in matcher.results() { match partial.split_first() { Some((base, rest)) => { - if matches(base, &entry_name, options) { - // We use `isdir` to confirm that the current component has - // at least one next component or a slash. - // Serves as confirmation to ignore longer completions for - // components in between. - if !rest.is_empty() || isdir { - completions.extend(complete_rec( - rest, - &built, - cwd, - options, - want_directory, - isdir, - )); - } else { - completions.push(built); - } + // We use `isdir` to confirm that the current component has + // at least one next component or a slash. + // Serves as confirmation to ignore longer completions for + // components in between. + if !rest.is_empty() || isdir { + completions.extend(complete_rec( + rest, + &[built], + options, + want_directory, + isdir, + )); + } else { + completions.push(built); } - if entry_name.eq(base) - && matches!(options.match_algorithm, MatchAlgorithm::Prefix) - && isdir - { - break; + + // For https://github.com/nushell/nushell/issues/13204 + if isdir && options.match_algorithm == MatchAlgorithm::Prefix { + let exact_match = if options.case_sensitive { + entry_name.eq(base) + } else { + entry_name.to_folded_case().eq(&base.to_folded_case()) + }; + if exact_match { + break; + } } } None => { @@ -147,15 +153,25 @@ fn surround_remove(partial: &str) -> String { partial.to_string() } +pub struct FileSuggestion { + pub span: nu_protocol::Span, + pub path: String, + pub style: Option