From da1caf363dc25b7b7617bcce4cf449003db46b1f Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Mon, 6 Jan 2025 17:14:17 -0500
Subject: [PATCH 01/15] feat: improved shellcheck fix messages

---
 wdl-lint/Cargo.toml                           |   1 +
 wdl-lint/src/fix.rs                           | 247 ++++++++++++++++++
 wdl-lint/src/lib.rs                           |   1 +
 wdl-lint/src/rules/shellcheck.rs              | 212 +++++++++++++--
 .../tests/lints/shellcheck-warn/source.errors |  62 ++---
 5 files changed, 475 insertions(+), 48 deletions(-)
 create mode 100644 wdl-lint/src/fix.rs

diff --git a/wdl-lint/Cargo.toml b/wdl-lint/Cargo.toml
index cc1d1211..155ee8df 100644
--- a/wdl-lint/Cargo.toml
+++ b/wdl-lint/Cargo.toml
@@ -15,6 +15,7 @@ readme = "../README.md"
 wdl-ast = { path = "../wdl-ast", version = "0.9.0" }
 anyhow = { workspace = true }
 convert_case = { workspace = true }
+ftree = "1.2.0"
 indexmap = { workspace = true }
 rand = { workspace = true }
 rowan = { workspace = true }
diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
new file mode 100644
index 00000000..ae92f213
--- /dev/null
+++ b/wdl-lint/src/fix.rs
@@ -0,0 +1,247 @@
+//! Module for applying fixes for diagnostics.
+
+use std::ops::Range;
+use std::ops::RangeInclusive;
+
+use ftree::FenwickTree;
+use serde::Deserialize;
+
+#[derive(Copy, Clone, Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+#[non_exhaustive]
+/// An insertion point.
+pub enum InsertionPoint {
+    /// Insert immediately before a specified region.
+    BeforeStart,
+    /// Insert immediately after a specified region.
+    AfterEnd,
+}
+
+#[derive(Clone, Debug)]
+/// A replacement to be applied to a String.
+pub struct Replacement {
+    /// The start position of the replacement.
+    start: usize,
+    /// The end position of the replacement.
+    end: usize,
+    /// where to insert the replacement.
+    insertion_point: InsertionPoint,
+    /// Value to be inserted.
+    replacement: String,
+    /// Precedence for replacement. Higher precedences are applied first.
+    precedence: usize,
+}
+
+#[allow(unused)]
+impl Replacement {
+    /// Create a new `Replacement`.
+    pub fn new(
+        start: usize,
+        end: usize,
+        insertion_point: InsertionPoint,
+        replacement: String,
+        precedence: usize,
+    ) -> Self {
+        Replacement {
+            start,
+            end,
+            insertion_point,
+            replacement,
+            precedence,
+        }
+    }
+
+    /// The start position of the replacement.
+    pub fn start(&self) -> usize {
+        self.start
+    }
+
+    /// The end position of the replacement.
+    pub fn end(&self) -> usize {
+        self.end
+    }
+
+    /// where to insert the replacement.
+    pub fn insertion_point(&self) -> InsertionPoint {
+        self.insertion_point
+    }
+
+    /// Value to be inserted.
+    pub fn replacement(&self) -> &str {
+        self.replacement.as_ref()
+    }
+
+    /// Precedence for replacement. Higher precedences are applied first.
+    pub fn precedence(&self) -> usize {
+        self.precedence
+    }
+}
+
+// Adapted from ShellCheck's [Fixer](https://github.com/koalaman/shellcheck/blob/master/src/ShellCheck/Fixer.hs)
+/// Apply a series of `Replacement`s to a String.
+///
+/// Internally uses a [Fenwick Tree](https://en.wikipedia.org/wiki/Fenwick_tree)
+/// which is updated as replacements are applied. This allows multiple
+/// replacements referencing only the original input. Although the canonical
+/// Fenwick tree is 1-indexed this implementation is 0-indexed, so replacement
+/// positions must be directly equivalent to string indices.
+#[derive(Clone, Debug)]
+pub struct Fixer {
+    /// The string to be modified.
+    value: String,
+    /// A Fenwick tree for tracking modifications.
+    tree: FenwickTree<i32>,
+}
+
+#[allow(unused)]
+impl Fixer {
+    /// Create a new Fixer from a String.
+    pub fn new(value: String) -> Self {
+        Fixer {
+            tree: FenwickTree::from_iter(vec![0; value.len()]),
+            value,
+        }
+    }
+
+    /// Apply a `Replacement` to the value contained in the Fixer.
+    pub fn apply_replacement(&mut self, rep: &Replacement) {
+        let old_start = rep.start;
+        let old_end = rep.end;
+        let new_start = self.transform(old_start);
+        let new_end = self.transform(old_end);
+
+        let rep_len =
+            i32::try_from(rep.replacement().len()).expect("replacement length fits into i32");
+        let range = i32::try_from(old_end - old_start).expect("range fits into i32");
+        let shift = rep_len - range;
+        let insert_at = match rep.insertion_point {
+            InsertionPoint::BeforeStart => old_start,
+            InsertionPoint::AfterEnd => old_end + 1,
+        };
+        self.tree.add_at(insert_at, shift);
+        self.value
+            .replace_range(new_start..new_end, &rep.replacement);
+    }
+
+    /// Apply multiple `Replacement`s in the correct order.
+    ///
+    /// Order is determined by the precedence field.
+    /// Higher precedences are applied first.
+    pub fn apply_replacements(&mut self, mut reps: Vec<Replacement>) {
+        reps.sort_by_key(|r| r.precedence);
+        reps.iter().rev().for_each(|r| self.apply_replacement(r));
+    }
+
+    /// Returns a reference to the value of the fixer with any applied
+    /// replacements.
+    pub fn value(&self) -> &str {
+        &self.value
+    }
+
+    /// Given a `Range`, update the bounds to account for any applied
+    /// replacements.
+    ///
+    /// Panics if the input does not fall within the bounds of the Fixer's
+    /// value or if the adjusted index does not fit within usize.
+    pub fn adj_range(&self, range: Range<usize>) -> Range<usize> {
+        self.transform(range.start)..self.transform(range.end)
+    }
+
+    /// Given a RangeInclusive, update the bounds to account for any applied
+    /// replacements.
+    ///
+    /// Panics if the input does not fall within the bounds of the Fixer's
+    /// value or if the adjusted index does not fit within usize.
+    pub fn adj_range_inc(&self, range: RangeInclusive<usize>) -> RangeInclusive<usize> {
+        self.transform(*range.start())..=self.transform(*range.end())
+    }
+
+    /// Returns a reference to the internal Fenwick tree.
+    pub fn tree(&self) -> &FenwickTree<i32> {
+        &self.tree
+    }
+
+    /// Get the updated index for a position.
+    ///
+    /// Returns the prefix sum of the index + index.
+    ///
+    /// Panics if the input index does not fit within i32 or
+    /// if the updated index does not fit within usize.
+    pub fn transform(&self, index: usize) -> usize {
+        usize::try_from(
+            i32::try_from(index).expect("index fits into i32") + self.tree.prefix_sum(index, 0i32),
+        )
+        .expect("updated index fits into usize")
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use pretty_assertions::assert_eq;
+
+    use crate::fix::Fixer;
+    use crate::fix::InsertionPoint;
+    use crate::fix::Replacement;
+
+    #[test]
+    fn test_fixer_insertion() {
+        let value = String::from("hello");
+        let insertion = String::from("world");
+        let rep = Replacement::new(
+            value.len(),
+            value.len(),
+            InsertionPoint::AfterEnd,
+            insertion,
+            2,
+        );
+        let rep2 = Replacement::new(5, 5, InsertionPoint::BeforeStart, String::from(" "), 1);
+
+        let mut fixer = Fixer::new(value);
+        let mut fixer2 = fixer.clone();
+
+        fixer.apply_replacement(&rep);
+        fixer.apply_replacement(&rep2);
+        assert_eq!(fixer.value(), "hello world");
+
+        fixer2.apply_replacements(vec![rep, rep2]);
+        assert_eq!(fixer2.value(), "hello world");
+    }
+
+    #[test]
+    fn test_fixer_deletion() {
+        let value = String::from("My grammar is perfect.");
+        let del = String::from("");
+        let del2 = String::from("bad");
+        let rep = Replacement::new(11, 14, InsertionPoint::BeforeStart, del, 2);
+        let rep2 = Replacement::new(14, 21, InsertionPoint::AfterEnd, del2, 1);
+
+        let mut fixer = Fixer::new(value);
+        let mut fixer2 = fixer.clone();
+
+        fixer.apply_replacement(&rep);
+        fixer.apply_replacement(&rep2);
+        assert_eq!(fixer.value(), "My grammar bad.");
+
+        fixer2.apply_replacements(vec![rep2, rep]);
+        assert_eq!(fixer2.value(), "My grammar bad.");
+    }
+
+    #[test]
+    fn test_fixer_indel() {
+        let value = String::from("This statement is false.");
+        let del = String::from("");
+        let ins = String::from("true");
+        let rep = Replacement::new(18, 23, InsertionPoint::BeforeStart, del, 2);
+        let rep2 = Replacement::new(18, 18, InsertionPoint::AfterEnd, ins, 1);
+
+        let mut fixer = Fixer::new(value);
+        let mut fixer2 = fixer.clone();
+
+        fixer.apply_replacement(&rep);
+        fixer.apply_replacement(&rep2);
+        assert_eq!(fixer.value(), "This statement is true.");
+
+        fixer2.apply_replacements(vec![rep2, rep]);
+        assert_eq!(fixer2.value(), "This statement is true.");
+    }
+}
diff --git a/wdl-lint/src/lib.rs b/wdl-lint/src/lib.rs
index cffdc5bc..8655b88a 100644
--- a/wdl-lint/src/lib.rs
+++ b/wdl-lint/src/lib.rs
@@ -33,6 +33,7 @@ use wdl_ast::Diagnostics;
 use wdl_ast::SyntaxKind;
 use wdl_ast::Visitor;
 
+pub(crate) mod fix;
 pub mod rules;
 mod tags;
 pub(crate) mod util;
diff --git a/wdl-lint/src/rules/shellcheck.rs b/wdl-lint/src/rules/shellcheck.rs
index 062166d8..ebf4e240 100644
--- a/wdl-lint/src/rules/shellcheck.rs
+++ b/wdl-lint/src/rules/shellcheck.rs
@@ -9,6 +9,7 @@ use std::sync::OnceLock;
 use anyhow::Context;
 use anyhow::Result;
 use anyhow::bail;
+use ftree::FenwickTree;
 use rand::distributions::Alphanumeric;
 use rand::distributions::DistString;
 use serde::Deserialize;
@@ -36,6 +37,9 @@ use wdl_ast::v1::TaskDefinition;
 use crate::Rule;
 use crate::Tag;
 use crate::TagSet;
+use crate::fix::Fixer;
+use crate::fix::InsertionPoint;
+use crate::fix::Replacement;
 use crate::util::count_leading_whitespace;
 use crate::util::is_properly_quoted;
 use crate::util::lines_with_offset;
@@ -64,27 +68,87 @@ static SHELLCHECK_EXISTS: OnceLock<bool> = OnceLock::new();
 /// The identifier for the command section ShellCheck rule.
 const ID: &str = "ShellCheck";
 
+#[derive(Clone, Debug, Deserialize)]
+/// Suggested fix for a ShellCheck diagnostic.
+struct ShellCheckFix {
+    /// The replacements to perform.
+    pub replacements: Vec<ShellCheckReplacement>,
+}
+
+#[derive(Clone, Debug, Deserialize)]
+/// A ShellCheck replacement.
+///
+/// This differs from a `Replacement` in that
+/// 1) columns are 1-indexed
+/// 2) it may span multiple lines and thus cannot be directly passed to a
+///    `Fixer`.
+///
+/// It must be normalized with `normalize_replacements` before use.
+struct ShellCheckReplacement {
+    /// Line number replacement occurs on.
+    pub line: usize,
+    /// Line number replacement ends on.
+    #[serde(rename = "endLine")]
+    pub end_line: usize,
+    /// Order in which replacements should happen. Highest precedence first.
+    pub precedence: usize,
+    /// An `InsertionPoint`.
+    #[serde(rename = "insertionPoint")]
+    pub insertion_point: InsertionPoint,
+    /// Column replacement occurs on.
+    pub column: usize,
+    /// Column replacements ends on.
+    #[serde(rename = "endColumn")]
+    pub end_column: usize,
+    /// Replacement text.
+    pub replacement: String,
+}
+
 /// A ShellCheck diagnostic.
 ///
-/// The `file` and `fix` fields are ommitted as we have no use for them.
+/// The `file` field is ommitted as we have no use for it.
 #[derive(Clone, Debug, Deserialize)]
 struct ShellCheckDiagnostic {
-    /// line number comment starts on
+    /// Line number comment starts on.
     pub line: usize,
-    /// line number comment ends on
+    /// Line number comment ends on.
     #[serde(rename = "endLine")]
     pub end_line: usize,
-    /// column comment starts on
+    /// Column comment starts on.
     pub column: usize,
-    /// column comment ends on
+    /// Column comment ends on.
     #[serde(rename = "endColumn")]
     pub end_column: usize,
-    /// severity of the comment
+    /// Severity of the comment.
     pub level: String,
-    /// shellcheck error code
+    /// ShellCheck error code.
     pub code: usize,
-    /// message associated with the comment
+    /// Message associated with the comment.
     pub message: String,
+    /// Optional fixes to apply.
+    pub fix: Option<ShellCheckFix>,
+}
+
+/// Convert `ShellCheckReplacement`s into `Replacement`s.
+///
+/// Column indices are shifted to 0-based.
+/// Multi-line replacements are normalized so that column indices are
+/// as though the string is on a single line.
+fn normalize_replacements(
+    reps: &[ShellCheckReplacement],
+    shift_tree: &FenwickTree<usize>,
+) -> Vec<Replacement> {
+    reps.iter()
+        .map(|r| {
+            Replacement::new(
+                r.column + shift_tree.prefix_sum(r.line - 1, 0) - 1,
+                r.end_column + shift_tree.prefix_sum(r.end_line - 1, 0) - 1,
+                r.insertion_point,
+                r.replacement.clone(),
+                r.precedence,
+            )
+        })
+        .collect()
 }
 
 /// Run shellcheck on a command.
@@ -195,12 +259,49 @@ fn gather_task_declarations(task: &TaskDefinition) -> HashSet<String> {
     decls
 }
 
+/// Create an appropriate 'fix' message.
+///
+/// Returns only the slice of the command that falls between
+/// the first and last replacements. If there is only one replacement,
+/// this is the slice between the replacement's start and end.
+fn create_fix_message(reps: Vec<Replacement>, command_text: &str) -> String {
+    let mut fixer = Fixer::new(command_text.to_owned());
+    // Get the original left-most and right-most replacement indices.
+    let start = reps
+        .iter()
+        .map(|r| r.start())
+        .min()
+        .expect("replacements is non-empty");
+    let end = reps
+        .iter()
+        .map(|r| r.end())
+        .max()
+        .expect("replacements is non-empty");
+    fixer.apply_replacements(reps);
+    // Adjust start and end based on final tree.
+    let adj_range = fixer.adj_range_inc(start..=end);
+    format!("did you mean `{}`?", &fixer.value()[adj_range])
+}
+
 /// Creates a "ShellCheck lint" diagnostic from a `ShellCheckDiagnostic`
-fn shellcheck_lint(diagnostic: &ShellCheckDiagnostic, span: Span) -> Diagnostic {
+fn shellcheck_lint(
+    diagnostic: &ShellCheckDiagnostic,
+    command_text: &str,
+    line_map: &HashMap<usize, Span>,
+    shift_tree: &FenwickTree<usize>,
+) -> Diagnostic {
     let label = format!(
         "SC{}[{}]: {}",
         diagnostic.code, diagnostic.level, diagnostic.message
     );
+    let span = calculate_span(diagnostic, line_map);
+    let fix_msg = match diagnostic.fix {
+        Some(ref fix) => {
+            let reps = normalize_replacements(&fix.replacements, shift_tree);
+            create_fix_message(reps, command_text)
+        }
+        None => String::from("address the diagnostic as recommended in the message"),
+    };
     Diagnostic::note(&diagnostic.message)
         .with_rule(ID)
         .with_label(label, span)
@@ -208,7 +309,7 @@ fn shellcheck_lint(diagnostic: &ShellCheckDiagnostic, span: Span) -> Diagnostic
             format!("more info: {}/SC{}", &SHELLCHECK_WIKI, diagnostic.code),
             span,
         )
-        .with_fix("address the diagnostic as recommended in the message")
+        .with_fix(fix_msg)
 }
 
 /// Sanitize a `CommandSection`.
@@ -258,9 +359,9 @@ fn sanitize_command(section: &CommandSection) -> Option<(String, HashSet<String>
     }
 }
 
