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/<TAB>` 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
This commit is contained in:
Yash Thakur 2025-05-28 09:00:55 -04:00 committed by GitHub
parent 37bc922a67
commit adc9bbdc18
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 92 additions and 132 deletions

View File

@ -39,8 +39,10 @@ fn complete_rec(
isdir: bool, isdir: bool,
enable_exact_match: bool, enable_exact_match: bool,
) -> Vec<PathBuiltFromString> { ) -> Vec<PathBuiltFromString> {
let has_more = !partial.is_empty() && (partial.len() > 1 || isdir);
if let Some((&base, rest)) = partial.split_first() { 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 let built_paths: Vec<_> = built_paths
.iter() .iter()
.map(|built| { .map(|built| {
@ -64,6 +66,9 @@ fn complete_rec(
let prefix = partial.first().unwrap_or(&""); let prefix = partial.first().unwrap_or(&"");
let mut matcher = NuMatcher::new(prefix, options); 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 { for built in built_paths {
let mut path = built.cwd.clone(); let mut path = built.cwd.clone();
for part in &built.parts { for part in &built.parts {
@ -83,48 +88,56 @@ fn complete_rec(
built.isdir = entry_isdir && !entry.path().is_symlink(); built.isdir = entry_isdir && !entry.path().is_symlink();
if !want_directory || entry_isdir { 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![]; // Don't show longer completions if we have a single exact match (#13204, #14794)
for (entry_name, built) in matcher.results() { if !multiple_exact_matches {
match partial.split_first() { if let Some(built) = exact_match {
Some((base, rest)) => { return complete_rec(
// We use `isdir` to confirm that the current component has &partial[1..],
// at least one next component or a slash. &[built],
// Serves as confirmation to ignore longer completions for options,
// components in between. want_directory,
if !rest.is_empty() || isdir { isdir,
// Don't show longer completions if we have an exact match (#13204, #14794) true,
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);
}
} }
} }
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)] #[derive(Debug)]

View File

@ -2346,6 +2346,32 @@ fn exact_match() {
assert!(suggestions.len() > 1); 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"] #[ignore = "was reverted, still needs fixing"]
#[rstest] #[rstest]
fn alias_offset_bug_7648() { fn alias_offset_bug_7648() {

View File

@ -14,11 +14,8 @@ fn create_default_context() -> EngineState {
nu_command::add_shell_command_context(nu_cmd_lang::create_default_context()) 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_helper(pwd: AbsolutePathBuf) -> (AbsolutePathBuf, String, EngineState, Stack) {
pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { let pwd_str = pwd
// Target folder inside assets
let dir = fs::fixtures().join("completions");
let dir_str = dir
.clone() .clone()
.into_os_string() .into_os_string()
.into_string() .into_string()
@ -36,13 +33,13 @@ pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) {
// Add pwd as env var // Add pwd as env var
stack.add_env_var( stack.add_env_var(
"PWD".to_string(), "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( stack.add_env_var(
"TEST".to_string(), "TEST".to_string(),
Value::string( Value::string(
"NUSHELL".to_string(), "NUSHELL".to_string(),
nu_protocol::Span::new(0, dir_str.len()), nu_protocol::Span::new(0, pwd_str.len()),
), ),
); );
#[cfg(windows)] #[cfg(windows)]
@ -50,7 +47,7 @@ pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) {
"Path".to_string(), "Path".to_string(),
Value::string( Value::string(
"c:\\some\\path;c:\\some\\other\\path".to_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))] #[cfg(not(windows))]
@ -58,7 +55,7 @@ pub fn new_engine() -> (AbsolutePathBuf, String, EngineState, Stack) {
"PATH".to_string(), "PATH".to_string(),
Value::string( Value::string(
"/some/path:/some/other/path".to_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); let merge_result = engine_state.merge_env(&mut stack);
assert!(merge_result.is_ok()); 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 /// Adds pseudo PATH env for external completion tests
@ -88,23 +90,13 @@ pub fn new_external_engine() -> EngineState {
engine 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) { pub fn new_dotnu_engine() -> (AbsolutePathBuf, String, EngineState, Stack) {
// Target folder inside assets // Target folder inside assets
let dir = fs::fixtures().join("dotnu_completions"); let dir = fs::fixtures().join("dotnu_completions");
let dir_str = dir let (dir, dir_str, mut engine_state, mut stack) = new_engine_helper(dir);
.clone()
.into_os_string()
.into_string()
.unwrap_or_default();
let dir_span = nu_protocol::Span::new(0, dir_str.len()); 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 // const $NU_LIB_DIRS
let mut working_set = StateWorkingSet::new(&engine_state); let mut working_set = StateWorkingSet::new(&engine_state);
let var_id = working_set.add_variable( 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()); 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( stack.add_env_var(
"NU_LIB_DIRS".into(), "NU_LIB_DIRS".into(),
Value::test_list(vec![ 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) { pub fn new_quote_engine() -> (AbsolutePathBuf, String, EngineState, Stack) {
// Target folder inside assets new_engine_helper(fs::fixtures().join("quoted_completions"))
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)
} }
pub fn new_partial_engine() -> (AbsolutePathBuf, String, EngineState, Stack) { pub fn new_partial_engine() -> (AbsolutePathBuf, String, EngineState, Stack) {
// Target folder inside assets new_engine_helper(fs::fixtures().join("partial_completions"))
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)
} }
/// match a list of suggestions with the expected values /// match a list of suggestions with the expected values