fix: formatting of command sections (#240)

Co-authored-by: Andrew Thrasher <1165729+adthrasher@users.noreply.github.com>
Co-authored-by: Andrew Thrasher <adthrasher@gmail.com>

wdl-ast/src/v1/task.rs

@@ -793,6 +793,7 @@ impl CommandSection {
                                 leading_spaces = 0;
                                 leading_tabs = 0;
                             }
+                            '\r' => {}
                             _ => {
                                 if parsing_leading_whitespace {
                                     parsing_leading_whitespace = false;
@@ -913,6 +914,15 @@ impl CommandSection {
             if text.ends_with('\n') {
                 text.pop();
             }
+            if text.lines().last().map_or(false, |l| l.trim().is_empty()) {
+                while let Some(last) = text.lines().last() {
+                    if last.trim().is_empty() {
+                        text.pop();
+                    } else {
+                        break;
+                    }
+                }
+            }
         }
 
         Some(result)
@@ -2303,4 +2313,40 @@ task test {
         let stripped = command.strip_whitespace();
         assert!(stripped.is_none());
     }
+
+    #[test]
+    fn whitespace_stripping_with_funky_indentation() {
+        let (document, diagnostics) = Document::parse(
+            r#"
+version 1.2
+
+task test {
+    command <<<
+    echo "hello"
+        echo "world"
+    echo \
+            "goodbye"
+                >>>
+        }"#,
+        );
+
+        assert!(diagnostics.is_empty());
+        let ast = document.ast();
+        let ast = ast.as_v1().expect("should be a V1 AST");
+        let tasks: Vec<_> = ast.tasks().collect();
+        assert_eq!(tasks.len(), 1);
+
+        let command = tasks[0].command().expect("should have a command section");
+
+        let stripped = command.strip_whitespace().unwrap();
+        assert_eq!(stripped.len(), 1);
+        let text = match &stripped[0] {
+            StrippedCommandPart::Text(text) => text,
+            _ => panic!("expected text"),
+        };
+        assert_eq!(
+            text,
+            "echo \"hello\"\n    echo \"world\"\necho \\\n        \"goodbye\"\n"
+        );
+    }
 }

wdl-format/CHANGELOG.md

@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Added
+
+* Leading whitespace in command text is now normalized ([#240](https://github.com/stjude-rust-labs/wdl/pull/240)).
+
+### Fixed
+
+* Multi-line placeholders in command blocks are now indented appropriately ([#240](https://github.com/stjude-rust-labs/wdl/pull/240)).
+
 ## 0.3.0 - 10-22-2024
 
 ### Fixed

wdl-format/src/token/post.rs

@@ -109,6 +109,12 @@ pub struct Postprocessor {
 
     /// Whether blank lines are allowed in the current context.
     line_spacing_policy: LineSpacingPolicy,
+
+    /// Whether temporary indentation is needed.
+    temp_indent_needed: bool,
+
+    /// Temporary indentation to add while formatting command blocks.
+    temp_indent: String,
 }
 
 impl Postprocessor {
@@ -166,6 +172,20 @@ impl Postprocessor {
             }
             PreToken::Literal(value, kind) => {
                 assert!(kind != SyntaxKind::Comment && kind != SyntaxKind::Whitespace);
+
+                // This is special handling for inserting the empty string.
+                // We remove any indentation or spaces from the end of the
+                // stream and then add the empty string as a literal.
+                // Then we set the position to [`LinePosition::MiddleOfLine`]
+                // in order to trigger a newline being added before the next
+                // token.
+                if value.is_empty() {
+                    self.trim_last_line(stream);
+                    stream.push(PostToken::Literal(value));
+                    self.position = LinePosition::MiddleOfLine;
+                    return;
+                }
+
                 if self.interrupted
                     && matches!(
                         kind,
@@ -178,6 +198,14 @@ impl Postprocessor {
                 {
                     stream.0.pop();
                 }
+
+                if kind == SyntaxKind::LiteralCommandText {
+                    self.temp_indent = value
+                        .chars()
+                        .take_while(|c| matches!(c, ' ' | '\t'))
+                        .collect();
+                }
+
                 stream.push(PostToken::Literal(value));
                 self.position = LinePosition::MiddleOfLine;
             }
@@ -221,6 +249,12 @@ impl Postprocessor {
                     self.end_line(stream);
                 }
             },
+            PreToken::TempIndentStart => {
+                self.temp_indent_needed = true;
+            }
+            PreToken::TempIndentEnd => {
+                self.temp_indent_needed = false;
+            }
         }
     }
 
@@ -236,7 +270,10 @@ impl Postprocessor {
 
     /// Trims spaces and indents (and not newlines) from the end of the stream.
     fn trim_last_line(&mut self, stream: &mut TokenStream<PostToken>) {
-        stream.trim_while(|token| matches!(token, PostToken::Space | PostToken::Indent));
+        stream.trim_while(|token| {
+            matches!(token, PostToken::Space | PostToken::Indent)
+                || token == &PostToken::Literal(self.temp_indent.clone())
+        });
     }
 
     /// Ends the current line without resetting the interrupted flag.
@@ -268,6 +305,10 @@ impl Postprocessor {
         for _ in 0..level {
             stream.push(PostToken::Indent);
         }
+
+        if self.temp_indent_needed {
+            stream.push(PostToken::Literal(self.temp_indent.clone()));
+        }
     }
 
     /// Creates a blank line and then indents.

wdl-format/src/token/pre.rs

@@ -42,6 +42,12 @@ pub enum PreToken {
 
     /// Trivia.
     Trivia(Trivia),
+
+    /// A temporary indent start. Used in command section formatting.
+    TempIndentStart,
+
+    /// A temporary indent end. Used in command section formatting.
+    TempIndentEnd,
 }
 
 /// The line length to use when displaying pretokens.
@@ -90,6 +96,8 @@ impl std::fmt::Display for PreToken {
                     }
                 },
             },
+            PreToken::TempIndentStart => write!(f, "<TempIndentStart>"),
+            PreToken::TempIndentEnd => write!(f, "<TempIndentEnd>"),
         }
     }
 }

wdl-format/src/v1/task.rs

@@ -1,6 +1,7 @@
 //! Formatting for tasks.
 
 use wdl_ast::SyntaxKind;
+use wdl_ast::v1::StrippedCommandPart;
 
 use crate::PreToken;
 use crate::TokenStream;
@@ -98,11 +99,14 @@ pub fn format_task_definition(element: &FormatElement, stream: &mut TokenStream<
     }
 
     stream.blank_lines_allowed();
+    let body_empty = body.is_empty();
     for child in body {
         (&child).write(stream);
     }
     stream.blank_lines_allowed_between_comments();
-    stream.blank_line();
+    if !body_empty {
+        stream.blank_line();
+    }
 
     if let Some(command) = command {
         (&command).write(stream);
@@ -164,32 +168,96 @@ pub fn format_command_section(element: &FormatElement, stream: &mut TokenStream<
             );
         }
     }
-    // Technically there's no trivia inside the command section,
-    // so we don't want to increment indent here.
-    // All the indentation should be handled by the command text itself.
-    // TODO: multi-line placeholders need better formatting
-    for child in children {
-        match child.element().kind() {
-            SyntaxKind::CloseBrace => {
-                stream.push_literal_in_place_of_token(
-                    child
-                        .element()
-                        .as_token()
-                        .expect("close brace should be token"),
-                    ">>>".to_string(),
-                );
-            }
-            SyntaxKind::CloseHeredoc => {
-                (&child).write(stream);
+
+    let parts = element
+        .element()
+        .as_node()
+        .expect("command section node")
+        .as_command_section()
+        .expect("command section")
+        .strip_whitespace();
+    match parts {
+        None => {
+            // The command section has mixed indentation, so we format it as is.
+            // TODO: We may want to format this differently in the future, but for now
+            // we can say "ugly input, ugly output".
+            for child in children {
+                match child.element().kind() {
+                    SyntaxKind::CloseBrace => {
+                        stream.push_literal_in_place_of_token(
+                            child
+                                .element()
+                                .as_token()
+                                .expect("close brace should be token"),
+                            ">>>".to_string(),
+                        );
+                    }
+                    SyntaxKind::CloseHeredoc => {
+                        (&child).write(stream);
+                    }
+                    SyntaxKind::LiteralCommandText | SyntaxKind::PlaceholderNode => {
+                        (&child).write(stream);
+                    }
+                    _ => {
+                        unreachable!(
+                            "unexpected child in command section: {:?}",
+                            child.element().kind()
+                        );
+                    }
+                }
             }
-            SyntaxKind::LiteralCommandText | SyntaxKind::PlaceholderNode => {
-                (&child).write(stream);
+        }
+        Some(parts) => {
+            // Now we parse the stripped command section and format it.
+            // End the line after the open delimiter and increment indent.
+            stream.increment_indent();
+
+            for (part, child) in parts.iter().zip(children.by_ref()) {
+                match part {
+                    StrippedCommandPart::Text(text) => {
+                        // Manually format the text and ignore the child.
+                        for (i, line) in text.lines().enumerate() {
+                            if i > 0 {
+                                stream.end_line();
+                            }
+                            stream.push_literal(line.to_owned(), SyntaxKind::LiteralCommandText);
+                        }
+
+                        if text.ends_with('\n') {
+                            stream.end_line();
+                        }
+                    }
+                    StrippedCommandPart::Placeholder(_) => {
+                        stream.push(PreToken::TempIndentStart);
+                        (&child).write(stream);
+                        stream.push(PreToken::TempIndentEnd);
+                    }
+                }
             }
-            _ => {
-                unreachable!(
-                    "unexpected child in command section: {:?}",
-                    child.element().kind()
-                );
+
+            stream.decrement_indent();
+
+            for child in children {
+                match child.element().kind() {
+                    SyntaxKind::CloseBrace => {
+                        stream.push_literal_in_place_of_token(
+                            child
+                                .element()
+                                .as_token()
+                                .expect("close brace should be token"),
+                            ">>>".to_string(),
+                        );
+                    }
+                    SyntaxKind::CloseHeredoc => {
+                        (&child).write(stream);
+                    }
+                    _ => {
+                        unreachable!(
+                            "unexpected child in command section: {:?}",
+                            child.element().kind()
+                        );
+                    }
+                }
             }
         }
     }