Repository: reubeno/brush
Pull request #385 (2025-02-18 07:51:43)

fix(builtins): correct read handling of IFS/space
* Adds a handful of compat test cases for `read`, with a specific focus on IFS handling and `-d` handling.
* Fixes multiple issues with `read` to get tests to pass; notably, implementing support for `-d ''` (i.e., empty delimiter) and correcting semantics with how IFS space characters delimit.
* Along the way, corrected discrepancies with how `declare` et al. format certain strings (e.g., upgrading to ANSI-C quoting when the string includes control characters, including newlines).

github-actions[bot]: ## Test Results
    2 files      9 suites   1m 41s ⏱️
  686 tests   686 ✅ 0 💤 0 ❌
1 358 runs  1 358 ✅ 0 💤 0 ❌

Results for commit b95f9e62.

[test-results]:data:application/gzip;base64,H4sIABw/tGcC/13MSw6DIBSF4a0Yxh3ApfLoZhoq3IRUpeExMt17kWirHX7/Sc5C0I8ukVsHl46k4nODrrAlmuzDXMkoq6FOeR2FErvuqQzDf3r6V030G9D48RRcjCFuJZZ5/WS8V5v2z1P6fTYfLpuPj0OYJp8ryEP3qJ0AVCgoCCMZMEQrr1Iajkr3VnAHAOT9AUoSHO0HAQAA

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   18.32 μs` | `   18.14 μs` | `   -0.18 μs` | `🟢 -0.98%    ` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.87 μs` | `    1.82 μs` | `   -0.05 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   22.01 μs` | `   21.73 μs` | `   -0.28 μs` | `🟢 -1.27%    ` |
| `function_call                       ` | `    2.46 μs` | `    2.39 μs` | `   -0.07 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   57.15 μs` | `   57.44 μs` | `    0.29 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `21267.27 μs` | `21997.51 μs` | `  730.24 μs` | `🟠 +3.43%    ` |
| `parse_bash_completion               ` | ` 1698.57 μs` | ` 1677.39 μs` | `  -21.18 μs` | `🟢 -1.25%    ` |
| `parse_sample_script                 ` | `    1.75 μs` | `    1.79 μs` | `    0.04 μs` | `🟠 +2.52%    ` |
| `run_echo_builtin_command            ` | `   15.17 μs` | `   14.97 μs` | `   -0.19 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2288.02 μs` | ` 2325.94 μs` | `   37.92 μs` | `⚪  Unchanged` |
| `tokenize_sample_script              ` | `    3.06 μs` | `    2.83 μs` | `   -0.23 μs` | `🟢 -7.36%    ` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/export.rs | 🟠 74.07% | 🟢 76.92% | 🟢 2.85% |
| brush-core/src/builtins/read.rs | 🟠 66.47% | 🟢 86.86% | 🟢 20.39% |
| brush-core/src/escape.rs | 🟢 87% | 🟢 89.44% | 🟢 2.44% |
| brush-core/src/expansion.rs | 🟢 96.14% | 🟢 96.18% | 🟢 0.04% |
| brush-core/src/variables.rs | 🟢 87.06% | 🟢 86.96% | 🔴 -0.1% |
| **Overall Coverage** | **🟢 77.98%** | **🟢 78.33%** | **🟢 0.35%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `78.33%`

# Test Summary: bash-completion test suite
| Outcome            | Count                   | Percentage |
| ------------------ | ----------------------: | ---------: |
| ✅ Pass            | 1308            | <span style='color:green'>62.29</span> |
| ❗️ Error           | 239           | <span style='color:red'>11.38</span> |
| ❌ Fail            | 184            | <span style='color:red'>8.76</span> |
| ⏩ Skip            | 355            | 16.90 |
| ❎ Expected Fail   | 13   | 0.62 |
| ✔️ Unexpected Pass | 1 | <span style='color:red'>0.05</span> |
| 📊 Total           | 2100           | 100.00 |

<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-core/src/builtins/complete.rs b/brush-core/src/builtins/complete.rs
index ee600bc..bb8793d 100644
--- a/brush-core/src/builtins/complete.rs
+++ b/brush-core/src/builtins/complete.rs
@@ -389,14 +389,14 @@ impl CompleteCommand {
             write!(
                 s,
                 " -G {}",
-                escape::force_quote(glob_pattern, escape::QuoteMode::Quote)
+                escape::force_quote(glob_pattern, escape::QuoteMode::SingleQuote)
             )?;
         }
         if let Some(word_list) = &spec.word_list {
             write!(
                 s,
                 " -W {}",
-                escape::force_quote(word_list, escape::QuoteMode::Quote)
+                escape::force_quote(word_list, escape::QuoteMode::SingleQuote)
             )?;
         }
         if let Some(function_name) = &spec.function_name {
@@ -406,28 +406,28 @@ impl CompleteCommand {
             write!(
                 s,
                 " -C {}",
-                escape::force_quote(command, escape::QuoteMode::Quote)
+                escape::force_quote(command, escape::QuoteMode::SingleQuote)
             )?;
         }
         if let Some(filter_pattern) = &spec.filter_pattern {
             write!(
                 s,
                 " -X {}",
-                escape::force_quote(filter_pattern, escape::QuoteMode::Quote)
+                escape::force_quote(filter_pattern, escape::QuoteMode::SingleQuote)
             )?;
         }
         if let Some(prefix) = &spec.prefix {
             write!(
                 s,
                 " -P {}",
-                escape::force_quote(prefix, escape::QuoteMode::Quote)
+                escape::force_quote(prefix, escape::QuoteMode::SingleQuote)
             )?;
         }
         if let Some(suffix) = &spec.suffix {
             write!(
                 s,
                 " -S {}",
-                escape::force_quote(suffix, escape::QuoteMode::Quote)
+                escape::force_quote(suffix, escape::QuoteMode::SingleQuote)
             )?;
         }
 
diff --git a/brush-core/src/builtins/export.rs b/brush-core/src/builtins/export.rs
index 98088e9..dc521c1 100644
--- a/brush-core/src/builtins/export.rs
+++ b/brush-core/src/builtins/export.rs
@@ -94,12 +94,12 @@ impl builtins::Command for ExportCommand {
             // Enumerate variables, sorted by key.
             for (name, variable) in context.shell.env.iter().sorted_by_key(|v| v.0) {
                 if variable.is_exported() {
-                    writeln!(
-                        context.stdout(),
-                        "declare -x {}=\"{}\"",
-                        name,
-                        variable.value().to_cow_str(context.shell)
-                    )?;
+                    let value = variable.value().try_get_cow_str(context.shell);
+                    if let Some(value) = value {
+                        writeln!(context.stdout(), "declare -x {name}=\"{value}\"")?;
+                    } else {
+                        writeln!(context.stdout(), "declare -x {name}")?;
+                    }
                 }
             }
         }
diff --git a/brush-core/src/builtins/read.rs b/brush-core/src/builtins/read.rs
index 09247eb..7f66eda 100644
--- a/brush-core/src/builtins/read.rs
+++ b/brush-core/src/builtins/read.rs
@@ -89,13 +89,16 @@ impl builtins::Command for ReadCommand {
             context.stdin()
         };
 
+        // Retrieve effective value of IFS for splitting.
+        let ifs = context.shell.get_ifs();
+
         let input_line = self.read_line(input_stream, context.stdout())?;
 
         if let Some(input_line) = input_line {
-            let mut fields: VecDeque<_> = split_line_by_ifs(&context, input_line.as_str());
-
             // If -a was specified, then place the fields as elements into the array.
             if let Some(array_variable) = &self.array_variable {
+                let fields: VecDeque<_> =
+                    split_line_by_ifs(ifs.as_ref(), input_line.as_str(), None /*max_fields*/);
                 let literal_fields = fields.into_iter().map(|f| (None, f)).collect();
 
                 context.shell.env.update_or_add(
@@ -106,6 +109,12 @@ impl builtins::Command for ReadCommand {
                     env::EnvironmentScope::Global,
                 )?;
             } else if !self.variable_names.is_empty() {
+                let mut fields: VecDeque<_> = split_line_by_ifs(
+                    ifs.as_ref(),
+                    input_line.as_str(),
+                    /*max_fields*/ Some(self.variable_names.len()),
+                );
+
                 for (i, name) in self.variable_names.iter().enumerate() {
                     if fields.is_empty() {
                         // Ensure the var is empty.
@@ -142,11 +151,11 @@ impl builtins::Command for ReadCommand {
                     }
                 }
             } else {
-                // If no variable names were specified, then place the fields into the
+                // If no variable names were specified, then place everything into the
                 // REPLY variable.
                 context.shell.env.update_or_add(
                     "REPLY",
-                    variables::ShellValueLiteral::Scalar(fields.into_iter().join(" ")),
+                    variables::ShellValueLiteral::Scalar(input_line),
                     |_| Ok(()),
                     env::EnvironmentLookup::Anywhere,
                     env::EnvironmentScope::Global,
@@ -178,7 +187,14 @@ impl ReadCommand {
         let delimiter = if self.return_after_n_chars_no_delimiter.is_some() {
             None
         } else if let Some(delimiter_str) = &self.delimiter {
-            delimiter_str.chars().next()
+            // If the delimiter string is empty, then the docs indicate we need to use
+            // the NUL character as the actual delimiter.
+            if delimiter_str.is_empty() {
+                Some('\0')
+            } else {
+                // In other cases, use the first character in the string as the delimiter.
+                delimiter_str.chars().next()
+            }
         } else {
             Some('\n')
         };
@@ -274,12 +290,93 @@ impl ReadCommand {
     }
 }
 
-fn split_line_by_ifs(context: &commands::ExecutionContext<'_>, line: &str) -> VecDeque<String> {
-    // Retrieve effective value of IFS for splitting.
-    let ifs = context.shell.get_ifs();
-    let split_chars: Vec<char> = ifs.chars().collect();
+fn split_line_by_ifs(ifs: &str, line: &str, max_fields: Option<usize>) -> VecDeque<String> {
+    // Separate out the chars to split by.
+    let ifs_chars = ifs.chars().collect::<Vec<_>>();
+
+    // Compute which IFS characters are one of the default 3 whitespace chars.
+    let ifs_space_chars = ifs_chars
+        .iter()
+        .copied()
+        .filter(|c| *c == ' ' || *c == '\t' || *c == '\n')
+        .collect::<Vec<_>>();
+
+    // First, trim from the prefix and suffix of the string any matching chars that are
+    // *both* whitespace and present in the IFS string.
+    let trimmed_line = line.trim_matches(ifs_space_chars.as_slice());
+    if trimmed_line.is_empty() {
+        return VecDeque::new();
+    }
+
+    let max_fields = max_fields.unwrap_or(usize::MAX);
+
+    // Now, iterate through the string, manually splitting it by the shell's rules for
+    // honoring IFS. We implement this by hand because we need to ensure that any
+    // IFS character is a valid delimiter, *but* consecutive adjacent IFS characters
+    // are considered a single delimiter if and only if they are one of the 3 default
+    // whitespace characters (i.e., ' ', '\t', or '\n').
+    let mut fields = VecDeque::new();
+    let mut current_field = String::new();
+    let mut skipping_ifs_whitespace = false;
 
-    line.split(split_chars.as_slice())
-        .map(|field| field.to_owned())
-        .collect()
+    for c in trimmed_line.chars() {
+        if skipping_ifs_whitespace {
+            if ifs_space_chars.contains(&c) {
+                continue;
+            }
+
+            skipping_ifs_whitespace = false;
+        }
+
+        if fields.len() + 1 < max_fields && ifs_chars.contains(&c) {
+            fields.push_back(current_field);
+            current_field = String::new();
+            skipping_ifs_whitespace = ifs_space_chars.contains(&c);
+        } else {
+            current_field.push(c);
+        }
+    }
+
+    fields.push_back(current_field);
+
+    fields
+}
+
+#[cfg(test)]
+mod tests {
+    use itertools::assert_equal;
+
+    use super::*;
+
+    #[test]
+    fn test_split_line_by_ifs() {
+        let result = split_line_by_ifs(",", "a,b,c", None);
+        assert_equal(result, VecDeque::from(vec!["a", "b", "c"]));
+    }
+
+    #[test]
+    fn test_split_line_by_ifs_leading_or_trailing_space() {
+        // Test with leading or trailing space.
+        let result = split_line_by_ifs(" ", "  a b c ", None);
+        assert_equal(result, VecDeque::from(vec!["a", "b", "c"]));
+    }
+
+    #[test]
+    fn test_split_line_by_ifs_extra_interior_space() {
+        // Test with leading or trailing space.
+        let result = split_line_by_ifs(" ", "a  b c", None);
+        assert_equal(result, VecDeque::from(vec!["a", "b", "c"]));
+    }
+
+    #[test]
+    fn test_split_line_by_ifs_leading_non_space_delimiter() {
+        let result = split_line_by_ifs(",", ",a,b,c", None);
+        assert_equal(result, VecDeque::from(vec!["", "a", "b", "c"]));
+    }
+
+    #[test]
+    fn test_split_line_by_ifs_trailing_non_space_delimiter() {
+        let result = split_line_by_ifs(",", "a,b,c,", None);
+        assert_equal(result, VecDeque::from(vec!["a", "b", "c", ""]));
+    }
 }
diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs
index 3c6ddfd..ee3edbd 100644
--- a/brush-core/src/commands.rs
+++ b/brush-core/src/commands.rs
@@ -171,14 +171,14 @@ impl From<&String> for CommandArg {
 impl CommandArg {
     pub fn quote_for_tracing(&self) -> Cow<'_, str> {
         match self {
-            CommandArg::String(s) => escape::quote_if_needed(s, escape::QuoteMode::Quote),
+            CommandArg::String(s) => escape::quote_if_needed(s, escape::QuoteMode::SingleQuote),
             CommandArg::Assignment(a) => {
                 let mut s = a.name.to_string();
                 let op = if a.append { "+=" } else { "=" };
                 s.push_str(op);
                 s.push_str(&escape::quote_if_needed(
                     a.value.to_string().as_str(),
-                    escape::QuoteMode::Quote,
+                    escape::QuoteMode::SingleQuote,
                 ));
                 s.into()
             }
diff --git a/brush-core/src/completion.rs b/brush-core/src/completion.rs
index 030ea66..3b21c5f 100644
--- a/brush-core/src/completion.rs
+++ b/brush-core/src/completion.rs
@@ -604,7 +604,7 @@ impl Spec {
         for arg in args {
             command_line.push(' ');
 
-            let escaped_arg = escape::quote_if_needed(arg, escape::QuoteMode::Quote);
+            let escaped_arg = escape::quote_if_needed(arg, escape::QuoteMode::SingleQuote);
             command_line.push_str(escaped_arg.as_ref());
         }
 
diff --git a/brush-core/src/escape.rs b/brush-core/src/escape.rs
index 7aaf9c5..5fd32d3 100644
--- a/brush-core/src/escape.rs
+++ b/brush-core/src/escape.rs
@@ -167,31 +167,73 @@ pub(crate) fn expand_backslash_escapes(
     Ok((result, true))
 }
 
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Default)]
 pub(crate) enum QuoteMode {
+    #[default]
+    SingleQuote,
+    DoubleQuote,
     BackslashEscape,
-    Quote,
 }
 
-pub(crate) fn force_quote(s: &str, mode: QuoteMode) -> String {
-    match mode {
-        QuoteMode::BackslashEscape => escape_with_backslash(s, true).to_string(),
-        QuoteMode::Quote => escape_with_quoting(s, true).to_string(),
-    }
+/// Options influencing how to escape/quote an input string.
+#[derive(Default)]
+pub(crate) struct QuoteOptions {
+    /// Whether or not to *always* escape or quote the input; if false, then escaping/quoting
+    /// will only be applied if the input contains characters that *require* it.
+    pub always_quote: bool,
+    /// Preferred mode for quoting/escaping. Quoting may be "upgraded" to a more expressive
+    /// format if the input is not expressible otherwise.
+    pub preferred_mode: QuoteMode,
+    /// Whether or not to *avoid* using ANSI C quoting just for the benefit of newline characters.
+    /// Default is for newline characters to require upgrading the string's quoting to
+    /// ANSI C quoting.
+    pub avoid_ansi_c_quoting_newline: bool,
 }
 
-pub(crate) fn quote_if_needed(s: &str, mode: QuoteMode) -> Cow<'_, str> {
-    match mode {
-        QuoteMode::BackslashEscape => escape_with_backslash(s, false),
-        QuoteMode::Quote => escape_with_quoting(s, false),
+pub(crate) fn quote<'a>(s: &'a str, options: &QuoteOptions) -> Cow<'a, str> {
+    let use_ansi_c_quotes = s.contains(|c| {
+        needs_ansi_c_quoting(c) && (!options.avoid_ansi_c_quoting_newline || c != '\n')
+    });
+
+    if use_ansi_c_quotes {
+        return ansi_c_quote(s).into();
     }
-}
 
-fn escape_with_backslash(s: &str, force: bool) -> Cow<'_, str> {
-    if !force && !s.chars().any(needs_escaping) {
+    let use_default_quotes =
+        !use_ansi_c_quotes && (options.always_quote || s.is_empty() || s.contains(needs_escaping));
+
+    if !use_default_quotes {
         return s.into();
     }
 
+    match options.preferred_mode {
+        QuoteMode::BackslashEscape => backslash_escape(s).into(),
+        QuoteMode::SingleQuote => single_quote(s).into(),
+        QuoteMode::DoubleQuote => double_quote(s).into(),
+    }
+}
+
+pub(crate) fn force_quote(s: &str, mode: QuoteMode) -> String {
+    let options = QuoteOptions {
+        always_quote: true,
+        preferred_mode: mode,
+        ..Default::default()
+    };
+
+    quote(s, &options).to_string()
+}
+
+pub(crate) fn quote_if_needed(s: &str, mode: QuoteMode) -> Cow<'_, str> {
+    let options = QuoteOptions {
+        always_quote: false,
+        preferred_mode: mode,
+        ..Default::default()
+    };
+
+    quote(s, &options)
+}
+
+fn backslash_escape(s: &str) -> String {
     let mut output = String::new();
 
     // TODO: Handle other interesting sequences.
@@ -205,18 +247,88 @@ fn escape_with_backslash(s: &str, force: bool) -> Cow<'_, str> {
         }
     }
 
-    output.into()
+    output
+}
+
+fn single_quote(s: &str) -> String {
+    // Special-case the empty string.
+    if s.is_empty() {
+        return "''".into();
+    }
+
+    let mut result = String::new();
+
+    // Go through the string; put everything in single quotes except for
+    // the single quote character itself. It will get escaped outside
+    // all quoting.
+    let mut first = true;
+    for part in s.split('\'') {
+        if !first {
+            result.push('\\');
+            result.push('\'');
+        } else {
+            first = false;
+        }
+
+        if !part.is_empty() {
+            result.push('\'');
+            result.push_str(part);
+            result.push('\'');
+        }
+    }
+
+    result
+}
+
+fn double_quote(s: &str) -> String {
+    let mut result = String::new();
+
+    result.push('"');
+
+    for c in s.chars() {
+        if matches!(c, '$' | '`' | '"' | '\\') {
+            result.push('\\');
+        }
+
+        result.push(c);
+    }
+
+    result.push('"');
+
+    result
 }
 
