From 461c85a9445333f7a17f92d89ca760fbf27fe335 Mon Sep 17 00:00:00 2001
From: Peter Huene <peter@huene.dev>
Date: Mon, 13 Jan 2025 16:57:09 -0800
Subject: [PATCH] fix: remove trailing carriage return when stripping
 whitespace.

This fixes whitespace stripping for both commands and multiline strings that
left a trailing carriage return when using Windows line endings.

Also fixes the calculation of diagnostic spans for the preamble and version
formatting lints when Windows line endings are used.
---
 wdl-ast/CHANGELOG.md                      |  2 ++
 wdl-ast/src/v1/expr.rs                    | 31 +++++++++++++++++++++++
 wdl-ast/src/v1/task.rs                    | 27 ++++++++++++++++++++
 wdl-lint/src/rules/preamble_formatting.rs |  6 ++---
 wdl-lint/src/rules/version_formatting.rs  |  6 ++---
 5 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/wdl-ast/CHANGELOG.md b/wdl-ast/CHANGELOG.md
index d4e201942..d662fcad4 100644
--- a/wdl-ast/CHANGELOG.md
+++ b/wdl-ast/CHANGELOG.md
@@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+* Fixed a bug in `strip_whitespace` that left a trailing carriage return at the
+  end of commands and multiline strings when using Windows line endings ([#291](https://github.com/stjude-rust-labs/wdl/pull/291)).
 * Fixed bug in `strip_whitespace()` that erroneously stripped characters from the first line when it had content.
   Closed [issue #268](https://github.com/stjude-rust-labs/wdl/issues/268) ([#271](https://github.com/stjude-rust-labs/wdl/pull/271)).
 * Fixed same #268 bug in mutliline strings as well as command sections  ([#272](https://github.com/stjude-rust-labs/wdl/pull/272)).
diff --git a/wdl-ast/src/v1/expr.rs b/wdl-ast/src/v1/expr.rs
index 672a3aeb4..a737113bf 100644
--- a/wdl-ast/src/v1/expr.rs
+++ b/wdl-ast/src/v1/expr.rs
@@ -2238,6 +2238,10 @@ impl LiteralString {
             if text.ends_with('\n') {
                 text.pop();
             }
+
+            if text.ends_with('\r') {
+                text.pop();
+            }
         }
 
         // Now that the string has been unescaped and the first and last lines trimmed,
@@ -7873,4 +7877,31 @@ task test {
             _ => panic!("expected text part"),
         }
     }
+
+    #[test]
+    fn whitespace_stripping_on_windows() {
+        let (document, diagnostics) = Document::parse(
+            "version 1.2\r\ntask test {\r\n    String s = <<<\r\n        hello\r\n    >>>\r\n}\r\n",
+        );
+
+        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 decls: Vec<_> = tasks[0].declarations().collect();
+        assert_eq!(decls.len(), 1);
+
+        let expr = decls[0].expr().unwrap_literal().unwrap_string();
+        let stripped = expr.strip_whitespace().unwrap();
+        assert_eq!(stripped.len(), 1);
+        match &stripped[0] {
+            StrippedStringPart::Text(text) => {
+                assert_eq!(text.as_str(), "hello")
+            }
+            _ => panic!("expected text part"),
+        }
+    }
 }
diff --git a/wdl-ast/src/v1/task.rs b/wdl-ast/src/v1/task.rs
index cd4b79388..7b96ae5f4 100644
--- a/wdl-ast/src/v1/task.rs
+++ b/wdl-ast/src/v1/task.rs
@@ -962,6 +962,10 @@ impl CommandSection {
             if text.ends_with('\n') {
                 text.pop();
             }
+
+            if text.ends_with('\r') {
+                text.pop();
+            }
         }
 
         // Check for no indentation or all whitespace, in which case we're done
@@ -2510,4 +2514,27 @@ task test {
         };
         assert_eq!(text, "");
     }
+
+    #[test]
+    fn whitespace_stripping_on_windows() {
+        let (document, diagnostics) = Document::parse(
+            "version 1.2\r\ntask test {\r\n    command <<<\r\n        echo \"hello\"\r\n    \
+             >>>\r\n}\r\n",
+        );
+
+        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\"");
+    }
 }
diff --git a/wdl-lint/src/rules/preamble_formatting.rs b/wdl-lint/src/rules/preamble_formatting.rs
index a63f9a725..773ef4a0a 100644
--- a/wdl-lint/src/rules/preamble_formatting.rs
+++ b/wdl-lint/src/rules/preamble_formatting.rs
@@ -264,10 +264,10 @@ impl Visitor for PreambleFormattingRule {
                     if s.chars().filter(|&c| c == '\n').count() == 2 {
                         for (line, start, end) in lines_with_offset(s) {
                             if !line.is_empty() {
-                                let end_offset = if s.ends_with('\n') {
-                                    1
-                                } else if s.ends_with("\r\n") {
+                                let end_offset = if s.ends_with("\r\n") {
                                     2
+                                } else if s.ends_with('\n') {
+                                    1
                                 } else {
                                     0
                                 };
diff --git a/wdl-lint/src/rules/version_formatting.rs b/wdl-lint/src/rules/version_formatting.rs
index 27c51cecc..f5fbbcfda 100644
--- a/wdl-lint/src/rules/version_formatting.rs
+++ b/wdl-lint/src/rules/version_formatting.rs
@@ -131,10 +131,10 @@ impl Visitor for VersionFormattingRule {
                     if ws.chars().filter(|&c| c == '\n').count() == 2 {
                         for (line, start, end) in lines_with_offset(ws) {
                             if !line.is_empty() {
-                                let end_offset = if ws.ends_with('\n') {
-                                    1
-                                } else if ws.ends_with("\r\n") {
+                                let end_offset = if ws.ends_with("\r\n") {
                                     2
+                                } else if ws.ends_with('\n') {
+                                    1
                                 } else {
                                     0
                                 };