From adc9bbdc1840e04dea808655b69c4a43e251e176 Mon Sep 17 00:00:00 2001 From: Yash Thakur <45539777+ysthakur@users.noreply.github.com> Date: Wed, 28 May 2025 09:00:55 -0400 Subject: [PATCH] Handle multiple exact matches (#15772) # Description Fixes #15734. With case-insensitive matching, when completing a file/folder, there can be multiple exact matches. For example, if you have three folders `aa/`, `AA/`, and `aaa/`, `aa/` should match all of them. But, as reported in #15734, when using prefix matching, only `AA/` will be shown. This is because when there's an exact match in prefix match mode, we only show the first exact match. There are two options for fixing this: - Show all matched suggestions (`aa/`, `AA/`, and `aaa/`) - I chose this option - Show only the suggestions that matched exactly (`aa/` and `AA/`) but not others (`aaa/`) - This felt unintuitive # User-Facing Changes As mentioned above, when: - you have multiple folders with the same name but in different cases - and you're using prefix matching - and you're using case-insensitive matching - and you type in the name of one of these folders exactly then you'll be suggested every folder matching the typed text, rather than just exact matches # Tests + Formatting I added a test that doesn't run on Windows or MacOS (to avoid case-insensitive filesystems). While adding this test, I felt like using `Playground` rather than adding files to `tests/fixtures`. To make this easier, I refactored the `new_*_engine()` helpers in `completion_helpers.rs` a bit. There was quite a bit of code duplication there. # After Submitting N/A --- .../src/completions/completion_common.rs | 87 ++++++++------ crates/nu-cli/tests/completions/mod.rs | 26 ++++ .../support/completions_helpers.rs | 111 +++--------------- 3 files changed, 92 insertions(+), 132 deletions(-) diff --git a/crates/nu-cli/src/completions/completion_common.rs b/crates/nu-cli/src/completions/completion_common.rs index d258c84a0f..41f6c0c7db 100644 --- a/crates/nu-cli/src/completions/completion_common.rs +++ b/crates/nu-cli/src/completions/completion_common.rs @@ -39,8 +39,10 @@ fn complete_rec( isdir: bool, enable_exact_match: bool, ) -> Vec { + let has_more = !partial.is_empty() && (partial.len() > 1 || isdir); + if let Some((&base, rest)) = partial.split_first() { - if base.chars().all(|c| c == '.') && (isdir || !rest.is_empty()) { + if base.chars().all(|c| c == '.') && has_more { let built_paths: Vec<_> = built_paths .iter() .map(|built| { @@ -64,6 +66,9 @@ fn complete_rec( let prefix = partial.first().unwrap_or(&""); let mut matcher = NuMatcher::new(prefix, options); + let mut exact_match = None; + // Only relevant for case insensitive matching + let mut multiple_exact_matches = false; for built in built_paths { let mut path = built.cwd.clone(); for part in &built.parts { @@ -83,48 +88,56 @@ fn complete_rec( built.isdir = entry_isdir && !entry.path().is_symlink(); if !want_directory || entry_isdir { - matcher.add(entry_name.clone(), (entry_name, built)); + if enable_exact_match && !multiple_exact_matches && has_more { + let matches = if options.case_sensitive { + entry_name.eq(prefix) + } else { + entry_name.eq_ignore_case(prefix) + }; + if matches { + if exact_match.is_none() { + exact_match = Some(built.clone()); + } else { + multiple_exact_matches = true; + } + } + } + + matcher.add(entry_name, built); } } } - let mut completions = vec![]; - for (entry_name, built) in matcher.results() { - match partial.split_first() { - Some((base, rest)) => { - // 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 { - // Don't show longer completions if we have an exact match (#13204, #14794) - let exact_match = enable_exact_match - && (if options.case_sensitive { - entry_name.eq(base) - } else { - entry_name.eq_ignore_case(base) - }); - completions.extend(complete_rec( - rest, - &[built], - options, - want_directory, - isdir, - exact_match, - )); - if exact_match { - break; - } - } else { - completions.push(built); - } - } - None => { - completions.push(built); - } + // Don't show longer completions if we have a single exact match (#13204, #14794) + if !multiple_exact_matches { + if let Some(built) = exact_match { + return complete_rec( + &partial[1..], + &[built], + options, + want_directory, + isdir, + true, + ); } } - completions + + if has_more { + let mut completions = vec![]; + for built in matcher.results() { + completions.extend(complete_rec( + &partial[1..], + &[built], + options, + want_directory, + isdir, + false, + )); + } + completions + } else { + matcher.results() + } } #[derive(Debug)] diff --git a/crates/nu-cli/tests/completions/mod.rs b/crates/nu-cli/tests/completions/mod.rs index d1dde334a7..360bde8797 100644 --- a/crates/nu-cli/tests/completions/mod.rs +++ b/crates/nu-cli/tests/completions/mod.rs @@ -2346,6 +2346,32 @@ fn exact_match() { assert!(suggestions.len() > 1); } +#[cfg(all(not(windows), not(target_os = "macos")))] +#[test] +fn exact_match_case_insensitive() { + use nu_test_support::playground::Playground; + use support::completions_helpers::new_engine_helper; + + Playground::setup("exact_match_case_insensitive", |dirs, playground| { + playground.mkdir("AA/foo"); + playground.mkdir("aa/foo"); + playground.mkdir("aaa/foo"); + + let (dir, _, engine, stack) = new_engine_helper(dirs.test().into()); + let mut completer = NuCompleter::new(Arc::new(engine), Arc::new(stack)); + + let target = format!("open {}", folder(dir.join("aa"))); + match_suggestions( + &vec![ + folder(dir.join("AA").join("foo")).as_str(), + folder(dir.join("aa").join("foo")).as_str(), + folder(dir.join("aaa").join("foo")).as_str(), + ], + &completer.complete(&target, target.len()), + ); + }); +} + #[ignore = "was reverted, still needs fixing"] #[rstest] fn alias_offset_bug_7648() { diff --git a/crates/nu-cli/tests/completions/support/completions_helpers.rs b/crates/nu-cli/tests/completions/support/completions_helpers.rs index 38b8c00db1..2d5334a0b0 100644 --- a/crates/nu-cli/tests/completions/support/completions_helpers.rs +++ b/crates/nu-cli/tests/completions/support/completions_helpers.rs @@ -14,11 +14,8 @@ fn create_default_context() -> EngineState { nu_command::add_shell_command_context(nu_cmd_lang::create_default_context()) } -/// creates a new engine with the current path into the completions fixtures folder -pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { - // Target folder inside assets - let dir = fs::fixtures().join("completions"); - let dir_str = dir +pub fn new_engine_helper(pwd: AbsolutePathBuf) -> (AbsolutePathBuf, String, EngineState, Stack) { + let pwd_str = pwd .clone() .into_os_string() .into_string() @@ -36,13 +33,13 @@ pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { // Add pwd as env var stack.add_env_var( "PWD".to_string(), - Value::string(dir_str.clone(), nu_protocol::Span::new(0, dir_str.len())), + Value::string(pwd_str.clone(), nu_protocol::Span::new(0, pwd_str.len())), ); stack.add_env_var( "TEST".to_string(), Value::string( "NUSHELL".to_string(), - nu_protocol::Span::new(0, dir_str.len()), + nu_protocol::Span::new(0, pwd_str.len()), ), ); #[cfg(windows)] @@ -50,7 +47,7 @@ pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { "Path".to_string(), Value::string( "c:\\some\\path;c:\\some\\other\\path".to_string(), - nu_protocol::Span::new(0, dir_str.len()), + nu_protocol::Span::new(0, pwd_str.len()), ), ); #[cfg(not(windows))] @@ -58,7 +55,7 @@ pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { "PATH".to_string(), Value::string( "/some/path:/some/other/path".to_string(), - nu_protocol::Span::new(0, dir_str.len()), + nu_protocol::Span::new(0, pwd_str.len()), ), ); @@ -66,7 +63,12 @@ pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { let merge_result = engine_state.merge_env(&mut stack); assert!(merge_result.is_ok()); - (dir, dir_str, engine_state, stack) + (pwd, pwd_str, engine_state, stack) +} + +/// creates a new engine with the current path in the completions fixtures folder +pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { + new_engine_helper(fs::fixtures().join("completions")) } /// Adds pseudo PATH env for external completion tests @@ -88,23 +90,13 @@ pub fn new_external_engine() -> EngineState { engine } -/// creates a new engine with the current path into the completions fixtures folder +/// creates a new engine with the current path in the dotnu_completions fixtures folder pub fn new_dotnu_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { // Target folder inside assets let dir = fs::fixtures().join("dotnu_completions"); - let dir_str = dir - .clone() - .into_os_string() - .into_string() - .unwrap_or_default(); + let (dir, dir_str, mut engine_state, mut stack) = new_engine_helper(dir); let dir_span = nu_protocol::Span::new(0, dir_str.len()); - // Create a new engine with default context - let mut engine_state = create_default_context(); - - // Add $nu - engine_state.generate_nu_constant(); - // const $NU_LIB_DIRS let mut working_set = StateWorkingSet::new(&engine_state); let var_id = working_set.add_variable( @@ -122,15 +114,6 @@ pub fn new_dotnu_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { ); let _ = engine_state.merge_delta(working_set.render()); - // New stack - let mut stack = Stack::new(); - - // Add pwd as env var - stack.add_env_var("PWD".to_string(), Value::string(dir_str.clone(), dir_span)); - stack.add_env_var( - "TEST".to_string(), - Value::string("NUSHELL".to_string(), dir_span), - ); stack.add_env_var( "NU_LIB_DIRS".into(), Value::test_list(vec![ @@ -147,73 +130,11 @@ pub fn new_dotnu_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { } pub fn new_quote_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { - // Target folder inside assets - let dir = fs::fixtures().join("quoted_completions"); - let dir_str = dir - .clone() - .into_os_string() - .into_string() - .unwrap_or_default(); - - // Create a new engine with default context - let mut engine_state = create_default_context(); - - // New stack - let mut stack = Stack::new(); - - // Add pwd as env var - stack.add_env_var( - "PWD".to_string(), - Value::string(dir_str.clone(), nu_protocol::Span::new(0, dir_str.len())), - ); - stack.add_env_var( - "TEST".to_string(), - Value::string( - "NUSHELL".to_string(), - nu_protocol::Span::new(0, dir_str.len()), - ), - ); - - // Merge environment into the permanent state - let merge_result = engine_state.merge_env(&mut stack); - assert!(merge_result.is_ok()); - - (dir, dir_str, engine_state, stack) + new_engine_helper(fs::fixtures().join("quoted_completions")) } pub fn new_partial_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { - // Target folder inside assets - let dir = fs::fixtures().join("partial_completions"); - let dir_str = dir - .clone() - .into_os_string() - .into_string() - .unwrap_or_default(); - - // Create a new engine with default context - let mut engine_state = create_default_context(); - - // New stack - let mut stack = Stack::new(); - - // Add pwd as env var - stack.add_env_var( - "PWD".to_string(), - Value::string(dir_str.clone(), nu_protocol::Span::new(0, dir_str.len())), - ); - stack.add_env_var( - "TEST".to_string(), - Value::string( - "NUSHELL".to_string(), - nu_protocol::Span::new(0, dir_str.len()), - ), - ); - - // Merge environment into the permanent state - let merge_result = engine_state.merge_env(&mut stack); - assert!(merge_result.is_ok()); - - (dir, dir_str, engine_state, stack) + new_engine_helper(fs::fixtures().join("partial_completions")) } /// match a list of suggestions with the expected values