From efce176ceb1d56cbbcce7e7821b29bac503c2bf7 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Mon, 13 Jan 2025 06:25:35 +0100 Subject: [PATCH] Support help argument in "{ -h" Unlike other builtins, "{" is a separate token, not a keyword-string token. Allow the left brace token as command string; produce it when parsing "{ -h"/"{ --help" (and nowhere else). By using a decorated statement, we reuse logic for redirections etc. Other syntax elements like "and" are in the builtin list, which - adds highlighting logic - adds it to "builtin --names" - makes it runnable as builtin (e.g. "builtin '{'" would hypothetically print the man page) These don't seem very important (highlighting for '{' needs to match '}' anyway). Additionally, making it a real builtin would mean that we'd need to deactivate a few places that unescape "{" to BRACE_BEGIN. Let's not add it to the built in list. Instead, simply synthesize builtin_generic in the right spot. I'm assuming we want "{ -h" to print help, but '"{" -h' to run an external command, since the latter is historical behavior. This works naturally with the above fake builtin approach which never tries to unescape the left brace. --- share/functions/__fish_print_help.fish | 2 + src/ast.rs | 125 ++++++++++++++++++++++++- src/builtins/fish_indent.rs | 1 + src/builtins/shared.rs | 13 ++- src/highlight/highlight.rs | 2 +- src/parse_execution.rs | 36 ++++--- src/parse_util.rs | 2 +- src/reader.rs | 2 +- src/tests/parser.rs | 1 + tests/checks/braces.fish | 18 +++- tests/checks/indent.fish | 3 + 11 files changed, 180 insertions(+), 25 deletions(-) diff --git a/share/functions/__fish_print_help.fish b/share/functions/__fish_print_help.fish index a4888898f..e30f2a93b 100644 --- a/share/functions/__fish_print_help.fish +++ b/share/functions/__fish_print_help.fish @@ -8,6 +8,8 @@ function __fish_print_help --description "Print help message for the specified f set item true case '[' set item test + case '{' + set item begin end # Do nothing if the file does not exist diff --git a/src/ast.rs b/src/ast.rs index 93c8bba0e..9b4825940 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -71,6 +71,7 @@ fn visit_block_statement_header( &mut self, _node: &mut BlockStatementHeaderVariant, ) -> VisitResult; + fn visit_command_token(&mut self, _node: &mut CommandTokenVariant) -> VisitResult; fn visit_statement(&mut self, _node: &mut StatementVariant) -> VisitResult; fn visit_decorated_statement_decorator( @@ -179,6 +180,9 @@ fn as_argument_or_redirection(&self) -> Option<&ArgumentOrRedirection> { fn as_argument_or_redirection_list(&self) -> Option<&ArgumentOrRedirectionList> { None } + fn as_command_token(&self) -> Option<&CommandToken> { + None + } fn as_statement(&self) -> Option<&Statement> { None } @@ -300,6 +304,9 @@ fn as_mut_argument_or_redirection(&mut self) -> Option<&mut ArgumentOrRedirectio fn as_mut_argument_or_redirection_list(&mut self) -> Option<&mut ArgumentOrRedirectionList> { None } + fn as_mut_command_token(&mut self) -> Option<&mut CommandToken> { + None + } fn as_mut_statement(&mut self) -> Option<&mut Statement> { None } @@ -923,6 +930,9 @@ macro_rules! visit_variant_field_mut { (BlockStatementHeaderVariant, $visitor:ident, $field:expr) => { $visitor.visit_block_statement_header(&mut $field) }; + (CommandTokenVariant, $visitor:ident, $field:expr) => { + $visitor.visit_command_token(&mut $field) + }; (StatementVariant, $visitor:ident, $field:expr) => { $visitor.visit_statement(&mut $field) }; @@ -1023,6 +1033,19 @@ macro_rules! set_parent_of_union_field { $self.$field_name.as_mut_redirection().set_parents(); } }; + ( + $self:ident, + $field_name:ident, + CommandTokenVariant + ) => { + if matches!($self.$field_name, CommandTokenVariant::LeftBrace(_)) { + $self.$field_name.as_mut_left_brace().parent = Some($self); + $self.$field_name.as_mut_left_brace().set_parents(); + } else { + $self.$field_name.as_mut_string().parent = Some($self); + $self.$field_name.as_mut_string().set_parents(); + } + }; ( $self:ident, $field_name:ident, @@ -1164,6 +1187,38 @@ fn as_mut_argument_or_redirection_list(&mut self) -> Option<&mut ArgumentOrRedir } } +#[derive(Default, Debug)] +pub struct CommandToken { + parent: Option<*const dyn Node>, + pub contents: CommandTokenVariant, +} +implement_node!(CommandToken, branch, command_token); +implement_acceptor_for_branch!( + CommandToken, + (contents: (variant)) +); +impl ConcreteNode for CommandToken { + fn as_command_token(&self) -> Option<&CommandToken> { + Some(self) + } +} +impl ConcreteNodeMut for CommandToken { + fn as_mut_command_token(&mut self) -> Option<&mut CommandToken> { + Some(self) + } +} +impl CheckParse for CommandToken { + fn can_be_parsed(pop: &mut Populator<'_>) -> bool { + let typ = pop.peek_type(0); + matches!(typ, ParseTokenType::string | ParseTokenType::left_brace) + } +} +impl CommandToken { + pub fn is_left_brace(&self) -> bool { + matches!(&self.contents, CommandTokenVariant::LeftBrace(_)) + } +} + /// A statement is a normal command, or an if / while / etc #[derive(Default, Debug)] pub struct Statement { @@ -1652,7 +1707,7 @@ pub struct DecoratedStatement { /// An optional decoration (command, builtin, exec, etc). pub opt_decoration: Option, /// Command to run. - pub command: String_, + pub command: CommandToken, /// Args and redirs pub args_or_redirs: ArgumentOrRedirectionList, } @@ -1660,7 +1715,7 @@ pub struct DecoratedStatement { implement_acceptor_for_branch!( DecoratedStatement, (opt_decoration: (Option)), - (command: (String_)), + (command: (CommandToken)), (args_or_redirs: (ArgumentOrRedirectionList)), ); impl ConcreteNode for DecoratedStatement { @@ -2171,6 +2226,56 @@ pub fn redirection(&self) -> &Redirection { } } +#[derive(Debug)] +pub enum CommandTokenVariant { + String(String_), + LeftBrace(TokenLeftBrace), +} + +impl Default for CommandTokenVariant { + fn default() -> Self { + CommandTokenVariant::String(String_::default()) + } +} + +impl Acceptor for CommandTokenVariant { + fn accept<'a>(&'a self, visitor: &mut dyn NodeVisitor<'a>, reversed: bool) { + match self { + CommandTokenVariant::String(child) => child.accept(visitor, reversed), + CommandTokenVariant::LeftBrace(child) => child.accept(visitor, reversed), + } + } +} +impl AcceptorMut for CommandTokenVariant { + fn accept_mut(&mut self, visitor: &mut dyn NodeVisitorMut, reversed: bool) { + match self { + CommandTokenVariant::String(child) => child.accept_mut(visitor, reversed), + CommandTokenVariant::LeftBrace(child) => child.accept_mut(visitor, reversed), + } + } +} + +impl CommandTokenVariant { + pub fn embedded_node(&self) -> &dyn Node { + match self { + CommandTokenVariant::String(node) => node, + CommandTokenVariant::LeftBrace(node) => node, + } + } + fn as_mut_string(&mut self) -> &mut String_ { + match self { + CommandTokenVariant::String(node) => node, + _ => panic!(), + } + } + fn as_mut_left_brace(&mut self) -> &mut TokenLeftBrace { + match self { + CommandTokenVariant::LeftBrace(node) => node, + _ => panic!(), + } + } +} + #[derive(Debug)] pub enum BlockStatementHeaderVariant { None, @@ -2425,6 +2530,7 @@ pub fn ast_type_to_string(t: Type) -> &'static wstr { Type::variable_assignment_list => L!("variable_assignment_list"), Type::argument_or_redirection => L!("argument_or_redirection"), Type::argument_or_redirection_list => L!("argument_or_redirection_list"), + Type::command_token => L!("command_token"), Type::statement => L!("statement"), Type::job_pipeline => L!("job_pipeline"), Type::job_conjunction => L!("job_conjunction"), @@ -3105,6 +3211,17 @@ fn visit_block_statement_header( *node = self.allocate_populate_block_header(); VisitResult::Continue(()) } + fn visit_command_token(&mut self, node: &mut CommandTokenVariant) -> VisitResult { + if let Some(lbrace) = self.try_parse::() { + *node = CommandTokenVariant::LeftBrace(*lbrace); + } else if let Some(s) = self.try_parse::() { + *node = CommandTokenVariant::String(*s); + } else { + // On error, we return an unsourced decorated statement. + *node = CommandTokenVariant::String(Default::default()); + } + ControlFlow::Continue(()) + } fn visit_statement(&mut self, node: &mut StatementVariant) -> VisitResult { *node = self.allocate_populate_statement_contents(); VisitResult::Continue(()) @@ -3653,6 +3770,9 @@ fn new_decorated_statement(slf: &mut Populator<'_>) -> StatementVariant { // Construct a decorated statement, which will be unsourced. self.allocate_visit::(); } else if self.peek_token(0).typ == ParseTokenType::left_brace { + if self.peek_token(1).is_help_argument { + return new_decorated_statement(self); + } let embedded = self.allocate_visit::(); return StatementVariant::BraceStatement(embedded); } else if self.peek_token(0).typ != ParseTokenType::string { @@ -4134,6 +4254,7 @@ pub enum Type { variable_assignment_list, argument_or_redirection, argument_or_redirection_list, + command_token, statement, job_pipeline, job_conjunction, diff --git a/src/builtins/fish_indent.rs b/src/builtins/fish_indent.rs index c41a84c94..90125b3f4 100644 --- a/src/builtins/fish_indent.rs +++ b/src/builtins/fish_indent.rs @@ -342,6 +342,7 @@ fn gap_text_flags_before_node(&self, node: &dyn Node) -> GapFlags { // Allow escaped newlines before commands that follow a variable assignment // since both can be long (#7955). let p = node.parent().unwrap(); + let p = p.parent().unwrap(); if p.typ() != Type::decorated_statement { return result; } diff --git a/src/builtins/shared.rs b/src/builtins/shared.rs index f80935333..400939772 100644 --- a/src/builtins/shared.rs +++ b/src/builtins/shared.rs @@ -415,12 +415,17 @@ pub fn builtin_run(parser: &Parser, argv: &mut [&wstr], streams: &mut IoStreams) return ProcStatus::from_exit_code(STATUS_CMD_OK.unwrap()); } - let Some(builtin) = builtin_lookup(argv[0]) else { - FLOGF!(error, "%s", wgettext_fmt!(UNKNOWN_BUILTIN_ERR_MSG, argv[0])); - return ProcStatus::from_exit_code(STATUS_CMD_ERROR.unwrap()); + let builtin_func = if argv[0] == L!("{") { + builtin_generic + } else { + let Some(builtin) = builtin_lookup(argv[0]) else { + FLOGF!(error, "%s", wgettext_fmt!(UNKNOWN_BUILTIN_ERR_MSG, argv[0])); + return ProcStatus::from_exit_code(STATUS_CMD_ERROR.unwrap()); + }; + builtin.func }; - let builtin_ret = (builtin.func)(parser, streams, argv); + let builtin_ret = (builtin_func)(parser, streams, argv); // Flush our out and error streams, and check for their errors. let out_ret = streams.out.flush_and_check_error(); diff --git a/src/highlight/highlight.rs b/src/highlight/highlight.rs index a44f0bcd2..8356f245b 100644 --- a/src/highlight/highlight.rs +++ b/src/highlight/highlight.rs @@ -753,7 +753,7 @@ fn io_still_ok(&self) -> bool { } // Color a command. - fn color_command(&mut self, node: &ast::String_) { + fn color_command(&mut self, node: &ast::CommandToken) { let source_range = node.source_range(); let cmd_str = self.get_source(source_range); diff --git a/src/parse_execution.rs b/src/parse_execution.rs index 988cca1f5..3b58148ee 100644 --- a/src/parse_execution.rs +++ b/src/parse_execution.rs @@ -486,7 +486,7 @@ fn expand_command( // Get the unexpanded command string. We expect to always get it here. let unexp_cmd = self.node_source(&statement.command); - let pos_of_command_token = statement.command.range().unwrap().start(); + let pos_of_command_token = statement.command.try_source_range().unwrap().start(); // Expand the string to produce completions, and report errors. let expand_err = expand_to_command_and_args( @@ -733,23 +733,29 @@ fn populate_plain_process( // Get the command and any arguments due to expanding the command. let mut cmd = WString::new(); let mut args_from_cmd_expansion = vec![]; - let ret = self.expand_command(ctx, statement, &mut cmd, &mut args_from_cmd_expansion); - if ret != EndExecutionReason::ok { - return ret; - } + let mut process_type = if statement.command.is_left_brace() { + cmd = L!("{").to_owned(); + ProcessType::builtin + } else { + let ret = self.expand_command(ctx, statement, &mut cmd, &mut args_from_cmd_expansion); + if ret != EndExecutionReason::ok { + return ret; + } - // For no-exec, having an empty command is okay. We can't do anything more with it tho. - if no_exec() { - return EndExecutionReason::ok; - } + // For no-exec, having an empty command is okay. We can't do anything more with it tho. + if no_exec() { + return EndExecutionReason::ok; + } - assert!( - !cmd.is_empty(), - "expand_command should not produce an empty command", - ); + assert!( + !cmd.is_empty(), + "expand_command should not produce an empty command", + ); + + // Determine the process type. + self.process_type_for_command(ctx, statement, &cmd) + }; - // Determine the process type. - let mut process_type = self.process_type_for_command(ctx, statement, &cmd); let external_cmd = if [ProcessType::external, ProcessType::exec].contains(&process_type) { let parser = ctx.parser(); // Determine the actual command. This may be an implicit cd. diff --git a/src/parse_util.rs b/src/parse_util.rs index 15ab8d288..deb0b7ddd 100644 --- a/src/parse_util.rs +++ b/src/parse_util.rs @@ -1679,7 +1679,7 @@ fn detect_errors_in_decorated_statement( } let unexp_command = com; - if !unexp_command.is_empty() { + if !unexp_command.is_empty() && !dst.command.is_left_brace() { // Check that we can expand the command. // Make a new error list so we can fix the offset for just those, then append later. let mut new_errors = ParseErrorList::new(); diff --git a/src/reader.rs b/src/reader.rs index 48155aa53..dd6c38e51 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -5315,7 +5315,7 @@ fn extract_tokens(s: &wstr) -> Vec { let mut cursor = Some(node); while let Some(cur) = cursor { if let Some(stmt) = cur.as_decorated_statement() { - if node.pointer_eq(&stmt.command) { + if node.pointer_eq(stmt.command.contents.embedded_node()) { return true; } } diff --git a/src/tests/parser.rs b/src/tests/parser.rs index 532d92db9..0b89806f6 100644 --- a/src/tests/parser.rs +++ b/src/tests/parser.rs @@ -496,6 +496,7 @@ macro_rules! validate { validate!("command", "command", "", StatementDecoration::none); validate!("command -", "command", "-", StatementDecoration::none); validate!("command --", "command", "--", StatementDecoration::none); + validate!("{ -h", "{", "-h", StatementDecoration::none); validate!( "builtin --names", "builtin", diff --git a/tests/checks/braces.fish b/tests/checks/braces.fish index 4b555ebc2..95e64196d 100644 --- a/tests/checks/braces.fish +++ b/tests/checks/braces.fish @@ -187,10 +187,26 @@ PATH= "{" # CHECKERR: PATH= "{" # CHECKERR: ^~^ +$fish -c '{ -h' +# CHECKERR: fish: '{': missing man page +# CHECKERR: Documentation may not be installed. +# CHECKERR: `help '{'` will show an online version + +PATH= "{" -h +# CHECKERR: fish: Unknown command: '{' +# CHECKERR: {{.*}}/braces.fish (line {{\d+}}): +# CHECKERR: PATH= "{" -h +# CHECKERR: ^~^ + $fish -c 'builtin {' # CHECKERR: fish: Expected end of the statement, but found a '{' # CHECKERR: builtin { -# CHECKERR: ^ +# CHECKERR: ^ + +$fish -c 'builtin "{"' +# CHECKERR: fish: Unknown builtin '"{"' +# CHECKERR: builtin "{" +# CHECKERR: ^~~~~~~~~~^ $fish -c 'command {' # CHECKERR: fish: Expected end of the statement, but found a '{' diff --git a/tests/checks/indent.fish b/tests/checks/indent.fish index 3fabffb0f..64e05643b 100644 --- a/tests/checks/indent.fish +++ b/tests/checks/indent.fish @@ -482,6 +482,9 @@ level 2 } } # CHECK: {{^ level 2$}} # CHECK: {{^ \}$}} # CHECK: {{^\}$}} + +echo '{ -h' +# CHECK: { -h } | $fish_indent echo 'multiline-\\