Added support for arithmetic exprs and negative values in substring e…

…xpansion

crates/deno_task_shell/src/grammar.pest

@@ -41,7 +41,7 @@ QUOTED_PENDING_WORD = ${ (
 )* }
 
 PARAMETER_PENDING_WORD = ${ 
-    TILDE_PREFIX  ~ ( !"}" ~ (
+    TILDE_PREFIX  ~ ( !"}" ~ !":" ~ (
         EXIT_STATUS | 
         PARAMETER_ESCAPE_CHAR | 
         "$" ~ ARITHMETIC_EXPRESSION |
@@ -50,7 +50,7 @@ PARAMETER_PENDING_WORD = ${
         QUOTED_WORD | 
         QUOTED_CHAR
     ))* | 
-    ( !"}" ~ (
+    ( !"}" ~ !":" ~ (
         EXIT_STATUS | 
         PARAMETER_ESCAPE_CHAR | 
         "$" ~ ARITHMETIC_EXPRESSION |
@@ -100,10 +100,10 @@ VARIABLE_MODIFIER = _{
     VAR_SUBSTRING
 }
 
-VAR_DEFAULT_VALUE = { ":-" ~ PARAMETER_PENDING_WORD? }
-VAR_ASSIGN_DEFAULT = { ":=" ~ PARAMETER_PENDING_WORD }
-VAR_ALTERNATE_VALUE = { ":+" ~ PARAMETER_PENDING_WORD }
-VAR_SUBSTRING = { ":" ~ NUMBER ~ (":" ~ NUMBER)? } 
+VAR_DEFAULT_VALUE = !{ ":-" ~ PARAMETER_PENDING_WORD? }
+VAR_ASSIGN_DEFAULT = !{ ":=" ~ PARAMETER_PENDING_WORD }
+VAR_ALTERNATE_VALUE = !{ ":+" ~ PARAMETER_PENDING_WORD }
+VAR_SUBSTRING = !{ ":" ~ PARAMETER_PENDING_WORD ~ (":" ~ PARAMETER_PENDING_WORD)? } 
 
 TILDE_PREFIX = ${
     "~" ~ (!(OPERATOR | WHITESPACE | NEWLINE | "/") ~ (

crates/deno_task_shell/src/parser.rs

@@ -1,7 +1,7 @@
 // Copyright 2018-2024 the Deno authors. MIT license.
 
 use lazy_static::lazy_static;
-use miette::{miette, Context, IntoDiagnostic, Result};
+use miette::{miette, Context, Result};
 use pest::iterators::Pair;
 use pest::pratt_parser::{Assoc, Op, PrattParser};
 use pest::Parser;
@@ -375,8 +375,8 @@ impl Word {
 pub enum VariableModifier {
   #[error("Invalid substring")]
   Substring {
-    begin: i64,
-    length: Option<i64>,
+    begin: Word,
+    length: Option<Word>,
   },
   DefaultValue(Word),
   AssignDefault(Word),
@@ -1556,13 +1556,14 @@ fn parse_variable_expansion(part: Pair<Rule>) -> Result<WordPart> {
     match modifier.as_rule() {
       Rule::VAR_SUBSTRING => {
         let mut numbers = modifier.into_inner();
-        let begin = numbers
-          .next()
-          .and_then(|n| n.as_str().parse::<i64>().ok())
-          .unwrap_or(0);
+        let begin: Word = if let Some(n) = numbers.next() {
+          parse_word(n)?
+        } else {
+          return Err(miette!("Expected a number for substring begin"));
+        };
 
         let length = if let Some(len_word) = numbers.next() {
-          Some(len_word.as_str().parse::<i64>().into_diagnostic()?)
+          Some(parse_word(len_word)?)
         } else {
           None
         };

crates/deno_task_shell/src/shell/execute.rs

@@ -1245,23 +1245,60 @@ impl VariableModifier {
       VariableModifier::Substring { begin, length } => {
         if let Some(val) = state.get_var(name) {
           let chars: Vec<char> = val.chars().collect();
-          let start = usize::try_from(*begin).into_diagnostic()?;
+
+          let mut changes = Vec::new();
+
+          // TODO figure out a way to get rid of cloning stdin and stderr
+          let begin =
+            evaluate_word(begin.clone(), state, stdin.clone(), stderr.clone())
+              .await
+              .into_diagnostic()
+              .and_then(|v| {
+                changes.extend(v.clone().changes); // TODO figure out a way to get rid of cloning here
+                v.to_integer().map_err(|e| {
+                  miette::miette!("Failed to parse start index: {:?}", e)
+                })
+              })?;
+
+          let start = if begin < 0 {
+            chars
+              .len()
+              .saturating_sub(usize::try_from(-begin).into_diagnostic()?)
+          } else {
+            usize::try_from(begin).into_diagnostic()?
+          };
           let end = match length {
             Some(len) => {
-              if *len < 0 {
-                let len = usize::try_from(-len).into_diagnostic()?;
-                if chars.len().saturating_sub(len) < start {
-                  return Err(miette::miette!("Invalid length: resulting end index is less than start index"));
-                }
-                chars.len().saturating_sub(len)
+              let len = evaluate_word(len.clone(), state, stdin, stderr)
+                .await
+                .into_diagnostic()
+                .and_then(|v| {
+                  changes.extend(v.clone().changes); // TODO figure out a way to get rid of cloning here
+                  v.to_integer().map_err(|e| {
+                    miette::miette!("Failed to parse start index: {:?}", e)
+                  })
+                })?;
+
+              if len < 0 {
+                chars
+                  .len()
+                  .saturating_sub(usize::try_from(-len).into_diagnostic()?)
               } else {
-                let len = usize::try_from(*len).into_diagnostic()?;
+                let len = usize::try_from(len).into_diagnostic()?;
                 start.saturating_add(len).min(chars.len())
               }
             }
             None => chars.len(),
           };
-          Ok((chars[start..end].iter().collect(), None))
+          if start > end {
+            Err(miette::miette!(
+              "Invalid substring range: {}..{}",
+              start,
+              end
+            ))
+          } else {
+            Ok((chars[start..end].iter().collect(), Some(changes)))
+          }
         } else {
           Err(miette::miette!("Undefined variable: {}", name))
         }

crates/deno_task_shell/src/shell/types.rs

@@ -1173,6 +1173,13 @@ impl WordResult {
     self.value.push_str(&other.value);
     self.changes.extend(other.changes);
   }
+
+  pub fn to_integer(&self) -> Result<i64, Error> {
+    self
+      .value
+      .parse::<i64>()
+      .map_err(|_| miette::miette!("Invalid integer: {}", self.value))
+  }
 }
 
 impl PartialEq for WordResult {

crates/tests/src/lib.rs

@@ -2,6 +2,7 @@
 
 #[cfg(test)]
 mod test_builder;
+
 #[cfg(test)]
 use deno_task_shell::ExecuteResult;
 #[cfg(test)]
@@ -1165,6 +1166,30 @@ async fn variable_expansion() {
         .assert_stdout("5\n")
         .run()
         .await;
+
+    TestBuilder::new()
+        .command("FOO=12345 && echo ${FOO:2:$((2+2))}")
+        .assert_stdout("345\n")
+        .run()
+        .await;
+
+    TestBuilder::new()
+        .command("FOO=12345 && echo ${FOO: -2:-1}")
+        .assert_stdout("4\n")
+        .run()
+        .await;
+
+    TestBuilder::new()
+        .command("FOO=12345 && echo ${FOO: -2}")
+        .assert_stdout("45\n")
+        .run()
+        .await;
+
+    TestBuilder::new()
+        .command("FOO=12345 && echo ${FOO: -4: 2}")
+        .assert_stdout("23\n")
+        .run()
+        .await;
 }
 
 #[cfg(test)]