From d50c12e142b86393166399e39eb70038ac530f81 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Wed, 17 Jun 2026 11:48:54 +0800 Subject: [PATCH] Better-named and idiomatic return type for cmdsub parsing Commit 2f6ed618335 (parse_util_cmdsubst_extent to return an exclusive range, 2024-04-27) used the "Parentheses" naming in preparation for raw quotes. We still don't have them, so let's clean that up for now. Also, reuse the Result and Option types. --- src/expand.rs | 54 ++++++++++++++++--------- src/highlight/highlight.rs | 23 +++++------ src/parse_execution.rs | 6 +-- src/parse_util.rs | 82 ++++++++++++++++++-------------------- src/reader/reader.rs | 18 ++++----- 5 files changed, 96 insertions(+), 87 deletions(-) diff --git a/src/expand.rs b/src/expand.rs index 0ab2bdf50..d1897b5ab 100644 --- a/src/expand.rs +++ b/src/expand.rs @@ -16,7 +16,7 @@ history::{History, history_id}, operation_context::OperationContext, parse_constants::{ParseError, ParseErrorCode, ParseErrorList, SOURCE_LOCATION_UNKNOWN}, - parse_util::{MaybeParentheses, expand_variable_error, locate_cmdsubst_range}, + parse_util::{expand_variable_error, locate_cmdsubst_range}, path::path_apply_working_directory, prelude::*, wildcard::{WildcardResult, wildcard_expand_string, wildcard_has_internal}, @@ -900,30 +900,30 @@ pub fn expand_cmdsubst( let mut cursor = 0; let mut is_quoted = false; let mut has_dollar = false; - let parens = match locate_cmdsubst_range( + let cmdsub = match locate_cmdsubst_range( &input, &mut cursor, false, Some(&mut is_quoted), Some(&mut has_dollar), ) { - MaybeParentheses::Error => { + Err(()) => { append_syntax_error!(errors, SOURCE_LOCATION_UNKNOWN, "Mismatched parenthesis"); return ExpandResult::make_error(STATUS_EXPAND_ERROR); } - MaybeParentheses::None => { + Ok(None) => { if !out.add(input) { return append_overflow_error(errors, None); } return ExpandResult::ok(); } - MaybeParentheses::CommandSubstitution(parens) => parens, + Ok(Some(cmdsub)) => cmdsub, }; let mut sub_res = vec![]; let job_group = ctx.job_group.clone(); let subshell_status = exec_subshell_for_expand( - &input[parens.command()], + &input[cmdsub.command_range()], ctx.parser(), job_group.as_ref(), &mut sub_res, @@ -974,12 +974,17 @@ pub fn expand_cmdsubst( wgettext!("Unknown error while evaluating command substitution") } }; - append_cmdsub_error_formatted!(errors, parens.start(), parens.end() - 1, err.to_owned()); + append_cmdsub_error_formatted!( + errors, + cmdsub.opening_paren_offset(), + cmdsub.end() - 1, + err.to_owned() + ); return ExpandResult::make_error(subshell_status); } // Expand slices like (cat /var/words)[1] - let mut tail_begin = parens.end(); + let mut tail_begin = cmdsub.end(); if input.as_char_slice().get(tail_begin) == Some(&'[') { let mut slice_idx = vec![]; let slice_begin = tail_begin; @@ -1052,9 +1057,15 @@ pub fn expand_cmdsubst( for tail_item in tail_expand { let mut whole_item = WString::new(); whole_item.reserve( - parens.start() + 1 + sub_res_joined.len() + 1 + tail_item.completion.len(), + cmdsub.opening_paren_offset() + + 1 + + sub_res_joined.len() + + 1 + + tail_item.completion.len(), + ); + whole_item.push_utfstr( + &input[..cmdsub.opening_paren_offset() - if has_dollar { 1 } else { 0 }], ); - whole_item.push_utfstr(&input[..parens.start() - if has_dollar { 1 } else { 0 }]); whole_item.push(INTERNAL_SEPARATOR); whole_item.push_utfstr(&sub_res_joined); whole_item.push(INTERNAL_SEPARATOR); @@ -1071,9 +1082,16 @@ pub fn expand_cmdsubst( let sub_item2 = escape_string(&sub_item, EscapeStringStyle::Script(EscapeFlags::COMMA)); for tail_item in &*tail_expand { let mut whole_item = WString::new(); - whole_item - .reserve(parens.start() + 1 + sub_item2.len() + 1 + tail_item.completion.len()); - whole_item.push_utfstr(&input[..parens.start() - if has_dollar { 1 } else { 0 }]); + whole_item.reserve( + cmdsub.opening_paren_offset() + + 1 + + sub_item2.len() + + 1 + + tail_item.completion.len(), + ); + whole_item.push_utfstr( + &input[..cmdsub.opening_paren_offset() - if has_dollar { 1 } else { 0 }], + ); whole_item.push(INTERNAL_SEPARATOR); whole_item.push_utfstr(&sub_item2); whole_item.push(INTERNAL_SEPARATOR); @@ -1287,18 +1305,18 @@ fn stage_cmdsubst(&mut self, input: WString, out: &mut CompletionReceiver) -> Ex if self.flags.contains(ExpandFlags::FAIL_ON_CMDSUBST) { let mut cursor = 0; match locate_cmdsubst_range(&input, &mut cursor, true, None, None) { - MaybeParentheses::Error => ExpandResult::make_error(STATUS_EXPAND_ERROR), - MaybeParentheses::None => { + Err(()) => ExpandResult::make_error(STATUS_EXPAND_ERROR), + Ok(None) => { if !out.add(input) { return append_overflow_error(self.errors, None); } ExpandResult::ok() } - MaybeParentheses::CommandSubstitution(parens) => { + Ok(Some(cmdsub)) => { append_cmdsub_error!( self.errors, - parens.start(), - parens.end() - 1, + cmdsub.opening_paren_offset(), + cmdsub.end() - 1, "command substitutions not allowed in command position. Try var=(your-cmd) $var ..." ); ExpandResult::make_error(STATUS_EXPAND_ERROR) diff --git a/src/highlight/highlight.rs b/src/highlight/highlight.rs index d6417055c..e733128e7 100644 --- a/src/highlight/highlight.rs +++ b/src/highlight/highlight.rs @@ -17,9 +17,7 @@ parse_constants::{ ParseKeyword, ParseTokenType, ParseTreeFlags, SourceRange, StatementDecoration, }, - parse_util::{ - MaybeParentheses, get_process_first_token_offset, locate_cmdsubst_range, slice_length, - }, + parse_util::{get_process_first_token_offset, locate_cmdsubst_range, slice_length}, path::{path_as_implicit_cd, path_get_cdpath, path_get_path, paths_are_same_file}, terminal::Outputter, text_face::{ResettableStyle, SpecifiedTextFace, TextFace, UnderlineStyle, parse_text_face}, @@ -839,7 +837,7 @@ fn color_as_argument(&mut self, node: &dyn ast::Node, options_allowed: bool /* = // Now do command substitutions. let mut cmdsub_cursor = 0; let mut is_quoted = false; - while let MaybeParentheses::CommandSubstitution(parens) = locate_cmdsubst_range( + while let Ok(Some(cmdsub)) = locate_cmdsubst_range( arg_str, &mut cmdsub_cursor, /*accept_incomplete=*/ true, @@ -848,17 +846,17 @@ fn color_as_argument(&mut self, node: &dyn ast::Node, options_allowed: bool /* = ) { // Highlight the parens. The open parens must exist; the closed paren may not if it was // incomplete. - assert!(parens.start() < arg_str.len()); - self.color_array[arg_start..][parens.opening()] + assert!(cmdsub.opening_paren_offset() < arg_str.len()); + self.color_array[arg_start..][cmdsub.opening_paren_range()] .fill(HighlightSpec::with_fg(HighlightRole::Operat)); - self.color_array[arg_start..][parens.closing()] + self.color_array[arg_start..][cmdsub.closing_paren_range()] .fill(HighlightSpec::with_fg(HighlightRole::Operat)); // Highlight it recursively. let arg_cursor = self .cursor - .map(|c| c.wrapping_sub(arg_start + parens.start())); - let cmdsub_contents = &arg_str[parens.command()]; + .map(|c| c.wrapping_sub(arg_start + cmdsub.opening_paren_offset())); + let cmdsub_contents = &arg_str[cmdsub.command_range()]; let mut cmdsub_highlighter = Highlighter::new( cmdsub_contents, arg_cursor, @@ -870,7 +868,7 @@ fn color_as_argument(&mut self, node: &dyn ast::Node, options_allowed: bool /* = // Copy out the subcolors back into our array. assert_eq!(subcolors.len(), cmdsub_contents.len()); - self.color_array[arg_start..][parens.command()].copy_from_slice(&subcolors); + self.color_array[arg_start..][cmdsub.command_range()].copy_from_slice(&subcolors); } } // Colors the source range of a node with a given color. @@ -1135,9 +1133,8 @@ fn visit_brace_statement(&mut self, brace_statement: &BraceStatement) { fn has_cmdsub(src: &wstr) -> bool { let mut cursor = 0; match locate_cmdsubst_range(src, &mut cursor, true, None, None) { - MaybeParentheses::Error => false, - MaybeParentheses::None => false, - MaybeParentheses::CommandSubstitution(_) => true, + Err(()) | Ok(None) => false, + Ok(Some(_)) => true, } } diff --git a/src/parse_execution.rs b/src/parse_execution.rs index 74978abdc..3650a41ef 100644 --- a/src/parse_execution.rs +++ b/src/parse_execution.rs @@ -31,9 +31,7 @@ ParseTokenType, StatementDecoration, parse_error_offset_source_start, }, parse_tree::{NodeRef, ParsedSourceRef}, - parse_util::{ - MaybeParentheses::CommandSubstitution, locate_cmdsubst_range, unescape_wildcards, - }, + parse_util::{locate_cmdsubst_range, unescape_wildcards}, parser::{ Block, BlockData, BlockId, BlockType, LoopStatus, Parser, ParserEnvSetMode, ProfileItem, }, @@ -1474,7 +1472,7 @@ fn determine_redirections( None, None, ) { - CommandSubstitution(p) => p.start() == 0, + Ok(Some(cmdsub)) => cmdsub.opening_paren_offset() == 0, _ => false, } } { diff --git a/src/parse_util.rs b/src/parse_util.rs index 90cef2619..4769b2750 100644 --- a/src/parse_util.rs +++ b/src/parse_util.rs @@ -86,36 +86,29 @@ pub fn slice_length(input: &wstr) -> Option { } #[derive(Debug, Default, Eq, PartialEq)] -pub struct Parentheses { +pub struct CommandSubstitution { range: Range, num_closing: usize, } -impl Parentheses { - pub fn start(&self) -> usize { +impl CommandSubstitution { + pub fn opening_paren_offset(&self) -> usize { self.range.start } pub fn end(&self) -> usize { self.range.end } - pub fn opening(&self) -> Range { + pub fn opening_paren_range(&self) -> Range { self.range.start..self.range.start + 1 } - pub fn closing(&self) -> Range { + pub fn closing_paren_range(&self) -> Range { self.range.end - self.num_closing..self.range.end } - pub fn command(&self) -> Range { + pub fn command_range(&self) -> Range { self.range.start + 1..self.range.end - self.num_closing } } -#[derive(Eq, PartialEq, Debug)] -pub enum MaybeParentheses { - Error, - None, - CommandSubstitution(Parentheses), -} - /// Alternative API. Iterate over command substitutions. /// /// \param str the string to search for subshells @@ -130,16 +123,17 @@ pub enum MaybeParentheses { /// \param out_has_dollar whether the command substitution has the optional leading $. /// Return -1 on syntax error, 0 if no subshells exist and 1 on success #[allow(clippy::too_many_arguments)] +#[allow(clippy::result_unit_err)] pub fn locate_cmdsubst_range( s: &wstr, inout_cursor_offset: &mut usize, accept_incomplete: bool, inout_is_quoted: Option<&mut bool>, out_has_dollar: Option<&mut bool>, -) -> MaybeParentheses { +) -> Result, ()> { // Nothing to do if the offset is at or past the end of the string. if *inout_cursor_offset >= s.len() { - return MaybeParentheses::None; + return Ok(None); } // Defer to the wonky version. @@ -151,9 +145,9 @@ pub fn locate_cmdsubst_range( out_has_dollar, ); match &ret { - MaybeParentheses::Error | MaybeParentheses::None => (), - MaybeParentheses::CommandSubstitution(parens) => { - *inout_cursor_offset = parens.end(); + Err(()) | Ok(None) => (), + Ok(Some(cmdsub)) => { + *inout_cursor_offset = cmdsub.end(); } } ret @@ -173,13 +167,13 @@ pub fn get_cmdsubst_extent(buff: &wstr, cursor: usize) -> ops::Range { let mut result = 0..buff.len(); let mut pos = 0; loop { - let parens = match locate_cmdsub(buff, pos, true, None, None) { + let cmdsub = match locate_cmdsub(buff, pos, true, None, None) { // No subshell found, all done. - MaybeParentheses::Error | MaybeParentheses::None => break, - MaybeParentheses::CommandSubstitution(parens) => parens, + Err(()) | Ok(None) => break, + Ok(Some(cmdsub)) => cmdsub, }; - let command = parens.command(); + let command = cmdsub.command_range(); if command.start <= cursor && command.end >= cursor { // This command substitution surrounds the cursor, so it's a tighter fit. result = command; @@ -196,7 +190,7 @@ pub fn get_cmdsubst_extent(buff: &wstr, cursor: usize) -> ops::Range { } else { // This command substitution ends before the cursor. Skip it. assert!(command.end < cursor); - pos = parens.end(); + pos = cmdsub.end(); assert!(pos <= buff.len()); } } @@ -209,7 +203,7 @@ fn locate_cmdsub( allow_incomplete: bool, mut inout_is_quoted: Option<&mut bool>, mut out_has_dollar: Option<&mut bool>, -) -> MaybeParentheses { +) -> Result, ()> { let input = input.as_char_slice(); let mut escaped = false; @@ -352,11 +346,11 @@ fn process_opening_quote( syntax_error |= paran_count > 0 && !allow_incomplete; if syntax_error { - return MaybeParentheses::Error; + return Err(()); } let Some(paran_begin) = paran_begin else { - return MaybeParentheses::None; + return Ok(None); }; let end = if paran_count != 0 { @@ -365,12 +359,12 @@ fn process_opening_quote( paran_end.unwrap() + 1 }; - let parens = Parentheses { + let cmdsub = CommandSubstitution { range: paran_begin..end, num_closing: if paran_count == 0 { 1 } else { 0 }, }; - MaybeParentheses::CommandSubstitution(parens) + Ok(Some(cmdsub)) } /// Find the beginning and end of the process definition under the cursor @@ -869,21 +863,21 @@ fn indent_leaf(&mut self, range: SourceRange) { let mut was_double_quoted; loop { was_double_quoted = is_double_quoted; - let parens = match locate_cmdsubst_range( + let cmdsub = match locate_cmdsubst_range( node_src, &mut cursor, /*accept_incomplete=*/ true, Some(&mut is_double_quoted), None, ) { - MaybeParentheses::Error => break, - MaybeParentheses::None => { + Err(()) => break, + Ok(None) => { break; } - MaybeParentheses::CommandSubstitution(parens) => parens, + Ok(Some(cmdsub)) => cmdsub, }; - let command = parens.command(); + let command = cmdsub.command_range(); self.indent_string_part(done..range.start() + command.start, was_double_quoted); let cmdsub_contents = &node_src[command.clone()]; let indents = compute_indents_from(cmdsub_contents, self.indent + 1); @@ -891,7 +885,7 @@ fn indent_leaf(&mut self, range: SourceRange) { .copy_from_slice(&indents); done = range.start() + command.end; - if parens.closing().is_empty() { + if cmdsub.closing_paren_range().is_empty() { self.unclosed = true; } } @@ -1402,24 +1396,26 @@ pub fn detect_errors_in_argument( Some(&mut is_quoted), Some(&mut has_dollar), ) { - MaybeParentheses::Error => { + Err(()) => { issue.error = true; append_syntax_error!(out_errors, source_start, 1, "Mismatched parenthesis"); return Err(issue); } - MaybeParentheses::None => { + Ok(None) => { do_loop = false; } - MaybeParentheses::CommandSubstitution(parens) => { + Ok(Some(cmdsub)) => { issue.error |= check_subtoken( checked, - parens.start() - if has_dollar { 1 } else { 0 }, + cmdsub.opening_paren_offset() - if has_dollar { 1 } else { 0 }, out_errors, ); let mut subst_errors = ParseErrorList::new(); - if let Err(e) = - detect_parse_errors(&arg_src[parens.command()], Some(&mut subst_errors), false) - { + if let Err(e) = detect_parse_errors( + &arg_src[cmdsub.command_range()], + Some(&mut subst_errors), + false, + ) { issue.error |= e.error; issue.incomplete |= e.incomplete; } @@ -1427,13 +1423,13 @@ pub fn detect_errors_in_argument( // Our command substitution produced error offsets relative to its source. Tweak the // offsets of the errors in the command substitution to account for both its offset // within the string, and the offset of the node. - let error_offset = parens.start() + 1 + source_start; + let error_offset = cmdsub.opening_paren_offset() + 1 + source_start; parse_error_offset_source_start(&mut subst_errors, error_offset); if let Some(out_errors) = out_errors { out_errors.extend(subst_errors); } - checked = parens.end(); + checked = cmdsub.end(); } } } diff --git a/src/reader/reader.rs b/src/reader/reader.rs index 5e7229b6d..afca41fbf 100644 --- a/src/reader/reader.rs +++ b/src/reader/reader.rs @@ -68,10 +68,10 @@ panic::AT_EXIT, parse_constants::{ParseIssue, ParseTreeFlags, SourceRange}, parse_util::{ - MaybeParentheses, SPACES_PER_INDENT, compute_indents, contains_wildcards, - detect_parse_errors, escape_wildcards, get_cmdsubst_extent, get_line_from_offset, - get_offset, get_offset_from_line, get_process_extent, get_process_first_token_offset, - get_token_extent, lineno, locate_cmdsubst_range, + SPACES_PER_INDENT, compute_indents, contains_wildcards, detect_parse_errors, + escape_wildcards, get_cmdsubst_extent, get_line_from_offset, get_offset, + get_offset_from_line, get_process_extent, get_process_first_token_offset, get_token_extent, + lineno, locate_cmdsubst_range, }, parser::{BlockType, EvalRes, Parser, ParserEnvSetMode}, portable_atomic::AtomicU64, @@ -6084,14 +6084,14 @@ fn extract_tokens(s: &wstr) -> Vec { None, None, ) { - MaybeParentheses::Error | MaybeParentheses::None => break, - MaybeParentheses::CommandSubstitution(parens) => { - if parens.start() >= range.end() { + Err(()) | Ok(None) => break, + Ok(Some(cmdsub)) => { + if cmdsub.opening_paren_offset() >= range.end() { break; } has_cmd_subs = true; - for mut t in extract_tokens(&s[parens.command()]) { - t.range.start += u32::try_from(parens.command().start).unwrap(); + for mut t in extract_tokens(&s[cmdsub.command_range()]) { + t.range.start += u32::try_from(cmdsub.command_range().start).unwrap(); result.push(t); } }