From 7105591bf3a342cd0003b30487591e25ba974700 Mon Sep 17 00:00:00 2001 From: reuben olinsky Date: Thu, 24 Oct 2024 01:41:26 -0700 Subject: [PATCH] fix: assorted completion-related issues --- brush-core/src/builtins/complete.rs | 10 +- brush-core/src/builtins/printf.rs | 4 +- brush-core/src/completion.rs | 113 ++++++++++++------ brush-parser/src/lib.rs | 2 +- brush-parser/src/tokenizer.rs | 17 ++- brush-shell/tests/cases/builtins/compgen.yaml | 11 ++ brush-shell/tests/cases/builtins/getopts.yaml | 16 +++ brush-shell/tests/cases/builtins/printf.yaml | 11 ++ brush-shell/tests/completion_tests.rs | 2 - 9 files changed, 132 insertions(+), 54 deletions(-) diff --git a/brush-core/src/builtins/complete.rs b/brush-core/src/builtins/complete.rs index dcbf23cd..931d0912 100644 --- a/brush-core/src/builtins/complete.rs +++ b/brush-core/src/builtins/complete.rs @@ -442,15 +442,11 @@ impl builtins::Command for CompGenCommand { let spec = self.common_args.create_spec(); let token_to_complete = self.word.as_deref().unwrap_or_default(); - // N.B. We basic-expand the token-to-be-completed first. - let mut throwaway_shell = context.shell.clone(); - let expanded_token_to_complete = throwaway_shell - .basic_expand_string(token_to_complete) - .await - .unwrap_or_else(|_| token_to_complete.to_owned()); + // We unquote the token-to-be-completed before passing it to the completion system. + let unquoted_token = brush_parser::unquote_str(token_to_complete); let completion_context = completion::Context { - token_to_complete: expanded_token_to_complete.as_str(), + token_to_complete: unquoted_token.as_str(), preceding_token: None, command_name: None, token_index: 0, diff --git a/brush-core/src/builtins/printf.rs b/brush-core/src/builtins/printf.rs index 5e3f4fd8..50b60121 100644 --- a/brush-core/src/builtins/printf.rs +++ b/brush-core/src/builtins/printf.rs @@ -44,9 +44,7 @@ impl PrintfCommand { [fmt, arg] if fmt == "%s" => Ok(arg.clone()), // Special-case invocation of printf with %q-based format string from bash-completion. // It has hard-coded expectation of backslash-style escaping instead of quoting. - [fmt, arg] if fmt == "%q" || fmt == "~%q" => { - Ok(Self::evaluate_format_with_percent_q(None, arg)) - } + [fmt, arg] if fmt == "%q" => Ok(Self::evaluate_format_with_percent_q(None, arg)), [fmt, arg] if fmt == "~%q" => Ok(Self::evaluate_format_with_percent_q(Some("~"), arg)), // Fallback to external command. _ => self.evaluate_via_external_command(context), diff --git a/brush-core/src/completion.rs b/brush-core/src/completion.rs index 716ae3d1..57bd3600 100644 --- a/brush-core/src/completion.rs +++ b/brush-core/src/completion.rs @@ -241,20 +241,16 @@ impl Spec { shell.completion_config.current_completion_options = Some(self.options.clone()); // Generate completions based on any provided actions (and on words). - let mut candidates = self.generate_action_completions(shell, context)?; + let mut candidates = self.generate_action_completions(shell, context).await?; if let Some(word_list) = &self.word_list { let words = crate::expansion::full_expand_and_split_str(shell, word_list).await?; for word in words { - candidates.insert(word); + if word.starts_with(context.token_to_complete) { + candidates.insert(word); + } } } - // Only keep generated completions that match the token being completed. Further - // generations below don't get filtered. - if !context.token_to_complete.is_empty() { - candidates.retain(|candidate| candidate.starts_with(context.token_to_complete)); - } - if let Some(glob_pattern) = &self.glob_pattern { let pattern = patterns::Pattern::from(glob_pattern.as_str()) .set_extended_globbing(shell.options.extended_globbing); @@ -349,23 +345,27 @@ impl Spec { } #[allow(clippy::too_many_lines)] - fn generate_action_completions( + async fn generate_action_completions( &self, shell: &mut Shell, context: &Context<'_>, ) -> Result, error::Error> { let mut candidates = IndexSet::new(); + let token = context.token_to_complete; + for action in &self.actions { match action { CompleteAction::Alias => { for name in shell.aliases.keys() { - candidates.insert(name.to_string()); + if name.starts_with(token) { + candidates.insert(name.to_string()); + } } } CompleteAction::ArrayVar => { for (name, var) in shell.env.iter() { - if var.value().is_array() { + if var.value().is_array() && name.starts_with(token) { candidates.insert(name.to_owned()); } } @@ -375,7 +375,9 @@ impl Spec { } CompleteAction::Builtin => { for name in shell.builtins.keys() { - candidates.insert(name.to_owned()); + if name.starts_with(token) { + candidates.insert(name.to_owned()); + } } } CompleteAction::Command => { @@ -383,32 +385,34 @@ impl Spec { candidates.append(&mut command_completions); } CompleteAction::Directory => { - let mut file_completions = get_file_completions(shell, context, true); + let mut file_completions = + get_file_completions(shell, context.token_to_complete, true).await; candidates.append(&mut file_completions); } CompleteAction::Disabled => { for (name, registration) in &shell.builtins { - if registration.disabled { + if registration.disabled && name.starts_with(token) { candidates.insert(name.to_owned()); } } } CompleteAction::Enabled => { for (name, registration) in &shell.builtins { - if !registration.disabled { + if !registration.disabled && name.starts_with(token) { candidates.insert(name.to_owned()); } } } CompleteAction::Export => { for (key, value) in shell.env.iter() { - if value.is_exported() { + if value.is_exported() && key.starts_with(token) { candidates.insert(key.to_owned()); } } } CompleteAction::File => { - let mut file_completions = get_file_completions(shell, context, false); + let mut file_completions = + get_file_completions(shell, context.token_to_complete, false).await; candidates.append(&mut file_completions); } CompleteAction::Function => { @@ -418,35 +422,50 @@ impl Spec { } CompleteAction::Group => { for group_name in users::get_all_groups()? { - candidates.insert(group_name); + if group_name.starts_with(token) { + candidates.insert(group_name); + } } } CompleteAction::HelpTopic => { // For now, we only have help topics for built-in commands. for name in shell.builtins.keys() { - candidates.insert(name.to_owned()); + if name.starts_with(token) { + candidates.insert(name.to_owned()); + } } } CompleteAction::HostName => { // N.B. We only retrieve one hostname. if let Ok(name) = sys::network::get_hostname() { - candidates.insert(name.to_string_lossy().to_string()); + let name = name.to_string_lossy(); + if name.starts_with(token) { + candidates.insert(name.to_string()); + } } } CompleteAction::Job => { for job in &shell.jobs.jobs { - candidates.insert(job.get_command_name().to_owned()); + let command_name = job.get_command_name(); + if command_name.starts_with(token) { + candidates.insert(command_name.to_owned()); + } } } CompleteAction::Keyword => { for keyword in shell.get_keywords() { - candidates.insert(keyword.clone()); + if keyword.starts_with(token) { + candidates.insert(keyword.clone()); + } } } CompleteAction::Running => { for job in &shell.jobs.jobs { if matches!(job.state, jobs::JobState::Running) { - candidates.insert(job.get_command_name().to_owned()); + let command_name = job.get_command_name(); + if command_name.starts_with(token) { + candidates.insert(command_name.to_owned()); + } } } } @@ -455,34 +474,48 @@ impl Spec { } CompleteAction::SetOpt => { for (name, _) in namedoptions::SET_O_OPTIONS.iter() { - candidates.insert((*name).to_owned()); + if name.starts_with(token) { + candidates.insert((*name).to_owned()); + } } } CompleteAction::ShOpt => { for (name, _) in namedoptions::SHOPT_OPTIONS.iter() { - candidates.insert((*name).to_owned()); + if name.starts_with(token) { + candidates.insert((*name).to_owned()); + } } } CompleteAction::Signal => { for signal in traps::TrapSignal::all_values() { - candidates.insert(signal.to_string()); + let signal_str = signal.to_string(); + if signal_str.starts_with(token) { + candidates.insert(signal_str); + } } } CompleteAction::Stopped => { for job in &shell.jobs.jobs { if matches!(job.state, jobs::JobState::Stopped) { - candidates.insert(job.get_command_name().to_owned()); + let command_name = job.get_command_name(); + if command_name.starts_with(token) { + candidates.insert(job.get_command_name().to_owned()); + } } } } CompleteAction::User => { for user_name in users::get_all_users()? { - candidates.insert(user_name); + if user_name.starts_with(token) { + candidates.insert(user_name); + } } } CompleteAction::Variable => { for (key, _) in shell.env.iter() { - candidates.insert(key.to_owned()); + if key.starts_with(token) { + candidates.insert(key.to_owned()); + } } } } @@ -907,17 +940,27 @@ impl Config { }) } else { // If we didn't find a spec, then fall back to basic completion. - get_completions_using_basic_lookup(shell, &context) + get_completions_using_basic_lookup(shell, &context).await } } } -fn get_file_completions(shell: &Shell, context: &Context, must_be_dir: bool) -> IndexSet { - let glob = std::format!("{}*", context.token_to_complete); +async fn get_file_completions( + shell: &Shell, + token_to_complete: &str, + must_be_dir: bool, +) -> IndexSet { + // DBG:RRO + let mut throwaway_shell = shell.clone(); + let expanded_token = throwaway_shell + .basic_expand_string(token_to_complete) + .await + .unwrap_or_else(|_err| token_to_complete.to_owned()); + + let glob = std::format!("{expanded_token}*"); let path_filter = |path: &Path| !must_be_dir || shell.get_absolute_path(path).is_dir(); - // TODO: Pass through quoting. let pattern = patterns::Pattern::from(glob).set_extended_globbing(shell.options.extended_globbing); @@ -942,8 +985,8 @@ fn get_command_completions(shell: &Shell, context: &Context) -> IndexSet candidates.into_iter().collect() } -fn get_completions_using_basic_lookup(shell: &Shell, context: &Context) -> Answer { - let mut candidates = get_file_completions(shell, context, false); +async fn get_completions_using_basic_lookup(shell: &Shell, context: &Context<'_>) -> Answer { + let mut candidates = get_file_completions(shell, context.token_to_complete, false).await; // If this appears to be the command token (and if there's *some* prefix without // a path separator) then also consider whether we should search the path for diff --git a/brush-parser/src/lib.rs b/brush-parser/src/lib.rs index 175573b7..55bb9dde 100644 --- a/brush-parser/src/lib.rs +++ b/brush-parser/src/lib.rs @@ -15,4 +15,4 @@ mod tokenizer; pub use error::{ParseError, TestCommandParseError, WordParseError}; pub use parser::{parse_tokens, Parser, ParserOptions, SourceInfo}; -pub use tokenizer::{tokenize_str, SourcePosition, Token, TokenLocation}; +pub use tokenizer::{tokenize_str, unquote_str, SourcePosition, Token, TokenLocation}; diff --git a/brush-parser/src/tokenizer.rs b/brush-parser/src/tokenizer.rs index 3b0adffa..50170477 100644 --- a/brush-parser/src/tokenizer.rs +++ b/brush-parser/src/tokenizer.rs @@ -553,7 +553,7 @@ impl<'a, R: ?Sized + std::io::BufRead> Tokenizer<'a, R> { let next_here_tag = &self.cross_state.current_here_tags[0]; let tag_str: Cow<'_, str> = if next_here_tag.tag_was_escaped_or_quoted { - remove_quotes_from(next_here_tag.tag.as_str()).into() + unquote_str(next_here_tag.tag.as_str()).into() } else { next_here_tag.tag.as_str().into() }; @@ -1010,7 +1010,12 @@ fn is_quoting_char(c: char) -> bool { matches!(c, '\\' | '\'' | '\"') } -fn remove_quotes_from(s: &str) -> String { +/// Return a string with all the quoting removed. +/// +/// # Arguments +/// +/// * `s` - The string to unquote. +pub fn unquote_str(s: &str) -> String { let mut result = String::new(); let mut in_escape = false; @@ -1392,9 +1397,9 @@ SOMETHING #[test] fn test_quote_removal() { - assert_eq!(remove_quotes_from(r#""hello""#), "hello"); - assert_eq!(remove_quotes_from(r#"'hello'"#), "hello"); - assert_eq!(remove_quotes_from(r#""hel\"lo""#), r#"hel"lo"#); - assert_eq!(remove_quotes_from(r#"'hel\'lo'"#), r#"hel'lo"#); + assert_eq!(unquote_str(r#""hello""#), "hello"); + assert_eq!(unquote_str(r#"'hello'"#), "hello"); + assert_eq!(unquote_str(r#""hel\"lo""#), r#"hel"lo"#); + assert_eq!(unquote_str(r#"'hel\'lo'"#), r#"hel'lo"#); } } diff --git a/brush-shell/tests/cases/builtins/compgen.yaml b/brush-shell/tests/cases/builtins/compgen.yaml index b224f056..91d53050 100644 --- a/brush-shell/tests/cases/builtins/compgen.yaml +++ b/brush-shell/tests/cases/builtins/compgen.yaml @@ -101,3 +101,14 @@ cases: for p in $(compgen -f '$HOME/'); do echo ${p//$HOME/HOME} done + + - name: "compgen -f with quoted + escaped var" + known_failure: true + stdin: | + touch item1 + HOME=$(pwd) + + echo "[0]" + for p in $(compgen -f '\$HOME/'); do + echo ${p//$HOME/HOME} + done diff --git a/brush-shell/tests/cases/builtins/getopts.yaml b/brush-shell/tests/cases/builtins/getopts.yaml index 8e692113..ffbfc60a 100644 --- a/brush-shell/tests/cases/builtins/getopts.yaml +++ b/brush-shell/tests/cases/builtins/getopts.yaml @@ -94,3 +94,19 @@ cases: echo "OPTARG: ${OPTARG}" echo "OPTIND: ${OPTIND}" echo "OPTERR: ${OPTERR}" + + - name: "getopts: multiple options in one token" + known_failure: true + stdin: | + while getopts "ab:" myvar -ab something; do + echo "Result: $?" + echo "myvar: ${myvar}" + echo "OPTARG: ${OPTARG}" + echo "OPTIND: ${OPTIND}" + echo "OPTERR: ${OPTERR}" + done + echo "Done; result: $?" + echo "myvar: ${myvar}" + echo "OPTARG: ${OPTARG}" + echo "OPTIND: ${OPTIND}" + echo "OPTERR: ${OPTERR}" diff --git a/brush-shell/tests/cases/builtins/printf.yaml b/brush-shell/tests/cases/builtins/printf.yaml index 5bf69df4..e0fb9397 100644 --- a/brush-shell/tests/cases/builtins/printf.yaml +++ b/brush-shell/tests/cases/builtins/printf.yaml @@ -33,3 +33,14 @@ cases: echo "[3]" printf "%q" '"'; echo + + - name: "printf ~%q" + stdin: | + echo "[1]" + printf "~%q" 'TEXT'; echo + + echo "[2]" + printf "~%q" '$VAR'; echo + + echo "[3]" + printf "~%q" '"'; echo diff --git a/brush-shell/tests/completion_tests.rs b/brush-shell/tests/completion_tests.rs index a9b16a01..0e444765 100644 --- a/brush-shell/tests/completion_tests.rs +++ b/brush-shell/tests/completion_tests.rs @@ -148,7 +148,6 @@ async fn complete_absolute_paths() -> Result<()> { Ok(()) } -#[ignore] // TODO: Fix this for newer versions of bash-completion #[tokio::test] async fn complete_path_with_var() -> Result<()> { let mut test_shell = TestShellWithBashCompletion::new().await?; @@ -182,7 +181,6 @@ async fn complete_path_with_var() -> Result<()> { Ok(()) } -#[ignore] // TODO: Fix this for newer versions of bash-completion #[tokio::test] async fn complete_path_with_tilde() -> Result<()> { let mut test_shell = TestShellWithBashCompletion::new().await?;