Allow if/then/fi, while/do/done and for/do/done

share/completions/do.fish

@@ -0,0 +1 @@
+complete -c do -xa '(__fish_complete_subcommand)'

share/completions/then.fish

@@ -0,0 +1 @@
+complete -c then -xa '(__fish_complete_subcommand)'

src/ast.rs

@@ -79,8 +79,10 @@ fn visit_decorated_statement_decorator(
         _node: &mut Option<DecoratedStatementDecorator>,
     );
     fn visit_job_conjunction_decorator(&mut self, _node: &mut Option<JobConjunctionDecorator>);
+    fn visit_do(&mut self, _node: &mut Option<KeywordDo>);
     fn visit_else_clause(&mut self, _node: &mut Option<ElseClause>);
     fn visit_semi_nl(&mut self, _node: &mut Option<SemiNl>);
+    fn visit_then(&mut self, _node: &mut Option<KeywordThen>);
     fn visit_time(&mut self, _node: &mut Option<KeywordTime>);
     fn visit_token_background(&mut self, _node: &mut Option<TokenBackground>);
 }
@@ -977,6 +979,12 @@ macro_rules! visit_optional_field_mut {
     (KeywordTime, $field:expr, $visitor:ident) => {
         $visitor.visit_time(&mut $field);
     };
+    (KeywordDo, $field:expr, $visitor:ident) => {
+        $visitor.visit_do(&mut $field);
+    };
+    (KeywordThen, $field:expr, $visitor:ident) => {
+        $visitor.visit_then(&mut $field);
+    };
     (TokenBackground, $field:expr, $visitor:ident) => {
         $visitor.visit_token_background(&mut $field);
     };
@@ -1315,7 +1323,11 @@ fn can_be_parsed(pop: &mut Populator<'_>) -> bool {
             || (token.typ == ParseTokenType::string
                 && !matches!(
                     token.keyword,
-                    ParseKeyword::kw_case | ParseKeyword::kw_end | ParseKeyword::kw_else
+                    ParseKeyword::kw_case
+                        | ParseKeyword::kw_done
+                        | ParseKeyword::kw_else
+                        | ParseKeyword::kw_end
+                        | ParseKeyword::kw_fi
                 ))
     }
 }
