Repository: reubeno/brush
Pull request #468 (2025-05-06 06:25:27)

fix(expansion): allow negative subscripts with indexed arrays + slices
Enables expansion of expressions like `${array[-1]}` and `${array:-1}`. Notably, the latter syntax doesn't yield similar behavior with strings.

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   17.83 μs` | `   18.35 μs` | `    0.52 μs` | `🟠 +2.92%    ` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.88 μs` | `    1.86 μs` | `   -0.02 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   22.49 μs` | `   22.67 μs` | `    0.18 μs` | `⚪  Unchanged` |
| `function_call                       ` | `    2.33 μs` | `    2.38 μs` | `    0.05 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   57.31 μs` | `   57.74 μs` | `    0.42 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `22511.72 μs` | `24057.18 μs` | ` 1545.46 μs` | `🟠 +6.87%    ` |
| `parse_bash_completion               ` | ` 1651.60 μs` | ` 1659.10 μs` | `    7.51 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.76 μs` | `    1.75 μs` | `   -0.01 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   15.27 μs` | `   15.09 μs` | `   -0.18 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2328.83 μs` | ` 2215.43 μs` | ` -113.40 μs` | `🟢 -4.87%    ` |
| `tokenize_sample_script              ` | `    2.89 μs` | `    2.86 μs` | `   -0.03 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/expansion.rs | 🟢 96.18% | 🟢 95.9% | 🔴 -0.28% |
| brush-core/src/variables.rs | 🟢 87.41% | 🟢 87.64% | 🟢 0.23% |
| **Overall Coverage** | **🟢 78.58%** | **🟢 78.58%** | **⚪ 0%** |

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

# Test Summary: bash-completion test suite
| Outcome            | Count                   | Percentage |
| ------------------ | ----------------------: | ---------: |
| ✅ Pass            | 1294            | <span style='color:green'>61.36</span> |
| ❗️ Error           | 248           | <span style='color:red'>11.76</span> |
| ❌ Fail            | 181            | <span style='color:red'>8.58</span> |
| ⏩ Skip            | 371            | 17.59 |
| ❎ Expected Fail   | 14   | 0.66 |
| ✔️ Unexpected Pass | 1 | <span style='color:red'>0.05</span> |
| 📊 Total           | 2109           | 100.00 |

<!-- Sticky Pull Request Comment -->
github-actions[bot]: ## Test Results
    3 files     14 suites   3m 50s ⏱️
  733 tests   733 ✅ 0 💤 0 ❌
2 185 runs  2 185 ✅ 0 💤 0 ❌

Results for commit 342512c8.

[test-results]:data:application/gzip;base64,H4sIAL2uGWgC/12Myw6DIBQFf8Ww7gK4UKQ/Y8gVElKVhsfK9N+LRFvtcubkzEqcn2wijw5uHUnF5wZMVBpLNNmHpTIHWkXd8rYqgIOGVBD/1dO/qvo+Bmf8dBE2xhB3E8uyNTnr5U5H86J+zcanZONzEcM8+1yBgOCScezvwimDBiTlXCBV6DRoI7QBGLVVQN4fWJAGXAgBAAA=


Patch:
diff --git a/brush-core/src/error.rs b/brush-core/src/error.rs
index 8571706..38a1c20 100644
--- a/brush-core/src/error.rs
+++ b/brush-core/src/error.rs
@@ -192,6 +192,10 @@ pub enum Error {
     /// System time error.
     #[error("system time error: {0}")]
     TimeError(#[from] std::time::SystemTimeError),
+
+    /// Array index out of range.
+    #[error("array index out of range")]
+    ArrayIndexOutOfRange,
 }
 
 /// Convenience function for returning an error for unimplemented functionality.
diff --git a/brush-core/src/expansion.rs b/brush-core/src/expansion.rs
index a0e5975..5e36db8 100644
--- a/brush-core/src/expansion.rs
+++ b/brush-core/src/expansion.rs
@@ -937,34 +937,47 @@ impl<'a> WordExpander<'a> {
                     );
                 }
 
-                let expanded_offset = offset.eval(self.shell, self.params, false).await?;
-                let expanded_offset = if expanded_offset < 0 {
-                    0
-                } else {
-                    usize::try_from(expanded_offset)?
-                };
+                #[allow(clippy::cast_possible_wrap)]
+                let expanded_parameter_len = expanded_parameter.polymorphic_len() as i64;
+
+                let mut expanded_offset = offset.eval(self.shell, self.params, false).await?;
+                if expanded_offset < 0 {
+                    // For arrays--and only arrays--we handle negative indexes as offsets from the end
+                    // of the array, with -1 referencing the last element of the array.
+                    if expanded_parameter.from_array {
+                        expanded_offset += expanded_parameter_len;
+
+                        // If the offset is still negative, then we need to yield an empty slice.
+                        // We force the offset to the end of the array.
+                        if expanded_offset < 0 {
+                            expanded_offset = expanded_parameter_len;
+                        }
+                    } else {
+                        // For other values, we just treat negative indexes as 0.
+                        expanded_offset = 0;
+                    }
+                }
 
