Skip to content

Commit b729f4b

Browse files
committed
Address PR #155 review feedback
Fixed the following issues raised in code review: 1. Added validation to reject increment/decrement on literal numbers - Now correctly errors on expressions like `echo $(++2)` - Operators can only be applied to variables or parenthesized expressions 2. Replaced .unwrap() calls with proper error handling - Fixed unwrap() in unary_pre_arithmetic_expr (line 1738) - Fixed unwrap() in unary_post_arithmetic_expr (line 1783) - Used ok_or_else() with descriptive error messages 3. Fixed missing parse_variable_expansion function - Restored function that was accidentally removed during merge - Fixes compilation errors 4. Fixed stray conflict marker in types.rs - Removed leftover "<<<<<<< HEAD" marker All tests pass and manual testing confirms: - Post-increment (a++) returns original value, updates variable - Pre-increment (++a) returns new value, updates variable - Invalid expressions like ++2 correctly produce errors
1 parent 67c3cda commit b729f4b

File tree

2 files changed

+84
-3
lines changed

2 files changed

+84
-3
lines changed

crates/deno_task_shell/src/parser.rs

Lines changed: 84 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1735,7 +1735,10 @@ fn unary_pre_arithmetic_expr(pair: Pair<Rule>) -> Result<ArithmeticPart> {
17351735
let second = inner.next().ok_or_else(|| miette!("Expected operand"))?;
17361736
let operand = match second.as_rule() {
17371737
Rule::parentheses_expr => {
1738-
let inner = second.into_inner().next().unwrap();
1738+
let inner = second
1739+
.into_inner()
1740+
.next()
1741+
.ok_or_else(|| miette!("Expected expression in parentheses"))?;
17391742
let parts = parse_arithmetic_sequence(inner)?;
17401743
Ok(ArithmeticPart::ParenthesesExpr(Box::new(Arithmetic {
17411744
parts,
@@ -1752,6 +1755,14 @@ fn unary_pre_arithmetic_expr(pair: Pair<Rule>) -> Result<ArithmeticPart> {
17521755
match first.as_rule() {
17531756
Rule::pre_arithmetic_op => {
17541757
let op = parse_pre_arithmetic_op(first)?;
1758+
// Validate that increment/decrement are only applied to variables or expressions
1759+
if matches!(op, PreArithmeticOp::Increment | PreArithmeticOp::Decrement) {
1760+
if matches!(operand, ArithmeticPart::Number(_)) {
1761+
return Err(miette!(
1762+
"Increment/decrement operators cannot be applied to literal numbers"
1763+
));
1764+
}
1765+
}
17551766
Ok(ArithmeticPart::UnaryArithmeticExpr {
17561767
operator: UnaryArithmeticOp::Pre(op),
17571768
operand: Box::new(operand),
@@ -1780,7 +1791,10 @@ fn unary_post_arithmetic_expr(pair: Pair<Rule>) -> Result<ArithmeticPart> {
17801791

17811792
let operand = match first.as_rule() {
17821793
Rule::parentheses_expr => {
1783-
let inner = first.into_inner().next().unwrap();
1794+
let inner = first
1795+
.into_inner()
1796+
.next()
1797+
.ok_or_else(|| miette!("Expected expression in parentheses"))?;
17841798
let parts = parse_arithmetic_sequence(inner)?;
17851799
Ok(ArithmeticPart::ParenthesesExpr(Box::new(Arithmetic {
17861800
parts,
@@ -1793,6 +1807,14 @@ fn unary_post_arithmetic_expr(pair: Pair<Rule>) -> Result<ArithmeticPart> {
17931807
first.as_rule()
17941808
)),
17951809
}?;
1810+
1811+
// Validate that increment/decrement are only applied to variables or expressions
1812+
if matches!(operand, ArithmeticPart::Number(_)) {
1813+
return Err(miette!(
1814+
"Increment/decrement operators cannot be applied to literal numbers"
1815+
));
1816+
}
1817+
17961818
let op = parse_post_arithmetic_op(second)?;
17971819
Ok(ArithmeticPart::UnaryArithmeticExpr {
17981820
operator: UnaryArithmeticOp::Post(op),
@@ -1834,6 +1856,66 @@ fn parse_post_arithmetic_op(pair: Pair<Rule>) -> Result<PostArithmeticOp> {
18341856
}
18351857
}
18361858

1859+
fn parse_variable_expansion(part: Pair<Rule>) -> Result<WordPart> {
1860+
let mut inner = part.into_inner();
1861+
let variable = inner
1862+
.next()
1863+
.ok_or_else(|| miette!("Expected variable name"))?;
1864+
let variable_name = variable.as_str().to_string();
1865+
1866+
let modifier = inner.next();
1867+
let parsed_modifier = if let Some(modifier) = modifier {
1868+
match modifier.as_rule() {
1869+
Rule::VAR_SUBSTRING => {
1870+
let mut numbers = modifier.into_inner();
1871+
let begin: Word = if let Some(n) = numbers.next() {
1872+
parse_word(n)?
1873+
} else {
1874+
return Err(miette!(
1875+
"Expected a number for substring begin"
1876+
));
1877+
};
1878+
1879+
let length = if let Some(len_word) = numbers.next() {
1880+
Some(parse_word(len_word)?)
1881+
} else {
1882+
None
1883+
};
1884+
Some(Box::new(VariableModifier::Substring { begin, length }))
1885+
}
1886+
Rule::VAR_DEFAULT_VALUE => {
1887+
let value = if let Some(val) = modifier.into_inner().next() {
1888+
parse_word(val)?
1889+
} else {
1890+
Word::new_empty()
1891+
};
1892+
Some(Box::new(VariableModifier::DefaultValue(value)))
1893+
}
1894+
Rule::VAR_ASSIGN_DEFAULT => {
1895+
let value = modifier.into_inner().next().unwrap();
1896+
Some(Box::new(VariableModifier::AssignDefault(parse_word(
1897+
value,
1898+
)?)))
1899+
}
1900+
Rule::VAR_ALTERNATE_VALUE => {
1901+
let value = modifier.into_inner().next().unwrap();
1902+
Some(Box::new(VariableModifier::AlternateValue(parse_word(
1903+
value,
1904+
)?)))
1905+
}
1906+
_ => {
1907+
return Err(miette!(
1908+
"Unexpected rule in variable expansion modifier: {:?}",
1909+
modifier.as_rule()
1910+
));
1911+
}
1912+
}
1913+
} else {
1914+
None
1915+
};
1916+
Ok(WordPart::Variable(variable_name, parsed_modifier))
1917+
}
1918+
18371919
fn parse_tilde_prefix(pair: Pair<Rule>) -> Result<WordPart> {
18381920
let tilde_prefix_str = pair.as_str();
18391921
let user = if tilde_prefix_str.len() > 1 {

crates/deno_task_shell/src/shell/types.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1122,7 +1122,6 @@ impl ArithmeticResult {
11221122
}
11231123
};
11241124

1125-
<<<<<<< HEAD
11261125
let mut changes = self.changes.clone();
11271126
changes.extend(other.changes.clone());
11281127

0 commit comments

Comments
 (0)