@@ -1333,6 +1345,8 @@ pub struct ForHeader {
     pub args: ArgumentList,
     /// newline or semicolon
     pub semi_nl: SemiNl,
+    // Optional "do" for compatibility with POSIX.
+    pub kw_do: Option<KeywordDo>,
 }
 implement_node!(ForHeader, branch, for_header);
 implement_acceptor_for_branch!(
@@ -1342,6 +1356,7 @@ pub struct ForHeader {
     (kw_in: (KeywordIn)),
     (args: (ArgumentList)),
     (semi_nl: (SemiNl)),
+    (kw_do: (Option<KeywordDo>)),
 );
 impl ConcreteNode for ForHeader {
     fn as_for_header(&self) -> Option<&ForHeader> {
@@ -1361,6 +1376,8 @@ pub struct WhileHeader {
     pub kw_while: KeywordWhile,
     pub condition: JobConjunction,
     pub andor_tail: AndorJobList,
+    // Optional "do" for compatibility with POSIX.
+    pub kw_do: Option<KeywordDo>,
 }
 implement_node!(WhileHeader, branch, while_header);
 implement_acceptor_for_branch!(
@@ -1368,6 +1385,7 @@ pub struct WhileHeader {
     (kw_while: (KeywordWhile)),
     (condition: (JobConjunction)),
     (andor_tail: (AndorJobList)),
+    (kw_do: (Option<KeywordDo>)),
 );
 impl ConcreteNode for WhileHeader {
     fn as_while_header(&self) -> Option<&WhileHeader> {
@@ -1441,7 +1459,7 @@ pub struct BlockStatement {
     /// List of jobs in this block.
     pub jobs: JobList,
     /// The 'end' node.
-    pub end: KeywordEnd,
+    pub end: KeywordEndOrDone,
     /// Arguments and redirections associated with the block.
     pub args_or_redirs: ArgumentOrRedirectionList,
 }
@@ -1450,7 +1468,7 @@ pub struct BlockStatement {
     BlockStatement,
     (header: (variant<BlockStatementHeaderVariant>)),
     (jobs: (JobList)),
-    (end: (KeywordEnd)),
+    (end: (KeywordEndOrDone)),
     (args_or_redirs: (ArgumentOrRedirectionList)),
 );
 impl ConcreteNode for BlockStatement {
@@ -1504,6 +1522,10 @@ pub struct IfClause {
     pub condition: JobConjunction,
     /// 'and/or' tail.
     pub andor_tail: AndorJobList,
+    // Semi
+    pub semi_nl: SemiNl,
+    // Optional "then" for compatibility with POSIX.
+    pub kw_then: Option<KeywordThen>,
     /// The body to execute if the condition is true.
     pub body: JobList,
 }
@@ -1513,6 +1535,7 @@ pub struct IfClause {
     (kw_if: (KeywordIf)),
     (condition: (JobConjunction)),
     (andor_tail: (AndorJobList)),
+    (kw_then: (Option<KeywordThen>)),
     (body: (JobList)),
 );
 impl ConcreteNode for IfClause {
@@ -1610,7 +1633,7 @@ pub struct IfStatement {
     /// else part
     pub else_clause: Option<ElseClause>,
     /// literal end
-    pub end: KeywordEnd,
+    pub end: KeywordEndOrFi,
     /// block args / redirs
     pub args_or_redirs: ArgumentOrRedirectionList,
 }
@@ -1620,7 +1643,7 @@ pub struct IfStatement {
     (if_clause: (IfClause)),
     (elseif_clauses: (ElseifClauseList)),
     (else_clause: (Option<ElseClause>)),
-    (end: (KeywordEnd)),
+    (end: (KeywordEndOrFi)),
     (args_or_redirs: (ArgumentOrRedirectionList)),
 );
 impl ConcreteNode for IfStatement {
@@ -2073,14 +2096,18 @@ fn can_be_parsed(pop: &mut Populator<'_>) -> bool {
 define_keyword_node!(JobConjunctionDecorator, kw_and, kw_or);
 define_keyword_node!(KeywordBegin, kw_begin);
 define_keyword_node!(KeywordCase, kw_case);
+define_keyword_node!(KeywordDo, kw_do);
 define_keyword_node!(KeywordElse, kw_else);
 define_keyword_node!(KeywordEnd, kw_end);
+define_keyword_node!(KeywordEndOrDone, kw_end, kw_done);
+define_keyword_node!(KeywordEndOrFi, kw_end, kw_fi);
 define_keyword_node!(KeywordFor, kw_for);
 define_keyword_node!(KeywordFunction, kw_function);
 define_keyword_node!(KeywordIf, kw_if);
 define_keyword_node!(KeywordIn, kw_in);
 define_keyword_node!(KeywordNot, kw_not, kw_exclam);
 define_keyword_node!(KeywordSwitch, kw_switch);
+define_keyword_node!(KeywordThen, kw_then);
 define_keyword_node!(KeywordTime, kw_time);
 define_keyword_node!(KeywordWhile, kw_while);
 
@@ -2125,6 +2152,26 @@ fn can_be_parsed(pop: &mut Populator<'_>) -> bool {
     }
 }
 
+impl CheckParse for KeywordDo {
+    fn can_be_parsed(pop: &mut Populator<'_>) -> bool {
+        let keyword = pop.peek_token(0).keyword;
+        if !matches!(keyword, ParseKeyword::kw_do) {
+            return false;
+        }
+        !pop.peek_token(1).is_dash_prefix_string()
+    }
+}
+
+impl CheckParse for KeywordThen {
+    fn can_be_parsed(pop: &mut Populator<'_>) -> bool {
+        let keyword = pop.peek_token(0).keyword;
+        if !matches!(keyword, ParseKeyword::kw_then) {
+            return false;
+        }
+        !pop.peek_token(1).is_dash_prefix_string()
+    }
+}
+
 impl DecoratedStatement {
     /// Return the decoration for this statement.
     pub fn decoration(&self) -> StatementDecoration {
@@ -3161,7 +3208,10 @@ fn did_visit_fields_of<'a>(&'a mut self, node: &'a dyn NodeMut, flow: VisitResul
             if next_token.typ == ParseTokenType::string
                 && matches!(
                     next_token.keyword,
-                    ParseKeyword::kw_case | ParseKeyword::kw_else | ParseKeyword::kw_end
+                    ParseKeyword::kw_case
+                        | ParseKeyword::kw_else
+                        | ParseKeyword::kw_end
+                        | ParseKeyword::kw_fi
                 )
             {
                 self.consume_excess_token_generating_error();
@@ -3245,6 +3295,12 @@ fn visit_semi_nl(&mut self, node: &mut Option<SemiNl>) {
     fn visit_time(&mut self, node: &mut Option<KeywordTime>) {
         *node = self.try_parse::<KeywordTime>().map(|b| *b);
     }
+    fn visit_do(&mut self, node: &mut Option<KeywordDo>) {
+        *node = self.try_parse::<KeywordDo>().map(|b| *b);
+    }
+    fn visit_then(&mut self, node: &mut Option<KeywordThen>) {
+        *node = self.try_parse::<KeywordThen>().map(|b| *b);
+    }
     fn visit_token_background(&mut self, node: &mut Option<TokenBackground>) {
         *node = self.try_parse::<TokenBackground>().map(|b| *b);
     }
@@ -3546,6 +3602,14 @@ fn consume_excess_token_generating_error(&mut self) {
                             "'case' builtin not inside of switch block"
                         );
                     }
+                    ParseKeyword::kw_done => {
+                        parse_error!(
+                            self,
+                            tok,
+                            ParseErrorCode::unbalancing_case,
+                            "'done' outside of a block"
+                        );
+                    }
                     ParseKeyword::kw_end => {
                         parse_error!(
                             self,
@@ -3562,6 +3626,14 @@ fn consume_excess_token_generating_error(&mut self) {
                             "'else' builtin not inside of if block"
                         );
                     }
+                    ParseKeyword::kw_fi => {
+                        parse_error!(
+                            self,
+                            tok,
+                            ParseErrorCode::unbalancing_end,
+                            "'fi' outside of a block"
+                        );
+                    }
                     _ => {
                         internal_error!(
                             self,
@@ -3819,9 +3891,11 @@ fn new_decorated_statement(slf: &mut Populator<'_>) -> StatementVariant {
             // looks like an option (starts with a dash), then parse it as a decorated statement.
             let help_only_kws = [
                 ParseKeyword::kw_begin,
+                ParseKeyword::kw_do,
                 ParseKeyword::kw_function,
                 ParseKeyword::kw_if,
                 ParseKeyword::kw_switch,
+                ParseKeyword::kw_then,
                 ParseKeyword::kw_while,
             ];
             if if help_only_kws.contains(&self.peek_token(0).keyword) {
@@ -3834,8 +3908,13 @@ fn new_decorated_statement(slf: &mut Populator<'_>) -> StatementVariant {
 
             // Likewise if the next token doesn't look like an argument at all. This corresponds to
             // e.g. a "naked if".
-            let naked_invocation_invokes_help = ![ParseKeyword::kw_begin, ParseKeyword::kw_end]
+            let naked_invocation_invokes_help = ![
-                .contains(&self.peek_token(0).keyword);
+                ParseKeyword::kw_begin,
+                ParseKeyword::kw_done,
+                ParseKeyword::kw_end,
+                ParseKeyword::kw_fi,
+            ]
+            .contains(&self.peek_token(0).keyword);
             if naked_invocation_invokes_help
                 && [ParseTokenType::end, ParseTokenType::terminate]
                     .contains(&self.peek_token(1).typ)
@@ -3864,7 +3943,7 @@ fn new_decorated_statement(slf: &mut Populator<'_>) -> StatementVariant {
                 let embedded = self.allocate_visit::<SwitchStatement>();
                 StatementVariant::SwitchStatement(embedded)
             }
-            ParseKeyword::kw_end => {
+            ParseKeyword::kw_done | ParseKeyword::kw_end | ParseKeyword::kw_fi => {
                 // 'end' is forbidden as a command.
                 // For example, `if end` or `while end` will produce this error.
                 // We still have to descend into the decorated statement because
@@ -4036,7 +4115,7 @@ fn visit_keyword(&mut self, keyword: &mut dyn Keyword) -> VisitResult {
 
             // Special error reporting for keyword_t<kw_end>.
             let allowed_keywords = keyword.allowed_keywords();
-            if keyword.allowed_keywords() == [ParseKeyword::kw_end] {
+            if allowed_keywords.contains(&ParseKeyword::kw_end) {
                 return VisitResult::Break(MissingEndError {
                     allowed_keywords,
                     token: *self.peek_token(0),

src/builtins/shared.rs

@@ -209,6 +209,14 @@ struct BuiltinData {
         name: L!("disown"),
         func: disown::disown,
     },
+    BuiltinData {
+        name: L!("do"),
+        func: builtin_generic,
+    },
+    BuiltinData {
+        name: L!("done"),
+        func: builtin_generic,
+    },
     BuiltinData {
         name: L!("echo"),
         func: echo::echo,
@@ -245,6 +253,10 @@ struct BuiltinData {
         name: L!("fg"),
         func: fg::fg,
     },
+    BuiltinData {
+        name: L!("fi"),
+        func: builtin_generic,
+    },
     BuiltinData {
         name: L!("fish_indent"),
         func: fish_indent::fish_indent,
@@ -345,6 +357,10 @@ struct BuiltinData {
         name: L!("test"),
         func: test::test,
     },
+    BuiltinData {
+        name: L!("then"),
+        func: builtin_generic,
+    },
     BuiltinData {
         name: L!("time"),
         func: builtin_generic,
@@ -504,6 +520,8 @@ pub fn builtin_get_desc(name: &wstr) -> Option<&'static wstr> {
         _ if name == "continue" => wgettext!("Skip over remaining innermost loop"),
         _ if name == "count" => wgettext!("Count the number of arguments"),
         _ if name == "disown" => wgettext!("Remove job from job list"),
+        _ if name == "do" => wgettext!("Optional after a loop header"),
+        _ if name == "done" => wgettext!("Alternative to 'end', to end a block of commands"),
         _ if name == "echo" => wgettext!("Print arguments"),
         _ if name == "else" => wgettext!("Evaluate block if condition is false"),
         _ if name == "emit" => wgettext!("Emit an event"),
@@ -513,6 +531,7 @@ pub fn builtin_get_desc(name: &wstr) -> Option<&'static wstr> {
         _ if name == "exit" => wgettext!("Exit the shell"),
         _ if name == "false" => wgettext!("Return an unsuccessful result"),
         _ if name == "fg" => wgettext!("Send job to foreground"),
+        _ if name == "fi" => wgettext!("Alternative to 'end', to end a block of commands"),
         _ if name == "fish_key_reader" => wgettext!("explore what characters keyboard keys send"),
         _ if name == "for" => wgettext!("Perform a set of commands multiple times"),
         _ if name == "function" => wgettext!("Define a new function"),
@@ -531,6 +550,7 @@ pub fn builtin_get_desc(name: &wstr) -> Option<&'static wstr> {
         _ if name == "realpath" => wgettext!("Show absolute path sans symlinks"),
         _ if name == "return" => wgettext!("Stop the currently evaluated function"),
         _ if name == "set" => wgettext!("Handle environment variables"),
+        _ if name == "then" => wgettext!("Optional after the condition of an if-statement"),
         _ if name == "set_color" => wgettext!("Set the terminal color"),
         _ if name == "source" => wgettext!("Evaluate contents of file"),
         _ if name == "status" => wgettext!("Return status information about fish"),

src/highlight/highlight.rs

@@ -846,14 +846,18 @@ fn visit_keyword(&mut self, node: &dyn Keyword) {
             | ParseKeyword::kw_builtin
             | ParseKeyword::kw_case
             | ParseKeyword::kw_command
+            | ParseKeyword::kw_do
+            | ParseKeyword::kw_done
             | ParseKeyword::kw_else
             | ParseKeyword::kw_end
             | ParseKeyword::kw_exec
+            | ParseKeyword::kw_fi
             | ParseKeyword::kw_for
             | ParseKeyword::kw_function
             | ParseKeyword::kw_if
             | ParseKeyword::kw_in
             | ParseKeyword::kw_switch
+            | ParseKeyword::kw_then
             | ParseKeyword::kw_while => role = HighlightRole::keyword,
             ParseKeyword::kw_and
             | ParseKeyword::kw_or

src/parse_constants.rs

@@ -92,10 +92,13 @@ pub enum ParseKeyword {
     kw_builtin,
     kw_case,
     kw_command,
+    kw_do,
+    kw_done,
     kw_else,
     kw_end,
     kw_exclam,
     kw_exec,
+    kw_fi,
     kw_for,
     kw_function,
     kw_if,
@@ -103,6 +106,7 @@ pub enum ParseKeyword {
     kw_not,
     kw_or,
     kw_switch,
+    kw_then,
     kw_time,
     kw_while,
 }
@@ -237,10 +241,13 @@ pub fn to_wstr(self) -> &'static wstr {
             ParseKeyword::kw_builtin => L!("builtin"),
             ParseKeyword::kw_case => L!("case"),
             ParseKeyword::kw_command => L!("command"),
+            ParseKeyword::kw_do => L!("do"),
+            ParseKeyword::kw_done => L!("done"),
             ParseKeyword::kw_else => L!("else"),
             ParseKeyword::kw_end => L!("end"),
             ParseKeyword::kw_exclam => L!("!"),
             ParseKeyword::kw_exec => L!("exec"),
+            ParseKeyword::kw_fi => L!("fi"),
             ParseKeyword::kw_for => L!("for"),
             ParseKeyword::kw_function => L!("function"),
             ParseKeyword::kw_if => L!("if"),
@@ -248,6 +255,7 @@ pub fn to_wstr(self) -> &'static wstr {
             ParseKeyword::kw_not => L!("not"),
             ParseKeyword::kw_or => L!("or"),
             ParseKeyword::kw_switch => L!("switch"),
+            ParseKeyword::kw_then => L!("then"),
             ParseKeyword::kw_time => L!("time"),
             ParseKeyword::kw_while => L!("while"),
             _ => L!("unknown_keyword"),
@@ -270,10 +278,13 @@ fn from(s: &wstr) -> Self {
             _ if s == "builtin" => ParseKeyword::kw_builtin,
             _ if s == "case" => ParseKeyword::kw_case,
             _ if s == "command" => ParseKeyword::kw_command,
+            _ if s == "do" => ParseKeyword::kw_do,
+            _ if s == "done" => ParseKeyword::kw_done,
             _ if s == "else" => ParseKeyword::kw_else,
             _ if s == "end" => ParseKeyword::kw_end,
             _ if s == "exec" => ParseKeyword::kw_exec,
             _ if s == "for" => ParseKeyword::kw_for,
+            _ if s == "fi" => ParseKeyword::kw_fi,
             _ if s == "function" => ParseKeyword::kw_function,
             _ if s == "if" => ParseKeyword::kw_if,
             _ if s == "in" => ParseKeyword::kw_in,
@@ -281,6 +292,7 @@ fn from(s: &wstr) -> Self {
             _ if s == "or" => ParseKeyword::kw_or,
             _ if s == "switch" => ParseKeyword::kw_switch,
             _ if s == "time" => ParseKeyword::kw_time,
+            _ if s == "then" => ParseKeyword::kw_then,
             _ if s == "while" => ParseKeyword::kw_while,
             _ => ParseKeyword::none,
         }

src/parse_tree.rs

@@ -15,7 +15,7 @@
 use crate::wcstringutil::count_newlines;
 
 /// A struct representing the token type that we use internally.
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug)]
 pub struct ParseToken {
     /// The type of the token as represented by the parser
     pub typ: ParseTokenType,

src/parser_keywords.rs

@@ -50,10 +50,13 @@ macro_rules! reserved_words {
     ("case"),
     ("command", [subcommand]),
     ("continue"),
+    ("do", [subcommand]),
+    ("done"),
     ("else", [subcommand]),
     ("end"),
     ("eval"),
     ("exec", [subcommand]),
+    ("fi"),
     ("for"),
     ("function"),
     ("if", [subcommand]),
@@ -65,6 +68,7 @@ macro_rules! reserved_words {
     ("status"),
     ("string"),
     ("switch"),
+    ("then"),
     ("test"),
     ("time", [subcommand]),
     ("while", [subcommand]),

tests/checks/indent.fish

@@ -614,3 +614,12 @@ echo 'echo "foo" "bar"' > $tmpdir/indent_test.fish
 $fish_indent --write $tmpdir/indent_test.fish
 cat $tmpdir/indent_test.fish
 # CHECK: echo foo bar
+
+# TODO: We should not force the line break.
+echo 'if true; then
+    echo body
+fi' | $fish_indent
+# CHECK: if true
+# CHECK: then
+# CHECK: {{    }}echo body
+# CHECK: fi