From 218f523e1b813e9f1f8fa239d78690e72524b202 Mon Sep 17 00:00:00 2001
From: Michael Debertol <michael.debertol@gmail.com>
Date: Sun, 23 May 2021 21:58:18 +0200
Subject: [PATCH] expr: make substr infallible

Instead of returning an Err it should return the "null string"
(in our case that's the empty string) when the offset or length
is invalid.
---
 src/uu/expr/src/syntax_tree.rs | 36 ++++++++++++----------------------
 tests/by-util/test_expr.rs     | 29 +++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs
index c81adf0c8..a75f4c742 100644
--- a/src/uu/expr/src/syntax_tree.rs
+++ b/src/uu/expr/src/syntax_tree.rs
@@ -153,7 +153,7 @@ impl AstNode {
                     ":" | "match" => operator_match(&operand_values),
                     "length" => Ok(prefix_operator_length(&operand_values)),
                     "index" => Ok(prefix_operator_index(&operand_values)),
-                    "substr" => prefix_operator_substr(&operand_values),
+                    "substr" => Ok(prefix_operator_substr(&operand_values)),
 
                     _ => Err(format!("operation not implemented: {}", op_type)),
                 },
@@ -522,35 +522,23 @@ fn prefix_operator_index(values: &[String]) -> String {
     "0".to_string()
 }
 
-fn prefix_operator_substr(values: &[String]) -> Result<String, String> {
+fn prefix_operator_substr(values: &[String]) -> String {
     assert!(values.len() == 3);
     let subj = &values[0];
-    let mut idx = match values[1].parse::<i64>() {
-        Ok(i) => i,
-        Err(_) => return Err("expected integer as POS arg to 'substr'".to_string()),
+    let idx = match values[1]
+        .parse::<usize>()
+        .ok()
+        .and_then(|v| v.checked_sub(1))
+    {
+        Some(i) => i,
+        None => return String::new(),
     };
-    let mut len = match values[2].parse::<i64>() {
+    let len = match values[2].parse::<usize>() {
         Ok(i) => i,
-        Err(_) => return Err("expected integer as LENGTH arg to 'substr'".to_string()),
+        Err(_) => return String::new(),
     };
 
-    if idx <= 0 || len <= 0 {
-        return Ok("".to_string());
-    }
-
-    let mut out_str = String::new();
-    for ch in subj.chars() {
-        idx -= 1;
-        if idx <= 0 {
-            if len <= 0 {
-                break;
-            }
-            len -= 1;
-
-            out_str.push(ch);
-        }
-    }
-    Ok(out_str)
+    subj.chars().skip(idx).take(len).collect()
 }
 
 fn bool_as_int(b: bool) -> i64 {
diff --git a/tests/by-util/test_expr.rs b/tests/by-util/test_expr.rs
index bb0760676..6a969b5e9 100644
--- a/tests/by-util/test_expr.rs
+++ b/tests/by-util/test_expr.rs
@@ -54,3 +54,32 @@ fn test_and() {
 
     new_ucmd!().args(&["", "&", "1"]).run().stdout_is("0\n");
 }
+
+#[test]
+fn test_substr() {
+    new_ucmd!()
+        .args(&["substr", "abc", "1", "1"])
+        .succeeds()
+        .stdout_only("a\n");
+}
+
+#[test]
+fn test_invalid_substr() {
+    new_ucmd!()
+        .args(&["substr", "abc", "0", "1"])
+        .fails()
+        .status_code(1)
+        .stdout_only("\n");
+
+    new_ucmd!()
+        .args(&["substr", "abc", &(std::usize::MAX.to_string() + "0"), "1"])
+        .fails()
+        .status_code(1)
+        .stdout_only("\n");
+
+    new_ucmd!()
+        .args(&["substr", "abc", "0", &(std::usize::MAX.to_string() + "0")])
+        .fails()
+        .status_code(1)
+        .stdout_only("\n");
+}