-fn escape_with_quoting(s: &str, force: bool) -> Cow<'_, str> {
-    // TODO: Handle single-quote!
-    if force || s.is_empty() || s.chars().any(needs_escaping) {
-        std::format!("'{s}'").into()
-    } else {
-        s.into()
+fn ansi_c_quote(s: &str) -> String {
+    let mut result = String::new();
+
+    result.push_str("$'");
+
+    for c in s.chars() {
+        match c {
+            '\x07' => result.push_str("\\a"),
+            '\x08' => result.push_str("\\b"),
+            '\x1b' => result.push_str("\\E"),
+            '\x0c' => result.push_str("\\f"),
+            '\n' => result.push_str("\\n"),
+            '\r' => result.push_str("\\r"),
+            '\t' => result.push_str("\\t"),
+            '\x0b' => result.push_str("\\v"),
+            '\\' => result.push_str("\\\\"),
+            '\'' => result.push_str("\\'"),
+            c if needs_ansi_c_quoting(c) => {
+                result.push_str(std::format!("\\{:03o}", c as u8).as_str());
+            }
+            _ => result.push(c),
+        }
     }
+
+    result.push('\'');
+
+    result
 }
 
+// Returns whether or not the given character needs to be escaped (or quoted) if outside
+// quotes.
 fn needs_escaping(c: char) -> bool {
     matches!(
         c,
@@ -240,9 +352,14 @@ fn needs_escaping(c: char) -> bool {
             | '^'
             | ','
             | ' '
+            | '\''
     )
 }
 
+fn needs_ansi_c_quoting(c: char) -> bool {
+    c.is_ascii_control()
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -255,10 +372,11 @@ mod tests {
     }
 
     #[test]
-    fn test_quote_escape() {
-        assert_eq!(quote_if_needed("a", QuoteMode::Quote), "a");
-        assert_eq!(quote_if_needed("a b", QuoteMode::Quote), "'a b'");
-        assert_eq!(quote_if_needed("", QuoteMode::Quote), "''");
+    fn test_single_quote_escape() {
+        assert_eq!(quote_if_needed("a", QuoteMode::SingleQuote), "a");
+        assert_eq!(quote_if_needed("a b", QuoteMode::SingleQuote), "'a b'");
+        assert_eq!(quote_if_needed("", QuoteMode::SingleQuote), "''");
+        assert_eq!(quote_if_needed("'", QuoteMode::SingleQuote), "\\'");
     }
 
     fn assert_echo_expands_to(unexpanded: &str, expected: &str) {
diff --git a/brush-core/src/expansion.rs b/brush-core/src/expansion.rs
index a24b32f..5a0c71d 100644
--- a/brush-core/src/expansion.rs
+++ b/brush-core/src/expansion.rs
@@ -1030,7 +1030,10 @@ impl<'a> WordExpander<'a> {
                 op,
             } => {
                 let expanded_parameter = self.expand_parameter(&parameter, indirect).await?;
-                transform_expansion(expanded_parameter, |s| self.apply_transform_to(&op, s))
+                let came_from_undefined = expanded_parameter.undefined;
+                transform_expansion(expanded_parameter, |s| {
+                    self.apply_transform_to(&op, s, came_from_undefined)
+                })
             }
             brush_parser::word::ParameterExpr::UppercaseFirstChar {
                 parameter,
@@ -1302,9 +1305,12 @@ impl<'a> WordExpander<'a> {
                     if matches!(var.value(), ShellValue::Unset(_)) {
                         Ok(Expansion::undefined())
                     } else {
-                        Ok(Expansion::from(
-                            var.value().to_cow_str(self.shell).to_string(),
-                        ))
+                        let value = var.value().try_get_cow_str(self.shell);
+                        if let Some(value) = value {
+                            Ok(Expansion::from(value.to_string()))
+                        } else {
+                            Ok(Expansion::undefined())
+                        }
                     }
                 } else {
                     Ok(Expansion::undefined())
@@ -1549,6 +1555,7 @@ impl<'a> WordExpander<'a> {
         &self,
         op: &ParameterTransformOp,
         s: String,
+        came_from_undefined: bool,
     ) -> Result<String, error::Error> {
         match op {
             brush_parser::word::ParameterTransformOp::PromptExpand => {
@@ -1567,12 +1574,26 @@ impl<'a> WordExpander<'a> {
             brush_parser::word::ParameterTransformOp::PossiblyQuoteWithArraysExpanded {
                 separate_words: _separate_words,
             } => {
-                // TODO: This isn't right for arrays.
-                // TODO: This doesn't honor 'separate_words'
-                Ok(variables::quote_str_for_assignment(s.as_str()))
+                if came_from_undefined {
+                    Ok(String::new())
+                } else {
+                    // TODO: This isn't right for arrays.
+                    // TODO: This doesn't honor 'separate_words'
+                    Ok(escape::force_quote(
+                        s.as_str(),
+                        escape::QuoteMode::SingleQuote,
+                    ))
+                }
             }
             brush_parser::word::ParameterTransformOp::Quoted => {
-                Ok(variables::quote_str_for_assignment(s.as_str()))
+                if came_from_undefined {
+                    Ok(String::new())
+                } else {
+                    Ok(escape::force_quote(
+                        s.as_str(),
+                        escape::QuoteMode::SingleQuote,
+                    ))
+                }
             }
             brush_parser::word::ParameterTransformOp::ToLowerCase => Ok(s.to_lowercase()),
             brush_parser::word::ParameterTransformOp::ToUpperCase => Ok(s.to_uppercase()),
diff --git a/brush-core/src/variables.rs b/brush-core/src/variables.rs
index 89eb3f2..5c81af9 100644
--- a/brush-core/src/variables.rs
+++ b/brush-core/src/variables.rs
@@ -609,7 +609,7 @@ impl ShellValueLiteral {
     }
 
     fn fmt_scalar_for_tracing(s: &str, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let processed = escape::quote_if_needed(s, escape::QuoteMode::Quote);
+        let processed = escape::quote_if_needed(s, escape::QuoteMode::SingleQuote);
         write!(f, "{processed}")
     }
 }
@@ -777,25 +777,28 @@ impl ShellValue {
     pub fn format(&self, style: FormatStyle, shell: &Shell) -> Result<Cow<'_, str>, error::Error> {
         match self {
             ShellValue::Unset(_) => Ok("".into()),
-            ShellValue::String(s) => {
-                // TODO: Handle embedded newlines and other special chars.
-                match style {
-                    FormatStyle::Basic => {
-                        if s.contains(' ') {
-                            Ok(format!("'{s}'").into())
-                        } else {
-                            Ok(s.into())
-                        }
-                    }
-                    FormatStyle::DeclarePrint => Ok(format!("\"{s}\"").into()),
+            ShellValue::String(s) => match style {
+                FormatStyle::Basic => Ok(escape::quote_if_needed(
+                    s.as_str(),
+                    escape::QuoteMode::SingleQuote,
+                )),
+                FormatStyle::DeclarePrint => {
+                    Ok(escape::force_quote(s.as_str(), escape::QuoteMode::DoubleQuote).into())
                 }
-            }
+            },
             ShellValue::AssociativeArray(values) => {
                 let mut result = String::new();
                 result.push('(');
 
                 for (key, value) in values {
-                    write!(result, "[{key}]=\"{value}\" ")?;
+                    let formatted_key =
+                        escape::quote_if_needed(key.as_str(), escape::QuoteMode::DoubleQuote);
+                    let formatted_value =
+                        escape::force_quote(value.as_str(), escape::QuoteMode::DoubleQuote);
+
+                    // N.B. We include an unconditional trailing space character (even after the last
+                    // entry in the associative array) to match standard output behavior.
+                    write!(result, "[{formatted_key}]={formatted_value} ")?;
                 }
 
                 result.push(')');
@@ -809,7 +812,10 @@ impl ShellValue {
                     if i > 0 {
                         result.push(' ');
                     }
-                    write!(result, "[{key}]=\"{value}\"")?;
+
+                    let formatted_value =
+                        escape::force_quote(value.as_str(), escape::QuoteMode::DoubleQuote);
+                    write!(result, "[{key}]={formatted_value}")?;
                 }
 
                 result.push(')');
@@ -877,27 +883,37 @@ impl ShellValue {
 
     /// Converts this value to a string.
     pub fn to_cow_str(&self, shell: &Shell) -> Cow<'_, str> {
+        self.try_get_cow_str(shell).unwrap_or(Cow::Borrowed(""))
+    }
+
+    fn to_cow_str_without_dynamic_support(&self) -> Cow<'_, str> {
+        self.try_get_cow_str_without_dynamic_support()
+            .unwrap_or(Cow::Borrowed(""))
+    }
+
+    /// Tries to convert this value to a string; returns `None` if the value is unset
+    /// or otherwise doesn't exist.
+    pub fn try_get_cow_str(&self, shell: &Shell) -> Option<Cow<'_, str>> {
         match self {
             ShellValue::Dynamic { getter, .. } => {
                 let dynamic_value = getter(shell);
-                let result = dynamic_value.to_cow_str(shell).to_string();
-                result.into()
+                dynamic_value
+                    .try_get_cow_str(shell)
+                    .map(|s| s.to_string().into())
             }
-            _ => self.to_cow_str_without_dynamic_support(),
+            _ => self.try_get_cow_str_without_dynamic_support(),
         }
     }
 
-    fn to_cow_str_without_dynamic_support(&self) -> Cow<'_, str> {
+    fn try_get_cow_str_without_dynamic_support(&self) -> Option<Cow<'_, str>> {
         match self {
-            ShellValue::Unset(_) => Cow::Borrowed(""),
-            ShellValue::String(s) => Cow::Borrowed(s.as_str()),
-            ShellValue::AssociativeArray(values) => values
-                .get("0")
-                .map_or_else(|| Cow::Borrowed(""), |s| Cow::Borrowed(s.as_str())),
-            ShellValue::IndexedArray(values) => values
-                .get(&0)
-                .map_or_else(|| Cow::Borrowed(""), |s| Cow::Borrowed(s.as_str())),
-            ShellValue::Dynamic { .. } => "".into(),
+            ShellValue::Unset(_) => None,
+            ShellValue::String(s) => Some(Cow::Borrowed(s.as_str())),
+            ShellValue::AssociativeArray(values) => {
+                values.get("0").map(|s| Cow::Borrowed(s.as_str()))
+            }
+            ShellValue::IndexedArray(values) => values.get(&0).map(|s| Cow::Borrowed(s.as_str())),
+            ShellValue::Dynamic { .. } => None,
         }
     }
 
@@ -909,11 +925,13 @@ impl ShellValue {
     pub fn to_assignable_str(&self, index: Option<&str>, shell: &Shell) -> String {
         match self {
             ShellValue::Unset(_) => String::new(),
-            ShellValue::String(s) => quote_str_for_assignment(s.as_str()),
+            ShellValue::String(s) => {
+                escape::force_quote(s.as_str(), escape::QuoteMode::SingleQuote)
+            }
             ShellValue::AssociativeArray(_) | ShellValue::IndexedArray(_) => {
                 if let Some(index) = index {
                     if let Ok(Some(value)) = self.get_at(index, shell) {
-                        quote_str_for_assignment(value.as_ref())
+                        escape::force_quote(value.as_ref(), escape::QuoteMode::SingleQuote)
                     } else {
                         String::new()
                     }
@@ -957,25 +975,3 @@ impl From<Vec<&str>> for ShellValue {
         ShellValue::indexed_array_from_strs(values.as_slice())
     }
 }
-
-pub(crate) fn quote_str_for_assignment(s: &str) -> String {
-    let mut result = String::new();
-
-    let mut first = true;
-    for part in s.split('\'') {
-        if !first {
-            result.push('\\');
-            result.push('\'');
-        } else {
-            first = false;
-        }
-
-        if !part.is_empty() {
-            result.push('\'');
-            result.push_str(part);
-            result.push('\'');
-        }
-    }
-
-    result
-}


Test patch:
diff --git a/brush-core/src/extendedtests.rs b/brush-core/src/extendedtests.rs
index cc54e2b..224f197 100644
--- a/brush-core/src/extendedtests.rs
+++ b/brush-core/src/extendedtests.rs
@@ -56,7 +56,7 @@ async fn apply_unary_predicate(
     if shell.options.print_commands_and_arguments {
         shell.trace_command(std::format!(
             "[[ {op} {} ]]",
-            escape::quote_if_needed(&expanded_operand, escape::QuoteMode::Quote)
+            escape::quote_if_needed(&expanded_operand, escape::QuoteMode::SingleQuote)
         ))?;
     }
 
diff --git a/brush-shell/tests/cases/builtins/declare.yaml b/brush-shell/tests/cases/builtins/declare.yaml
index d14890a..aad8ec2 100644
--- a/brush-shell/tests/cases/builtins/declare.yaml
+++ b/brush-shell/tests/cases/builtins/declare.yaml
@@ -14,7 +14,7 @@ common_test_files:
       }
 
 cases:
-  - name: "Dump vars"
+  - name: "Display vars"
     stdin: |
       declare myvar=something
       declare -p myvar
@@ -22,6 +22,23 @@ cases:
       myarr=(a b c)
       declare -p myarr
 
+  - name: "Display vars with interesting chars"
+    stdin: |
+      (testvar=$'a\nb' && declare -p testvar && declare | grep testvar=)
+      echo "-------------------------"
+      (testvar="\"abc\"" && declare -p testvar && declare | grep testvar=)
+      echo "-------------------------"
+      (testvar="a b c" && declare -p testvar && declare | grep testvar=)
+      echo "-------------------------"
+      (testvar="'" && declare -p testvar && declare | grep testvar=)
+      echo "-------------------------"
+      (testvar=$'\x03' && declare -p testvar && declare | grep testvar=)
+      echo "-------------------------"
+      (testvar=$'\x08' && declare -p testvar && declare | grep testvar=)
+      echo "-------------------------"
+      (testvar=$(printf '\033[34mabc\033[0m') && declare -p testvar && declare | grep testvar=)
+      echo "-------------------------"
+
   - name: "Declare integer"
     stdin: |
       declare -i num=10
diff --git a/brush-shell/tests/cases/builtins/read.yaml b/brush-shell/tests/cases/builtins/read.yaml
index 42a4e36..8ba5cbc 100644
--- a/brush-shell/tests/cases/builtins/read.yaml
+++ b/brush-shell/tests/cases/builtins/read.yaml
@@ -4,10 +4,10 @@ cases:
     test_files:
       - path: "data.txt"
         contents: |
-          a
-          b
+          a 1
+          b 2
     stdin: |
-      while read name; do echo "Hello, $name"; done < data.txt
+      while read name num; do echo "Hello, $name => $num"; done < data.txt
 
   - name: "Basic read usage from pipe"
     stdin: |
@@ -42,3 +42,41 @@ cases:
       while IFS="" read myvar; do
           echo "myvar1: |${myvar}|"
       done < <(printf "a\tb\nc d\te\n")
+
+  - name: "read with empty entries"
+    stdin: |
+      (echo "x"; echo ""; echo "y"; echo ""; echo "") | while read line; do echo "LINE: '$line'"; done
+      echo 'x,,y,z' | (IFS=',' read READ1 READ2 READ3 READ4; declare -p READ1; declare -p READ2; declare -p READ3; declare -p READ4)
+
+  - name: "read -a with empty lines"
+    stdin: |
+      (echo "hi"; echo ""; echo "there"; echo ""; echo "you") | (read -a READ; declare -p READ)
+      (echo -e "hi\t\tthere\t\tyou") | (read -a READ -d $'\t'; declare -p READ)
+
+  - name: "read -a with empty interior field"
+    stdin: |
+      echo -e -n "hi||there||you" | (IFS='|' read -a READ; declare -p READ)
+      echo -e -n "hi  there  you " | (IFS=' ' read -a READ; declare -p READ)
+      echo -e -n "hi  there  you " | (read -a READ; declare -p READ)
+
+  - name: "read -a with empty leading field"
+    stdin: |
+      echo -e -n "|hi|there|you" | (IFS='|' read -a READ; declare -p READ)
+
+  - name: "read -a with empty trailing field"
+    known_failure: true # Needs further investigation
+    stdin: |
+      echo -e -n "hi|there|you|" | (IFS='|' read -a READ; declare -p READ)
+
+  - name: "read -a with empty entries + empty delimiter"
+    stdin: |
+      (echo "hi"; echo ""; echo "there"; echo ""; echo "you") | (read -a READ -d ''; declare -p READ)
+      echo -e -n "hi\n\nthere\n\nyou\n" | (read -a READ -d ''; declare -p READ)
+
+  - name: "read -a with empty entries + empty delimiter + custom IFS"
+    stdin: |
+      (echo 'x,,y,z'; echo 'w,v'; echo ''; echo ''; echo 'm') | (IFS=',' read -d "" -a READ; declare -p READ)
+
+  - name: "read with empty delimiter"
+    stdin: |
+      echo x | (read -d ""; declare -p REPLY)