From a1a1c6a050abb078e132c60329399a62b3d16abf 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/getopts.rs | 87 ++++++++++---- brush-core/src/builtins/printf.rs | 4 +- brush-core/src/completion.rs | 113 ++++++++++++------ brush-core/src/shell.rs | 11 +- brush-parser/src/lib.rs | 2 +- brush-parser/src/tokenizer.rs | 17 ++- brush-shell/tests/cases/arrays.yaml | 10 ++ brush-shell/tests/cases/builtins/compgen.yaml | 11 ++ brush-shell/tests/cases/builtins/getopts.yaml | 20 ++++ brush-shell/tests/cases/builtins/printf.yaml | 11 ++ brush-shell/tests/completion_tests.rs | 2 - 12 files changed, 219 insertions(+), 79 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/getopts.rs b/brush-core/src/builtins/getopts.rs index 22a25779..d80652ef 100644 --- a/brush-core/src/builtins/getopts.rs +++ b/brush-core/src/builtins/getopts.rs @@ -18,6 +18,8 @@ pub(crate) struct GetOptsCommand { args: Vec, } +const VAR_GETOPTS_NEXT_CHAR_INDEX: &str = "__GETOPTS_NEXT_CHAR"; + impl builtins::Command for GetOptsCommand { #[allow(clippy::too_many_lines)] async fn execute( @@ -48,7 +50,7 @@ impl builtins::Command for GetOptsCommand { last_char = Some(c); } - // TODO: OPTIND is supposed to be initialized to 1 in each shell/script. + // If unset, assume OPTIND is 1. let mut next_index: usize = context .shell .env @@ -60,9 +62,9 @@ impl builtins::Command for GetOptsCommand { return Ok(builtins::ExitCode::InvalidUsage); } - let new_optarg; + let mut new_optarg = None; let new_optind; - let variable_value; + let mut variable_value; let exit_code; // See if there are any args left to parse. @@ -71,27 +73,52 @@ impl builtins::Command for GetOptsCommand { let arg = self.args[next_index_zero_based].as_str(); // See if this is an option. - if arg.starts_with('-') && arg.len() == 2 && arg != "--" { - // Single character option - let c = arg.chars().nth(1).unwrap(); + if arg.starts_with('-') && arg != "--" { + // Figure out how far into this option we are. + const DEFAULT_NEXT_CHAR_INDEX: usize = 1; + let next_char_index = context + .shell + .env + .get_str(VAR_GETOPTS_NEXT_CHAR_INDEX) + .map_or(DEFAULT_NEXT_CHAR_INDEX, |s| { + s.parse().unwrap_or(DEFAULT_NEXT_CHAR_INDEX) + }); + + // Find the char. + let c = arg.chars().nth(next_char_index).unwrap(); + let is_last_char_in_option = next_char_index == arg.len() - 1; // Look up the char. + let mut is_error = false; if let Some(takes_arg) = args.get(&c) { variable_value = String::from(c); if *takes_arg { - next_index += 1; - next_index_zero_based += 1; - - if next_index_zero_based >= self.args.len() { - return Ok(builtins::ExitCode::Custom(1)); + // If the option takes a value but it's not the last option in this + // argument, then this is an error. + if is_last_char_in_option { + next_index += 1; + next_index_zero_based += 1; + + if next_index_zero_based >= self.args.len() { + return Ok(builtins::ExitCode::Custom(1)); + } + + new_optarg = Some(self.args[next_index_zero_based].clone()); + } else { + is_error = true; } - - new_optarg = Some(self.args[next_index_zero_based].clone()); } else { new_optarg = None; } } else { + // Set to satisfy compiler. + variable_value = String::from("?"); + + is_error = true; + } + + if is_error { // Unknown option; set variable to '?' and OPTARG to the unknown option (sans // hyphen). variable_value = String::from("?"); @@ -101,21 +128,43 @@ impl builtins::Command for GetOptsCommand { new_optarg = None; } + // TODO: honor OPTERR=0 indicating suppression of error messages if treat_unknown_options_as_failure { writeln!(context.stderr(), "getopts: illegal option -- {c}")?; } } - new_optind = next_index + 1; + if is_last_char_in_option { + // We're done with this argument, so unset the internal char index variable + // and request an update to OPTIND. + new_optind = next_index + 1; + context.shell.env.unset(VAR_GETOPTS_NEXT_CHAR_INDEX)?; + } else { + // We have more to go in this argument, so update the internal char index + // and request that OPTIND not be updated. + new_optind = next_index; + context.shell.env.update_or_add( + VAR_GETOPTS_NEXT_CHAR_INDEX, + variables::ShellValueLiteral::Scalar((next_char_index + 1).to_string()), + |_| Ok(()), + crate::env::EnvironmentLookup::Anywhere, + crate::env::EnvironmentScope::Global, + )?; + } + exit_code = builtins::ExitCode::Success; } else { variable_value = String::from("?"); new_optarg = None; + + // If it was "--" we encountered, then skip past it. if arg == "--" { new_optind = next_index + 1; } else { new_optind = next_index; } + + // Note that we're done parsing options. exit_code = builtins::ExitCode::Custom(1); } } else { @@ -156,16 +205,6 @@ impl builtins::Command for GetOptsCommand { crate::env::EnvironmentScope::Global, )?; - // Initialize OPTERR - // TODO: honor OPTERR=0 indicating suppression of error messages - context.shell.env.update_or_add( - "OPTERR", - variables::ShellValueLiteral::Scalar("1".to_string()), - |_| Ok(()), - crate::env::EnvironmentLookup::Anywhere, - crate::env::EnvironmentScope::Global, - )?; - Ok(exit_code) } } 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..450a661d 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 { + // Basic-expand the token-to-be-completed; it won't have been expanded to this point. + 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-core/src/shell.rs b/brush-core/src/shell.rs index 559c4b99..c9ec86d8 100644 --- a/brush-core/src/shell.rs +++ b/brush-core/src/shell.rs @@ -233,12 +233,20 @@ impl Shell { random_var.treat_as_integer(); env.set_global("RANDOM", random_var)?; + // Parsing and completion vars env.set_global("IFS", ShellVariable::new(" \t\n".into()))?; env.set_global( "COMP_WORDBREAKS", ShellVariable::new(" \t\n\"\'><=;|&(:".into()), )?; + // getopts vars + env.set_global("OPTERR", ShellVariable::new("1".into()))?; + let mut optind_var = ShellVariable::new("1".into()); + optind_var.treat_as_integer(); + env.set_global("OPTIND", optind_var)?; + + // OS info vars let os_type = match std::env::consts::OS { "linux" => "linux-gnu", "windows" => "windows", @@ -256,6 +264,7 @@ impl Shell { )?; } } + #[cfg(unix)] if !env.is_set("PATH") { env.set_global( @@ -273,7 +282,7 @@ impl Shell { env.set_global( "BASH_VERSINFO", ShellVariable::new(ShellValue::indexed_array_from_slice( - ["5", "1", "1", "1", "release", "unknown"].as_slice(), + ["5", "2", "15", "1", "release", "unknown"].as_slice(), )), )?; } 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/arrays.yaml b/brush-shell/tests/cases/arrays.yaml index b548d35f..8fb4f104 100644 --- a/brush-shell/tests/cases/arrays.yaml +++ b/brush-shell/tests/cases/arrays.yaml @@ -275,3 +275,13 @@ cases: stdin: | x=(2 3 4) echo ${x[${x[0]}]} + + - name: "Append array-resulting expansion to array" + skip: true # Need to asses on different bash versions + stdin: | + declare -a otherarray=("a" "b" "c") + declare -a ourarray=() + + ourarray+=("${otherarray[@]/a/x}") + declare -p otherarray + declare -p ourarray 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..f2f1db35 100644 --- a/brush-shell/tests/cases/builtins/getopts.yaml +++ b/brush-shell/tests/cases/builtins/getopts.yaml @@ -12,6 +12,7 @@ cases: esac echo "OPTIND is now ${OPTIND}" echo "OPTERR is now ${OPTERR}" + echo "----------------" done echo "Result: $?" echo "option: ${option}" @@ -39,6 +40,7 @@ cases: echo "OPTARG: ${OPTARG}" echo "OPTIND: ${OPTIND}" echo "OPTERR: ${OPTERR}" + echo "----------------" done echo "Done; result: $?" echo "myvar: ${myvar}" @@ -54,6 +56,7 @@ cases: echo "OPTARG: ${OPTARG}" echo "OPTIND: ${OPTIND}" echo "OPTERR: ${OPTERR}" + echo "----------------" done echo "Done; result: $?" echo "myvar: ${myvar}" @@ -69,6 +72,7 @@ cases: echo "OPTARG: ${OPTARG}" echo "OPTIND: ${OPTIND}" echo "OPTERR: ${OPTERR}" + echo "----------------" done echo "Done; result: $?" echo "myvar: ${myvar}" @@ -94,3 +98,19 @@ cases: echo "OPTARG: ${OPTARG}" echo "OPTIND: ${OPTIND}" echo "OPTERR: ${OPTERR}" + + - name: "getopts: multiple options in one token" + stdin: | + while getopts "a:bcdefg" myvar -fedcba something -g; do + echo "Result: $?" + echo "myvar: ${myvar}" + echo "OPTARG: ${OPTARG}" + echo "OPTIND: ${OPTIND}" + echo "OPTERR: ${OPTERR}" + echo "----------------" + 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?;