Repository: reubeno/brush
Pull request #357 (2025-01-23 23:33:57)

fix(variables): more correct handling of integer variables
Adds a handful of test cases and fixes for array and scalar integer variables (i.e., variables with the `i` attribute on them).

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

Results for commit 2ea184e0.

[test-results]:data:application/gzip;base64,H4sIAKrUkmcC/13MSw7CIABF0a00jB2UP7gZAxQSYlsMn5Fx7wIRbR2e+5L3BM6vNoHrhC4TSMXnDlmxlKiyD3sjrq5LbhsjaOiWijH/6e4fNc3f4JRfT8HGGOKnxLK3T4j40Pg8pd9n9+Gy+/howrb5XAGQVVAQOwtqOdVCL5pBATGVjhDEMRVSaKsYBK83ZsoPpAYBAAA=

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   16.41 μs` | `   16.37 μs` | `   -0.05 μs` | `⚪  Unchanged` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.56 μs` | `    1.58 μs` | `    0.01 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   34.89 μs` | `   34.16 μs` | `   -0.73 μs` | `🟢 -2.10%    ` |
| `function_call                       ` | `    3.34 μs` | `    3.39 μs` | `    0.05 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   48.73 μs` | `   49.41 μs` | `    0.68 μs` | `🟠 +1.39%    ` |
| `instantiate_shell_with_init_scripts ` | `22144.20 μs` | `22399.96 μs` | `  255.76 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 1655.73 μs` | ` 1657.77 μs` | `    2.04 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.74 μs` | `    1.74 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   16.15 μs` | `   16.17 μs` | `    0.02 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2460.05 μs` | ` 2125.87 μs` | ` -334.18 μs` | `🟢 -13.58%   ` |
| `tokenize_sample_script              ` | `    3.10 μs` | `    2.86 μs` | `   -0.24 μs` | `🟢 -7.77%    ` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/declare.rs | 🟢 81.94% | 🟢 82.5% | 🟢 0.56% |
| brush-core/src/variables.rs | 🟢 80.9% | 🟢 82.3% | 🟢 1.4% |
| **Overall Coverage** | **🟢 75.1%** | **🟢 75.16%** | **🟢 0.06%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `75.16%`
<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-core/src/variables.rs b/brush-core/src/variables.rs
index be9bfb0..3f4df71 100644
--- a/brush-core/src/variables.rs
+++ b/brush-core/src/variables.rs
@@ -213,6 +213,8 @@ impl ShellVariable {
             return Err(error::Error::ReadonlyVariable);
         }
 
+        let value = self.convert_value_literal_for_assignment(value);
+
         if append {
             match (&self.value, &value) {
                 // If we're appending an array to a declared-but-unset variable (or appending
@@ -361,6 +363,7 @@ impl ShellVariable {
         }
 
         let treat_as_int = self.is_treated_as_integer();
+        let value = self.convert_value_str_for_assignment(value);
 
         match &mut self.value {
             ShellValue::IndexedArray(arr) => {
@@ -415,6 +418,29 @@ impl ShellVariable {
         }
     }
 
+    fn convert_value_literal_for_assignment(&self, value: ShellValueLiteral) -> ShellValueLiteral {
+        match value {
+            ShellValueLiteral::Scalar(s) => {
+                ShellValueLiteral::Scalar(self.convert_value_str_for_assignment(s))
+            }
+            ShellValueLiteral::Array(literals) => ShellValueLiteral::Array(ArrayLiteral(
+                literals
+                    .0
+                    .into_iter()
+                    .map(|(k, v)| (k, self.convert_value_str_for_assignment(v)))
+                    .collect(),
+            )),
+        }
+    }
+
+    fn convert_value_str_for_assignment(&self, s: String) -> String {
+        if self.is_treated_as_integer() {
+            s.parse::<i64>().unwrap_or(0).to_string()
+        } else {
+            s
+        }
+    }
+
     /// Tries to unset the value stored at the given index in the variable. Returns
     /// whether or not a value was unset.
     ///


Test patch:
diff --git a/brush-shell/tests/cases/arrays.yaml b/brush-shell/tests/cases/arrays.yaml
index 2ab9da1..e503438 100644
--- a/brush-shell/tests/cases/arrays.yaml
+++ b/brush-shell/tests/cases/arrays.yaml
@@ -125,6 +125,12 @@ cases:
       echo "var[*]: ${var[*]}"
       declare -p var
 
+  - name: "Appending items to int indexed array"
+    stdin: |
+      declare -ai var=(1 2 3)
+      var+=(4 value 6)
+      declare -p var
+
   - name: "Appending array to associative array"
     stdin: |
       source helpers.sh
@@ -160,6 +166,13 @@ cases:
       assoc_array[b]+=2
       stable_print_assoc_array assoc_array
 
+  - name: "Appending items to int associative array"
+    stdin: |
+      source helpers.sh
+      declare -Ai assoc_array=([a]=1 [b]=2 [c]=3)
+      assoc_array+=([d]=4 [e]=value [f]=6)
+      stable_print_assoc_array assoc_array
+
   - name: "Appending array to item"
     stdin: |
       var=x
diff --git a/brush-shell/tests/cases/builtins/declare.yaml b/brush-shell/tests/cases/builtins/declare.yaml
index 71ce456..d14890a 100644
--- a/brush-shell/tests/cases/builtins/declare.yaml
+++ b/brush-shell/tests/cases/builtins/declare.yaml
@@ -38,6 +38,35 @@ cases:
       num+=10
       echo $num
 
+  - name: "Declare integer with non-integer string"
+    stdin: |
+      declare -i var=value
+      declare -p var
+
+  - name: "Update integer with non-integer string"
+    stdin: |
+      declare -i var=10
+      declare -p var
+
+      var=value
+      declare -p var
+
+  - name: "Update integer array with non-integer string"
+    stdin: |
+      declare -ai arr=()
+      declare -p arr
+
+      arr[0]="value"
+      declare -p arr
+
+  - name: "Update integer array with non-integer string"
+    stdin: |
+      declare -Ai arr=()
+      declare -p arr
+
+      arr['key']="value"
+      declare -p arr
+
   - name: "Declare readonly variable"
     ignore_stderr: true
     stdin: |
@@ -206,6 +235,13 @@ cases:
       declare var="changed"
       declare -p var
 
+  - name: "Updating value attributes"
+    stdin: |
+      declare -ix var=10
+      declare -p var
+      declare +ix var
+      declare -p var
+
   - name: "Updating array"
     stdin: |
       declare arr=(a b c)
diff --git a/brush-shell/tests/cases/variables.yaml b/brush-shell/tests/cases/variables.yaml
index b649c4a..6e6cb33 100644
--- a/brush-shell/tests/cases/variables.yaml
+++ b/brush-shell/tests/cases/variables.yaml
@@ -6,6 +6,21 @@ cases:
       x+=here
       echo "x: ${x}"
 
+  - name: "Appending an integer to an integer variable"
+    stdin: |
+      declare -i x=10
+      echo "x: ${x}"
+      x+=5
+      echo "x: ${x}"
+      x+=value
+      echo "x: ${x}"
+
+      y=value
+      declare -i y
+      echo "y: ${y}"
+      y+=5
+      echo "y: ${y}"
+
   - name: "Append to an unset variable"
     stdin: |
       declare -a myvar