-                let expanded_parameter_len = expanded_parameter.polymorphic_len();
+                // Make sure the offset is within the bounds of the array.
                 let expanded_offset = min(expanded_offset, expanded_parameter_len);
 
                 let end_offset = if let Some(length) = length {
                     let mut expanded_length = length.eval(self.shell, self.params, false).await?;
                     if expanded_length < 0 {
-                        let param_length: i64 = i64::try_from(expanded_parameter_len)?;
-                        expanded_length += param_length;
+                        expanded_length += expanded_parameter_len;
                     }
 
-                    let expanded_length = std::cmp::min(
-                        usize::try_from(expanded_length)?,
-                        expanded_parameter_len - expanded_offset,
-                    );
+                    let expanded_length =
+                        min(expanded_length, expanded_parameter_len - expanded_offset);
 
                     expanded_offset + expanded_length
                 } else {
                     expanded_parameter_len
                 };
 
-                Ok(expanded_parameter.polymorphic_subslice(expanded_offset, end_offset))
+                #[allow(clippy::cast_sign_loss)]
+                Ok(expanded_parameter
+                    .polymorphic_subslice(expanded_offset as usize, end_offset as usize))
             }
             brush_parser::word::ParameterExpr::Transform {
                 parameter,
@@ -1335,7 +1348,7 @@ impl<'a> WordExpander<'a> {
 
                 // Index into the array.
                 if let Some((_, var)) = self.shell.env.get(name) {
-                    if let Some(value) = var.value().get_at(index_to_use.as_str(), self.shell)? {
+                    if let Ok(Some(value)) = var.value().get_at(index_to_use.as_str(), self.shell) {
                         Ok(Expansion::from(value.to_string()))
                     } else {
                         Ok(Expansion::undefined())
diff --git a/brush-core/src/variables.rs b/brush-core/src/variables.rs
index 11e2a37..1ca3528 100644
--- a/brush-core/src/variables.rs
+++ b/brush-core/src/variables.rs
@@ -855,8 +855,22 @@ impl ShellValue {
                 Ok(values.get(index).map(|s| Cow::Borrowed(s.as_str())))
             }
             ShellValue::IndexedArray(values) => {
-                let key = index.parse::<u64>().unwrap_or(0);
-                Ok(values.get(&key).map(|s| Cow::Borrowed(s.as_str())))
+                let mut index_value = index.parse::<i64>().unwrap_or(0);
+
+                #[allow(clippy::cast_possible_wrap)]
+                if index_value < 0 {
+                    index_value += values.len() as i64;
+                    if index_value < 0 {
+                        return Err(error::Error::ArrayIndexOutOfRange);
+                    }
+                }
+
+                // Now that we've confirmed that the index is non-negative, we can safely convert it
+                // to a u64 without any fuss.
+                #[allow(clippy::cast_sign_loss)]
+                let index_value = index_value as u64;
+
+                Ok(values.get(&index_value).map(|s| Cow::Borrowed(s.as_str())))
             }
             ShellValue::Dynamic { getter, .. } => {
                 let dynamic_value = getter(shell);


Test patch:
diff --git a/brush-shell/tests/cases/arrays.yaml b/brush-shell/tests/cases/arrays.yaml
index 61536dd..5f62c2d 100644
--- a/brush-shell/tests/cases/arrays.yaml
+++ b/brush-shell/tests/cases/arrays.yaml
@@ -27,6 +27,19 @@ cases:
       echo "var[*]: ${var[*]}"
       declare -p var
 
+  - name: "Negative indexed array access"
+    stdin: |
+      var=(a b c)
+      echo "var[-1]: ${var[-1]}"
+      echo "var[-2]: ${var[-2]}"
+      echo "var[-3]: ${var[-3]}"
+
+  - name: "Too large negative indexed array access"
+    ignore_stderr: true
+    stdin: |
+      var=(a b c)
+      echo "var[-4]: ${var[-4]}"
+
   - name: "Define array with expansion"
     stdin: |
       body='a b'
diff --git a/brush-shell/tests/cases/word_expansion.yaml b/brush-shell/tests/cases/word_expansion.yaml
index 5e5361d..998f8a8 100644
--- a/brush-shell/tests/cases/word_expansion.yaml
+++ b/brush-shell/tests/cases/word_expansion.yaml
@@ -739,6 +739,16 @@ cases:
       echo "\${myarray[@]:2:2}: ${myarray[@]:2:2}"
       echo "\${myarray[@]:2}: ${myarray[@]:2}"
 
+  - name: "Substring operator on arrays with negative index"
+    stdin: |
+      set abcde fghij klmno pqrst uvwxy z
+      echo "\${@: -2:2}: ${@: -2:2}"
+      echo "\${@: -2}: ${@: -2}"
+
+      myarray=(abcde fghij klmno pqrst uvwxy z)
+      echo "\${myarray[@]: -2:2}: ${myarray[@]: -2:2}"
+      echo "\${myarray[@]: -2}: ${myarray[@]: -2}"
+
   - name: "Substring operator on single-element array"
     stdin: |
       myarray=("abcde fghij klmno pqrst uvwxy z")