-/// Maps each line as shellcheck sees it to its corresponding start position in
-/// the source.
-fn map_shellcheck_lines(section: &CommandSection) -> HashMap<usize, usize> {
+/// Maps each line as shellcheck sees it to its corresponding span in the
+/// source.
+fn map_shellcheck_lines(section: &CommandSection) -> HashMap<usize, Span> {
     let mut line_map = HashMap::new();
     let mut line_num = 1;
     let mut skip_next_line = false;
@@ -281,7 +382,7 @@ fn map_shellcheck_lines(section: &CommandSection) -> HashMap<usize, usize> {
                         continue;
                     };
                     let adjusted_start = text.span().start() + line_start + leading_ws;
-                    line_map.insert(line_num, adjusted_start);
+                    line_map.insert(line_num, Span::new(adjusted_start, line.len()));
                     line_num += 1;
                 }
             }
@@ -295,11 +396,12 @@ fn map_shellcheck_lines(section: &CommandSection) -> HashMap<usize, usize> {
 
 /// Calculates the correct `Span` for a `ShellCheckDiagnostic` relative to the
 /// source.
-fn calculate_span(diagnostic: &ShellCheckDiagnostic, line_map: &HashMap<usize, usize>) -> Span {
+fn calculate_span(diagnostic: &ShellCheckDiagnostic, line_map: &HashMap<usize, Span>) -> Span {
     // shellcheck 1-indexes columns, so subtract 1.
     let start = line_map
         .get(&diagnostic.line)
         .expect("shellcheck line corresponds to command line")
+        .start()
         + diagnostic.column
         - 1;
     let len = if diagnostic.end_line > diagnostic.line {
@@ -307,6 +409,7 @@ fn calculate_span(diagnostic: &ShellCheckDiagnostic, line_map: &HashMap<usize, u
         let end_line_end = line_map
             .get(&diagnostic.end_line)
             .expect("shellcheck line corresponds to command line")
+            .start()
             + diagnostic.end_column
             - 1;
         // - 2 to discount first and last newlines
@@ -387,6 +490,13 @@ impl Visitor for ShellCheckRule {
         decls.extend(cmd_decls);
         let line_map = map_shellcheck_lines(section);
 
+        // create a Fenwick tree where each index is a line number
+        // and each value is the length of the line.
+        // For efficiency, we do this only once.
+        let shift_values = lines_with_offset(&sanitized_command)
+            .map(|(_, line_start, next_start)| next_start - line_start);
+        let shift_tree = FenwickTree::from_iter(shift_values);
+
         match run_shellcheck(&sanitized_command) {
             Ok(diagnostics) => {
                 for diagnostic in diagnostics {
@@ -400,9 +510,8 @@ impl Visitor for ShellCheckRule {
                     {
                         continue;
                     }
-                    let span = calculate_span(&diagnostic, &line_map);
                     state.exceptable_add(
-                        shellcheck_lint(&diagnostic, span),
+                        shellcheck_lint(&diagnostic, &sanitized_command, &line_map, &shift_tree),
                         SyntaxElement::from(section.syntax().clone()),
                         &self.exceptable_nodes(),
                     )
@@ -423,3 +532,72 @@ impl Visitor for ShellCheckRule {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use ftree::FenwickTree;
+    use pretty_assertions::assert_eq;
+
+    use super::ShellCheckReplacement;
+    use super::normalize_replacements;
+    use crate::fix::Fixer;
+    use crate::fix::{self};
+    use crate::util::lines_with_offset;
+
+    #[test]
+    fn test_normalize_replacements() {
+        // shellcheck would see this as
+        // ABBBB
+        // BBBA
+        let ref_str = String::from("ABBBB\nBBBA");
+        let expected = String::from("AAAAA");
+        let sc_rep = ShellCheckReplacement {
+            line: 1,
+            end_line: 2,
+            column: 2,
+            end_column: 4,
+            precedence: 1,
+            insertion_point: fix::InsertionPoint::AfterEnd,
+            replacement: String::from("AAA"),
+        };
+        let shift_values =
+            lines_with_offset(&ref_str).map(|(_, line_start, next_start)| next_start - line_start);
+        let shift_tree = FenwickTree::from_iter(shift_values);
+        let normalized = normalize_replacements(&[sc_rep], &shift_tree);
+        let rep = &normalized[0];
+
+        assert_eq!(rep.start(), 1);
+        assert_eq!(rep.end(), 9);
+
+        let mut fixer = Fixer::new(ref_str);
+        fixer.apply_replacement(rep);
+        assert_eq!(fixer.value(), expected);
+    }
+
+    #[test]
+    fn test_normalize_replacements2() {
+        let ref_str = String::from("ABBBBBBBA");
+        let expected = String::from("AAAAA");
+        let sc_rep = ShellCheckReplacement {
+            line: 1,
+            end_line: 1,
+            column: 2,
+            end_column: 9,
+            precedence: 1,
+            insertion_point: fix::InsertionPoint::AfterEnd,
+            replacement: String::from("AAA"),
+        };
+        let shift_values =
+            lines_with_offset(&ref_str).map(|(_, line_start, next_start)| next_start - line_start);
+        let shift_tree = FenwickTree::from_iter(shift_values);
+        let normalized = normalize_replacements(&[sc_rep], &shift_tree);
+        let rep = &normalized[0];
+
+        assert_eq!(rep.start(), 1);
+        assert_eq!(rep.end(), 8);
+
+        let mut fixer = Fixer::new(ref_str);
+        fixer.apply_replacement(rep);
+        assert_eq!(fixer.value(), expected);
+    }
+}
diff --git a/wdl-lint/tests/lints/shellcheck-warn/source.errors b/wdl-lint/tests/lints/shellcheck-warn/source.errors
index 1781d2ff..0102914e 100644
--- a/wdl-lint/tests/lints/shellcheck-warn/source.errors
+++ b/wdl-lint/tests/lints/shellcheck-warn/source.errors
@@ -7,7 +7,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line17"`?
 
 note[ShellCheck]: line17 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:17:22
@@ -29,7 +29,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                     more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line18"`?
 
 note[ShellCheck]: line18 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:18:37
@@ -51,7 +51,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                    SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                    more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line19"`?
 
 note[ShellCheck]: line19 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:19:36
@@ -73,7 +73,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line30"`?
 
 note[ShellCheck]: line30 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:30:22
@@ -95,7 +95,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                           SC2086[info]: Double quote to prevent globbing and word splitting.
    │                           more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line31"`?
 
 note[ShellCheck]: line31 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:31:27
@@ -117,7 +117,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line49"`?
 
 note[ShellCheck]: line49 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:49:22
@@ -139,7 +139,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                     more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line50"`?
 
 note[ShellCheck]: line50 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:50:37
@@ -161,7 +161,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                    SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                    more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line51"`?
 
 note[ShellCheck]: line51 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:51:36
@@ -183,7 +183,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line52"`?
 
 note[ShellCheck]: line52 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:52:22
@@ -205,7 +205,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                           SC2086[info]: Double quote to prevent globbing and word splitting.
    │                           more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$bad_test"`?
 
 note[ShellCheck]: bad_test is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:53:27
@@ -227,7 +227,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                           SC2086[info]: Double quote to prevent globbing and word splitting.
    │                           more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$trailing_space"`?
 
 note[ShellCheck]: trailing_space is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:54:27
@@ -249,7 +249,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line72"`?
 
 note[ShellCheck]: line72 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:72:22
@@ -271,7 +271,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                     more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line73"`?
 
 note[ShellCheck]: line73 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:73:37
@@ -293,7 +293,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                    SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                    more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line74"`?
 
 note[ShellCheck]: line74 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:74:36
@@ -315,7 +315,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line75"`?
 
 note[ShellCheck]: line75 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:75:22
@@ -337,7 +337,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line76_trailing_pholder"`?
 
 note[ShellCheck]: line76_trailing_pholder is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:76:22
@@ -359,7 +359,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                     more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$leading_pholder"`?
 
 note[ShellCheck]: leading_pholder is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:77:37
@@ -381,7 +381,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line96"`?
 
 note[ShellCheck]: line96 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:96:22
@@ -403,7 +403,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                     more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line97"`?
 
 note[ShellCheck]: line97 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:97:37
@@ -425,7 +425,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                                    SC2086[info]: Double quote to prevent globbing and word splitting.
    │                                    more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line98"`?
 
 note[ShellCheck]: line98 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:98:36
@@ -447,7 +447,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
    │                      SC2086[info]: Double quote to prevent globbing and word splitting.
    │                      more info: https://www.shellcheck.net/wiki/SC2086
    │
-   = fix: address the diagnostic as recommended in the message
+   = fix: did you mean `"$line99"`?
 
 note[ShellCheck]: line99 is referenced but not assigned.
    ┌─ tests/lints/shellcheck-warn/source.wdl:99:22
@@ -469,7 +469,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                      SC2086[info]: Double quote to prevent globbing and word splitting.
     │                      more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$line100_trailing_pholder"`?
 
 note[ShellCheck]: line100_trailing_pholder is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:100:22
@@ -491,7 +491,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
     │                                     more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$leading_pholder"`?
 
 note[ShellCheck]: leading_pholder is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:101:37
@@ -513,7 +513,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                                  SC2086[info]: Double quote to prevent globbing and word splitting.
     │                                  more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$firstlinelint"`?
 
 note[ShellCheck]: firstlinelint is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:118:34
@@ -535,7 +535,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                      SC2086[info]: Double quote to prevent globbing and word splitting.
     │                      more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$line120"`?
 
 note[ShellCheck]: line120 is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:120:22
@@ -557,7 +557,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
     │                                     more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$line121"`?
 
 note[ShellCheck]: line121 is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:121:37
@@ -579,7 +579,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                                    SC2086[info]: Double quote to prevent globbing and word splitting.
     │                                    more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$line122"`?
 
 note[ShellCheck]: line122 is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:122:36
@@ -601,7 +601,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                      SC2086[info]: Double quote to prevent globbing and word splitting.
     │                      more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$line123"`?
 
 note[ShellCheck]: line123 is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:123:22
@@ -623,7 +623,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                      SC2086[info]: Double quote to prevent globbing and word splitting.
     │                      more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$line124_trailing_pholder"`?
 
 note[ShellCheck]: line124_trailing_pholder is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:124:22
@@ -645,7 +645,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                                     SC2086[info]: Double quote to prevent globbing and word splitting.
     │                                     more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$leading_pholder"`?
 
 note[ShellCheck]: leading_pholder is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:126:37
@@ -733,7 +733,7 @@ note[ShellCheck]: Double quote to prevent globbing and word splitting.
     │                        SC2086[info]: Double quote to prevent globbing and word splitting.
     │                        more info: https://www.shellcheck.net/wiki/SC2086
     │
-    = fix: address the diagnostic as recommended in the message
+    = fix: did you mean `"$lint146"`?
 
 note[ShellCheck]: lint146 is referenced but not assigned.
     ┌─ tests/lints/shellcheck-warn/source.wdl:146:24

From 08024e513bbede4c255927588e09bd496399f2fa Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Thu, 9 Jan 2025 19:06:43 -0500
Subject: [PATCH 02/15] chore: update changelog

---
 wdl-lint/CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/wdl-lint/CHANGELOG.md b/wdl-lint/CHANGELOG.md
index ad59bf69..c9c00a66 100644
--- a/wdl-lint/CHANGELOG.md
+++ b/wdl-lint/CHANGELOG.md
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Added
 
+* Improved `ShellCheck` rule fix messages and implemented the `fix` module ([#284](https://github.com/stjude-rust-labs/wdl/pull/284))
 * Added a `ShellCheck` rule ([#264](https://github.com/stjude-rust-labs/wdl/pull/264)).
 * Added a `RedundantInputAssignment` rule ([#244](https://github.com/stjude-rust-labs/wdl/pull/244)).
 

From 4c79cfb00b4b7c65d78ca432524976f87cb7bfb5 Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Sat, 11 Jan 2025 13:43:27 -0500
Subject: [PATCH 03/15] fix: assume InsertionPoint is exhaustive

---
 wdl-lint/src/fix.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index ae92f213..69322783 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -8,7 +8,6 @@ use serde::Deserialize;
 
 #[derive(Copy, Clone, Debug, Deserialize)]
 #[serde(rename_all = "camelCase")]
-#[non_exhaustive]
 /// An insertion point.
 pub enum InsertionPoint {
     /// Insert immediately before a specified region.

From 08c05bbbf98afbb84404753f793ee37ee7a972ab Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Sat, 11 Jan 2025 22:04:10 -0500
Subject: [PATCH 04/15] fix: edge cases

---
 wdl-lint/src/fix.rs                           |  2 +-
 wdl-lint/src/rules/shellcheck.rs              | 35 ++++++++++++++-----
 .../tests/lints/shellcheck-warn/source.errors | 33 +++++++++++++++++
 .../tests/lints/shellcheck-warn/source.wdl    | 20 +++++++++++
 4 files changed, 81 insertions(+), 9 deletions(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index 69322783..9aecf74d 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -97,7 +97,7 @@ impl Fixer {
     /// Create a new Fixer from a String.
     pub fn new(value: String) -> Self {
         Fixer {
-            tree: FenwickTree::from_iter(vec![0; value.len()]),
+            tree: FenwickTree::from_iter(vec![0; value.len() + 1]),
             value,
         }
     }
diff --git a/wdl-lint/src/rules/shellcheck.rs b/wdl-lint/src/rules/shellcheck.rs
index ebf4e240..7e1142f1 100644
--- a/wdl-lint/src/rules/shellcheck.rs
+++ b/wdl-lint/src/rules/shellcheck.rs
@@ -261,25 +261,38 @@ fn gather_task_declarations(task: &TaskDefinition) -> HashSet<String> {
 
 /// Create an appropriate 'fix' message.
 ///
-/// Returns only the slice of the command that falls between
-/// the first and last replacements. If there is only one replacement,
-/// this is the slice between the replacement's start and end.
-fn create_fix_message(reps: Vec<Replacement>, command_text: &str) -> String {
+/// Returns the following range of text:
+/// start = min(diagnostic highlight start, left-most replacement start)
+/// end = max(diagnostic highlight end, right-most replacement end)
+/// start..end
+fn create_fix_message(reps: Vec<Replacement>, command_text: &str, diag_span: Span) -> String {
     let mut fixer = Fixer::new(command_text.to_owned());
     // Get the original left-most and right-most replacement indices.
-    let start = reps
+    let rep_start = reps
         .iter()
         .map(|r| r.start())
         .min()
         .expect("replacements is non-empty");
-    let end = reps
+    let rep_end = reps
         .iter()
         .map(|r| r.end())
         .max()
         .expect("replacements is non-empty");
+    let start = rep_start.min(diag_span.start());
+    let end = rep_end.max(diag_span.end());
     fixer.apply_replacements(reps);
     // Adjust start and end based on final tree.
-    let adj_range = fixer.adj_range_inc(start..=end);
+    let adj_range = {
+        let range = fixer.adj_range(start..end);
+        // the prefix sum does not include the value at
+        // actual index. But, we want this value because
+        // we may have inserted text at the very end.
+        // ftree provides no method to get this value, so
+        // we must calculate it.
+        let max_pos = (end).min(fixer.value().len() - 1);
+        let extend_by = (fixer.transform(max_pos + 1) - fixer.transform(max_pos)).saturating_sub(1);
+        range.start..(range.end + extend_by)
+    };
     format!("did you mean `{}`?", &fixer.value()[adj_range])
 }
 
@@ -298,7 +311,13 @@ fn shellcheck_lint(
     let fix_msg = match diagnostic.fix {
         Some(ref fix) => {
             let reps = normalize_replacements(&fix.replacements, shift_tree);
-            create_fix_message(reps, command_text)
+            let diag_span = {
+                let start = diagnostic.column + shift_tree.prefix_sum(diagnostic.line - 1, 0) - 1;
+                let end =
+                    diagnostic.end_column + shift_tree.prefix_sum(diagnostic.end_line - 1, 0) - 1;
+                Span::new(start, end - start)
+            };
+            create_fix_message(reps, command_text, diag_span)
         }
         None => String::from("address the diagnostic as recommended in the message"),
     };
diff --git a/wdl-lint/tests/lints/shellcheck-warn/source.errors b/wdl-lint/tests/lints/shellcheck-warn/source.errors
index 0102914e..8ac30912 100644
--- a/wdl-lint/tests/lints/shellcheck-warn/source.errors
+++ b/wdl-lint/tests/lints/shellcheck-warn/source.errors
@@ -746,3 +746,36 @@ note[ShellCheck]: lint146 is referenced but not assigned.
     │
     = fix: address the diagnostic as recommended in the message
 
+note[ShellCheck]: version appears unused. Verify use (or export if used externally).
+    ┌─ tests/lints/shellcheck-warn/source.wdl:166:7
+    │
+166 │       version=`uname -r`
+    │       ^^^^^^^
+    │       │
+    │       SC2034[warning]: version appears unused. Verify use (or export if used externally).
+    │       more info: https://www.shellcheck.net/wiki/SC2034
+    │
+    = fix: address the diagnostic as recommended in the message
+
+note[ShellCheck]: Use $(...) notation instead of legacy backticks `...`.
+    ┌─ tests/lints/shellcheck-warn/source.wdl:166:15
+    │
+166 │       version=`uname -r`
+    │               ^^^^^^^^^^
+    │               │
+    │               SC2006[style]: Use $(...) notation instead of legacy backticks `...`.
+    │               more info: https://www.shellcheck.net/wiki/SC2006
+    │
+    = fix: did you mean `$(uname -r)`?
+
+note[ShellCheck]: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
+    ┌─ tests/lints/shellcheck-warn/source.wdl:168:7
+    │
+168 │       cd "DIR"
+    │       ^^^^^^^^
+    │       │
+    │       SC2164[warning]: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
+    │       more info: https://www.shellcheck.net/wiki/SC2164
+    │
+    = fix: did you mean `cd "DIR" || exit`?
+
diff --git a/wdl-lint/tests/lints/shellcheck-warn/source.wdl b/wdl-lint/tests/lints/shellcheck-warn/source.wdl
index 66760195..8b4f230f 100644
--- a/wdl-lint/tests/lints/shellcheck-warn/source.wdl
+++ b/wdl-lint/tests/lints/shellcheck-warn/source.wdl
@@ -152,3 +152,23 @@ task test5 {
 
     runtime {}
 }
+
+task test6 {
+    meta {}
+
+    parameter_meta {}
+
+    input {
+      Int placeholder
+    }
+
+    command <<<
+      version=`uname -r`
+
+      cd "DIR"
+    >>>
+
+    output {}
+
+    runtime {}
+}

From 57722c3118ddfb26a398da138ac93b360e6e013d Mon Sep 17 00:00:00 2001
From: Spencer Richman <richmanspencer@gmail.com>
Date: Sun, 12 Jan 2025 10:52:57 -0500
Subject: [PATCH 05/15] Apply suggestions from code review

Co-authored-by: Andrew Frantz <andrew.frantz@stjude.org>
---
 wdl-lint/src/fix.rs              | 4 ++--
 wdl-lint/src/rules/shellcheck.rs | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index 9aecf74d..83f53661 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -102,7 +102,7 @@ impl Fixer {
         }
     }
 
-    /// Apply a `Replacement` to the value contained in the Fixer.
+    /// Apply a [`Replacement`] to the value contained in the Fixer.
     pub fn apply_replacement(&mut self, rep: &Replacement) {
         let old_start = rep.start;
         let old_end = rep.end;
@@ -122,7 +122,7 @@ impl Fixer {
             .replace_range(new_start..new_end, &rep.replacement);
     }
 
-    /// Apply multiple `Replacement`s in the correct order.
+    /// Apply multiple [`Replacement`]s in the correct order.
     ///
     /// Order is determined by the precedence field.
     /// Higher precedences are applied first.
diff --git a/wdl-lint/src/rules/shellcheck.rs b/wdl-lint/src/rules/shellcheck.rs
index 7e1142f1..d3a4394b 100644
--- a/wdl-lint/src/rules/shellcheck.rs
+++ b/wdl-lint/src/rules/shellcheck.rs
@@ -78,10 +78,10 @@ struct ShellCheckFix {
 #[derive(Clone, Debug, Deserialize)]
 /// A ShellCheck replacement.
 ///
-/// This differs from a `Replacement` in that
+/// This differs from a [`Replacement`] in that
 /// 1) columns are 1-indexed
 /// 2) it may span multiple lines and thus cannot be directly passed to a
-///    `Fixer`.
+///    [`Fixer`].
 ///
 /// It must be normalized with `normalize_replacements` before use.
 struct ShellCheckReplacement {
@@ -129,7 +129,7 @@ struct ShellCheckDiagnostic {
     pub fix: Option<ShellCheckFix>,
 }
 
-/// Convert `ShellCheckReplacement`s into `Replacement`s.
+/// Convert [`ShellCheckReplacement`]s into [`Replacement`]s.
 ///
 /// Column indices are shifted to 0-based.
 /// Multi-line replacements are normalized so that column indices are

From 8f48ba93abf7d17fd62443a259a7039200dcf59b Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Sun, 12 Jan 2025 12:11:30 -0500
Subject: [PATCH 06/15] fix: move ftree depdency to workspace

---
 Cargo.toml          | 1 +
 wdl-lint/Cargo.toml | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Cargo.toml b/Cargo.toml
index 63395e12..7726f063 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -32,6 +32,7 @@ colored = "2.1.0"
 convert_case = "0.6.0"
 dirs = "5.0.1"
 faster-hex = "0.9.0"
+ftree = "1.2.0"
 futures = "0.3.30"
 git2 = "0.18.3"
 glob = "0.3.1"
diff --git a/wdl-lint/Cargo.toml b/wdl-lint/Cargo.toml
index 155ee8df..381ea3eb 100644
--- a/wdl-lint/Cargo.toml
+++ b/wdl-lint/Cargo.toml
@@ -15,7 +15,7 @@ readme = "../README.md"
 wdl-ast = { path = "../wdl-ast", version = "0.9.0" }
 anyhow = { workspace = true }
 convert_case = { workspace = true }
-ftree = "1.2.0"
+ftree = { workspace = true }
 indexmap = { workspace = true }
 rand = { workspace = true }
 rowan = { workspace = true }

From 82c42d611082b8c75ab3aa90ec52d273fff411b3 Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Sun, 12 Jan 2025 12:12:54 -0500
Subject: [PATCH 07/15] fix: variable naming and doc link format

---
 wdl-lint/src/fix.rs              | 10 +++++-----
 wdl-lint/src/rules/shellcheck.rs | 33 ++++++++++++++++++--------------
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index 83f53661..a71577a1 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -26,7 +26,7 @@ pub struct Replacement {
     /// where to insert the replacement.
     insertion_point: InsertionPoint,
     /// Value to be inserted.
-    replacement: String,
+    value: String,
     /// Precedence for replacement. Higher precedences are applied first.
     precedence: usize,
 }
@@ -38,14 +38,14 @@ impl Replacement {
         start: usize,
         end: usize,
         insertion_point: InsertionPoint,
-        replacement: String,
+        value: String,
         precedence: usize,
     ) -> Self {
         Replacement {
             start,
             end,
             insertion_point,
-            replacement,
+            value,
             precedence,
         }
     }
@@ -66,8 +66,8 @@ impl Replacement {
     }
 
     /// Value to be inserted.
-    pub fn replacement(&self) -> &str {
-        self.replacement.as_ref()
+    pub fn value(&self) -> &str {
+        self.value.as_ref()
     }
 
     /// Precedence for replacement. Higher precedences are applied first.
diff --git a/wdl-lint/src/rules/shellcheck.rs b/wdl-lint/src/rules/shellcheck.rs
index d3a4394b..057e7490 100644
--- a/wdl-lint/src/rules/shellcheck.rs
+++ b/wdl-lint/src/rules/shellcheck.rs
@@ -135,10 +135,11 @@ struct ShellCheckDiagnostic {
 /// Multi-line replacements are normalized so that column indices are
 /// as though the string is on a single line.
 fn normalize_replacements(
-    reps: &[ShellCheckReplacement],
+    replacements: &[ShellCheckReplacement],
     shift_tree: &FenwickTree<usize>,
 ) -> Vec<Replacement> {
-    reps.iter()
+    replacements
+        .iter()
         .map(|r| {
             Replacement::new(
                 r.column + shift_tree.prefix_sum(r.line - 1, 0) - 1,
@@ -265,38 +266,42 @@ fn gather_task_declarations(task: &TaskDefinition) -> HashSet<String> {
 /// start = min(diagnostic highlight start, left-most replacement start)
 /// end = max(diagnostic highlight end, right-most replacement end)
 /// start..end
-fn create_fix_message(reps: Vec<Replacement>, command_text: &str, diag_span: Span) -> String {
+fn create_fix_message(
+    replacements: Vec<Replacement>,
+    command_text: &str,
+    diagnostic_span: Span,
+) -> String {
     let mut fixer = Fixer::new(command_text.to_owned());
     // Get the original left-most and right-most replacement indices.
-    let rep_start = reps
+    let rep_start = replacements
         .iter()
         .map(|r| r.start())
         .min()
         .expect("replacements is non-empty");
-    let rep_end = reps
+    let rep_end = replacements
         .iter()
         .map(|r| r.end())
         .max()
         .expect("replacements is non-empty");
-    let start = rep_start.min(diag_span.start());
-    let end = rep_end.max(diag_span.end());
-    fixer.apply_replacements(reps);
+    let start = rep_start.min(diagnostic_span.start());
+    let end = rep_end.max(diagnostic_span.end());
+    fixer.apply_replacements(replacements);
     // Adjust start and end based on final tree.
     let adj_range = {
         let range = fixer.adj_range(start..end);
         // the prefix sum does not include the value at
-        // actual index. But, we want this value because
+        // the actual index. But, we want this value because
         // we may have inserted text at the very end.
         // ftree provides no method to get this value, so
         // we must calculate it.
-        let max_pos = (end).min(fixer.value().len() - 1);
-        let extend_by = (fixer.transform(max_pos + 1) - fixer.transform(max_pos)).saturating_sub(1);
+        let max_pos = (end + 1).min(fixer.value().len());
+        let extend_by = (fixer.transform(max_pos) - fixer.transform(max_pos - 1)).saturating_sub(1);
         range.start..(range.end + extend_by)
     };
     format!("did you mean `{}`?", &fixer.value()[adj_range])
 }
 
-/// Creates a "ShellCheck lint" diagnostic from a `ShellCheckDiagnostic`
+/// Creates a "ShellCheck lint" diagnostic from a [ShellCheckDiagnostic]
 fn shellcheck_lint(
     diagnostic: &ShellCheckDiagnostic,
     command_text: &str,
@@ -331,7 +336,7 @@ fn shellcheck_lint(
         .with_fix(fix_msg)
 }
 
-/// Sanitize a `CommandSection`.
+/// Sanitize a [CommandSection].
 ///
 /// Removes all trailing whitespace, replaces placeholders
 /// with dummy bash variables or literals, and records declarations.
@@ -413,7 +418,7 @@ fn map_shellcheck_lines(section: &CommandSection) -> HashMap<usize, Span> {
     line_map
 }
 
-/// Calculates the correct `Span` for a `ShellCheckDiagnostic` relative to the
+/// Calculates the correct [Span] for a [ShellCheckDiagnostic] relative to the
 /// source.
 fn calculate_span(diagnostic: &ShellCheckDiagnostic, line_map: &HashMap<usize, Span>) -> Span {
     // shellcheck 1-indexes columns, so subtract 1.

From 72f31a5cc810e8add15657d7fabc634abd7c7403 Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Sun, 12 Jan 2025 12:14:12 -0500
Subject: [PATCH 08/15] feat: add comments and bounds checking to Fixer

---
 wdl-lint/src/fix.rs | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index a71577a1..3fea44a4 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -84,6 +84,14 @@ impl Replacement {
 /// replacements referencing only the original input. Although the canonical
 /// Fenwick tree is 1-indexed this implementation is 0-indexed, so replacement
 /// positions must be directly equivalent to string indices.
+///
+/// The length of the tree is initialized to be 1 longer
+/// than the length of the initial value. This is because ftree provides
+/// no API for accessing the value of the final position, and the prefix sum
+/// only provides the cumulative sum < index. The extra index makes it possible
+/// to calculate the sum of the entire tree, which is necessary to enable
+/// slices of the value up to and beyond the original end of the value.
+/// Attempting to apply a replacement at this position will panic.
 #[derive(Clone, Debug)]
 pub struct Fixer {
     /// The string to be modified.
@@ -103,23 +111,27 @@ impl Fixer {
     }
 
     /// Apply a [`Replacement`] to the value contained in the Fixer.
-    pub fn apply_replacement(&mut self, rep: &Replacement) {
-        let old_start = rep.start;
-        let old_end = rep.end;
+    pub fn apply_replacement(&mut self, replacement: &Replacement) {
+        let old_start = replacement.start;
+        let old_end = replacement.end;
         let new_start = self.transform(old_start);
         let new_end = self.transform(old_end);
 
         let rep_len =
-            i32::try_from(rep.replacement().len()).expect("replacement length fits into i32");
+            i32::try_from(replacement.value().len()).expect("replacement length fits into i32");
         let range = i32::try_from(old_end - old_start).expect("range fits into i32");
         let shift = rep_len - range;
-        let insert_at = match rep.insertion_point {
+        let insert_at = match replacement.insertion_point {
             InsertionPoint::BeforeStart => old_start,
             InsertionPoint::AfterEnd => old_end + 1,
         };
+        assert!(
+            insert_at <= self.tree().len(),
+            "attempt to insert out-of-bounds"
+        );
         self.tree.add_at(insert_at, shift);
         self.value
-            .replace_range(new_start..new_end, &rep.replacement);
+            .replace_range(new_start..new_end, &replacement.value);
     }
 
     /// Apply multiple [`Replacement`]s in the correct order.
@@ -243,4 +255,15 @@ mod tests {
         fixer2.apply_replacements(vec![rep2, rep]);
         assert_eq!(fixer2.value(), "This statement is true.");
     }
+
+    #[test]
+    #[should_panic]
+    fn test_out_of_bounds_insert() {
+        let value = String::from("012345");
+        let ins = String::from("6");
+        let rep = Replacement::new(7, 7, InsertionPoint::AfterEnd, ins, 1);
+
+        let mut fixer = Fixer::new(value);
+        fixer.apply_replacement(&rep);
+    }
 }

From 5398f900bb23fa061599f917c2d05bd5e6d175ed Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Sun, 12 Jan 2025 12:18:14 -0500
Subject: [PATCH 09/15] fix: add comments and rename variables

---
 wdl-lint/src/rules/shellcheck.rs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/wdl-lint/src/rules/shellcheck.rs b/wdl-lint/src/rules/shellcheck.rs
index 057e7490..305121e7 100644
--- a/wdl-lint/src/rules/shellcheck.rs
+++ b/wdl-lint/src/rules/shellcheck.rs
@@ -312,17 +312,19 @@ fn shellcheck_lint(
         "SC{}[{}]: {}",
         diagnostic.code, diagnostic.level, diagnostic.message
     );
+    // This span is relative to the entire document.
     let span = calculate_span(diagnostic, line_map);
     let fix_msg = match diagnostic.fix {
         Some(ref fix) => {
             let reps = normalize_replacements(&fix.replacements, shift_tree);
-            let diag_span = {
+            // This span is relative to the command text.
+            let diagnostic_span = {
                 let start = diagnostic.column + shift_tree.prefix_sum(diagnostic.line - 1, 0) - 1;
                 let end =
                     diagnostic.end_column + shift_tree.prefix_sum(diagnostic.end_line - 1, 0) - 1;
                 Span::new(start, end - start)
             };
-            create_fix_message(reps, command_text, diag_span)
+            create_fix_message(reps, command_text, diagnostic_span)
         }
         None => String::from("address the diagnostic as recommended in the message"),
     };

From 500b64e382c767aeae6f3be96cc3f84cd31294fb Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Sun, 12 Jan 2025 12:33:04 -0500
Subject: [PATCH 10/15] feat: add comments

---
 wdl-lint/src/fix.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index 3fea44a4..982a888a 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -90,7 +90,7 @@ impl Replacement {
 /// no API for accessing the value of the final position, and the prefix sum
 /// only provides the cumulative sum < index. The extra index makes it possible
 /// to calculate the sum of the entire tree, which is necessary to enable
-/// slices of the value up to and beyond the original end of the value.
+/// slices of the new value beyond the original end position.
 /// Attempting to apply a replacement at this position will panic.
 #[derive(Clone, Debug)]
 pub struct Fixer {
@@ -125,6 +125,9 @@ impl Fixer {
             InsertionPoint::BeforeStart => old_start,
             InsertionPoint::AfterEnd => old_end + 1,
         };
+        // The final position in the tree is reserved
+        // to work around the ftree API and is not
+        // a valid insertion point.
         assert!(
             insert_at <= self.tree().len(),
             "attempt to insert out-of-bounds"

From 18ee1b020d9c2320c6c39ea03702cff40becaf95 Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Mon, 13 Jan 2025 16:50:19 -0500
Subject: [PATCH 11/15] fix: rename replacement to value

---
 wdl-lint/src/rules/shellcheck.rs | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/wdl-lint/src/rules/shellcheck.rs b/wdl-lint/src/rules/shellcheck.rs
index 305121e7..2dedd3b7 100644
--- a/wdl-lint/src/rules/shellcheck.rs
+++ b/wdl-lint/src/rules/shellcheck.rs
@@ -101,7 +101,8 @@ struct ShellCheckReplacement {
     #[serde(rename = "endColumn")]
     pub end_column: usize,
     /// Replacement text.
-    pub replacement: String,
+    #[serde(rename = "replacement")]
+    pub value: String,
 }
 
 /// A ShellCheck diagnostic.
@@ -145,7 +146,7 @@ fn normalize_replacements(
                 r.column + shift_tree.prefix_sum(r.line - 1, 0) - 1,
                 r.end_column + shift_tree.prefix_sum(r.end_line - 1, 0) - 1,
                 r.insertion_point,
-                r.replacement.clone(),
+                r.value.clone(),
                 r.precedence,
             )
         })
@@ -584,7 +585,7 @@ mod tests {
             end_column: 4,
             precedence: 1,
             insertion_point: fix::InsertionPoint::AfterEnd,
-            replacement: String::from("AAA"),
+            value: String::from("AAA"),
         };
         let shift_values =
             lines_with_offset(&ref_str).map(|(_, line_start, next_start)| next_start - line_start);
@@ -611,7 +612,7 @@ mod tests {
             end_column: 9,
             precedence: 1,
             insertion_point: fix::InsertionPoint::AfterEnd,
-            replacement: String::from("AAA"),
+            value: String::from("AAA"),
         };
         let shift_values =
             lines_with_offset(&ref_str).map(|(_, line_start, next_start)| next_start - line_start);

From eb482ddd9d3c1528d00be2a464cb6dc4cab421d0 Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Thu, 16 Jan 2025 18:07:31 -0500
Subject: [PATCH 12/15] chore: move doc comments above derive macros

---
 wdl-lint/src/fix.rs              | 8 ++++----
 wdl-lint/src/rules/shellcheck.rs | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index 982a888a..3e6452cd 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -6,9 +6,9 @@ use std::ops::RangeInclusive;
 use ftree::FenwickTree;
 use serde::Deserialize;
 
+/// An insertion point.
 #[derive(Copy, Clone, Debug, Deserialize)]
 #[serde(rename_all = "camelCase")]
-/// An insertion point.
 pub enum InsertionPoint {
     /// Insert immediately before a specified region.
     BeforeStart,
@@ -16,8 +16,8 @@ pub enum InsertionPoint {
     AfterEnd,
 }
 
-#[derive(Clone, Debug)]
 /// A replacement to be applied to a String.
+#[derive(Clone, Debug)]
 pub struct Replacement {
     /// The start position of the replacement.
     start: usize,
@@ -157,7 +157,7 @@ impl Fixer {
     ///
     /// Panics if the input does not fall within the bounds of the Fixer's
     /// value or if the adjusted index does not fit within usize.
-    pub fn adj_range(&self, range: Range<usize>) -> Range<usize> {
+    pub fn adjust_range(&self, range: Range<usize>) -> Range<usize> {
         self.transform(range.start)..self.transform(range.end)
     }
 
@@ -166,7 +166,7 @@ impl Fixer {
     ///
     /// Panics if the input does not fall within the bounds of the Fixer's
     /// value or if the adjusted index does not fit within usize.
-    pub fn adj_range_inc(&self, range: RangeInclusive<usize>) -> RangeInclusive<usize> {
+    pub fn adjust_range_inclusive(&self, range: RangeInclusive<usize>) -> RangeInclusive<usize> {
         self.transform(*range.start())..=self.transform(*range.end())
     }
 
diff --git a/wdl-lint/src/rules/shellcheck.rs b/wdl-lint/src/rules/shellcheck.rs
index 2dedd3b7..15e3a288 100644
--- a/wdl-lint/src/rules/shellcheck.rs
+++ b/wdl-lint/src/rules/shellcheck.rs
@@ -68,14 +68,13 @@ static SHELLCHECK_EXISTS: OnceLock<bool> = OnceLock::new();
 /// The identifier for the command section ShellCheck rule.
 const ID: &str = "ShellCheck";
 
-#[derive(Clone, Debug, Deserialize)]
 /// Suggested fix for a ShellCheck diagnostic.
+#[derive(Clone, Debug, Deserialize)]
 struct ShellCheckFix {
     /// The replacements to perform.
     pub replacements: Vec<ShellCheckReplacement>,
 }
 
-#[derive(Clone, Debug, Deserialize)]
 /// A ShellCheck replacement.
 ///
 /// This differs from a [`Replacement`] in that
@@ -84,6 +83,7 @@ struct ShellCheckFix {
 ///    [`Fixer`].
 ///
 /// It must be normalized with `normalize_replacements` before use.
+#[derive(Clone, Debug, Deserialize)]
 struct ShellCheckReplacement {
     /// Line number replacement occurs on.
     pub line: usize,
@@ -289,7 +289,7 @@ fn create_fix_message(
     fixer.apply_replacements(replacements);
     // Adjust start and end based on final tree.
     let adj_range = {
-        let range = fixer.adj_range(start..end);
+        let range = fixer.adjust_range(start..end);
         // the prefix sum does not include the value at
         // the actual index. But, we want this value because
         // we may have inserted text at the very end.

From a0e61124fb402281175f04213acc9d06f29c14a6 Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Thu, 16 Jan 2025 18:09:45 -0500
Subject: [PATCH 13/15] chore: add panic comment to apply_replacement

---
 wdl-lint/src/fix.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index 3e6452cd..ddaed258 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -111,6 +111,8 @@ impl Fixer {
     }
 
     /// Apply a [`Replacement`] to the value contained in the Fixer.
+    ///
+    /// Panics if the replacement is out-of-bounds.
     pub fn apply_replacement(&mut self, replacement: &Replacement) {
         let old_start = replacement.start;
         let old_end = replacement.end;

From 7a801b033f6f6a0749e2abb3197d6bad8cec7ca7 Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Thu, 16 Jan 2025 20:45:42 -0500
Subject: [PATCH 14/15] fix: remove unused method

---
 wdl-lint/src/fix.rs | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index ddaed258..9a49e270 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -31,7 +31,6 @@ pub struct Replacement {
     precedence: usize,
 }
 
-#[allow(unused)]
 impl Replacement {
     /// Create a new `Replacement`.
     pub fn new(
@@ -163,15 +162,6 @@ impl Fixer {
         self.transform(range.start)..self.transform(range.end)
     }
 
-    /// Given a RangeInclusive, update the bounds to account for any applied
-    /// replacements.
-    ///
-    /// Panics if the input does not fall within the bounds of the Fixer's
-    /// value or if the adjusted index does not fit within usize.
-    pub fn adjust_range_inclusive(&self, range: RangeInclusive<usize>) -> RangeInclusive<usize> {
-        self.transform(*range.start())..=self.transform(*range.end())
-    }
-
     /// Returns a reference to the internal Fenwick tree.
     pub fn tree(&self) -> &FenwickTree<i32> {
         &self.tree

From 74f64a3aa74890b8e2eb18c66f06f8f553c8476d Mon Sep 17 00:00:00 2001
From: thatrichman <richmanspencer@gmail.com>
Date: Thu, 16 Jan 2025 21:03:58 -0500
Subject: [PATCH 15/15] chore: fix clippy lints

---
 wdl-lint/src/fix.rs | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/wdl-lint/src/fix.rs b/wdl-lint/src/fix.rs
index 9a49e270..beb57e3f 100644
--- a/wdl-lint/src/fix.rs
+++ b/wdl-lint/src/fix.rs
@@ -1,7 +1,6 @@
 //! Module for applying fixes for diagnostics.
 
 use std::ops::Range;
-use std::ops::RangeInclusive;
 
 use ftree::FenwickTree;
 use serde::Deserialize;
@@ -122,7 +121,7 @@ impl Fixer {
             i32::try_from(replacement.value().len()).expect("replacement length fits into i32");
         let range = i32::try_from(old_end - old_start).expect("range fits into i32");
         let shift = rep_len - range;
-        let insert_at = match replacement.insertion_point {
+        let insert_at = match replacement.insertion_point() {
             InsertionPoint::BeforeStart => old_start,
             InsertionPoint::AfterEnd => old_end + 1,
         };
@@ -143,7 +142,7 @@ impl Fixer {
     /// Order is determined by the precedence field.
     /// Higher precedences are applied first.
     pub fn apply_replacements(&mut self, mut reps: Vec<Replacement>) {
-        reps.sort_by_key(|r| r.precedence);
+        reps.sort_by_key(|r| r.precedence());
         reps.iter().rev().for_each(|r| self.apply_replacement(r));
     }