Repository: seaofvoices/darklua (rust)

Issue:
Fix bug with remove_nil_declaration rule
Noticed a bug with the `remove_nil_declaration` where assignments with expressions that may expand to multiple values need to be handled specifically.

For example:

```lua
local a, b, c = call(), nil, nil
```

Should convert into

```lua
local a, b, c = call(), nil
-- or
local a, b, c = (call())
```

Pull request:
Fix remove nil declaration rule
Closes #80 

- [x] add entry to the changelog


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 69459ea..74bee22 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+* fix `remove_nil_declaration` ([#84](https://github.com/seaofvoices/darklua/pull/84))
 * upgrade CLI library (mostly changes the help messages format) ([#83](https://github.com/seaofvoices/darklua/pull/83))
 * add rule to remove compount assignments ([#78](https://github.com/seaofvoices/darklua/pull/78))
 * enhance `remove_unused_if_branch` to process if expressions ([#77](https://github.com/seaofvoices/darklua/pull/77))
diff --git a/site/content/rules/remove_nil_declaration.md b/site/content/rules/remove_nil_declaration.md
index ff5f4ef..0d404f5 100644
--- a/site/content/rules/remove_nil_declaration.md
+++ b/site/content/rules/remove_nil_declaration.md
@@ -5,6 +5,8 @@ parameters: []
 examples:
   - content: "local var = nil"
   - content: "local a, b, c = 1, nil, nil"
+  - content: "local a, b = nil, call()"
+  - content: "local var = call(), otherValue, true"
 ---
 
-This rule removes trailing `nil` values in local assignments.
+This rule removes trailing `nil` values in local assignments. Additionally, it will trim unnecessary expressions in assignments when they do not cause any side-effects.
diff --git a/src/nodes/statements/local_assign.rs b/src/nodes/statements/local_assign.rs
index c17c27d..e12816f 100644
--- a/src/nodes/statements/local_assign.rs
+++ b/src/nodes/statements/local_assign.rs
@@ -132,6 +132,16 @@ impl LocalAssignStatement {
         self.values.iter()
     }
 
+    #[inline]
+    pub fn push_variable(&mut self, variable: impl Into<Identifier>) {
+        self.variables.push(variable.into());
+    }
+
+    #[inline]
+    pub fn push_value(&mut self, value: impl Into<Expression>) {
+        self.values.push(value.into());
+    }
+
     #[inline]
     pub fn append_values(&mut self, values: &mut Vec<Expression>) {
         self.values.append(values);
@@ -142,8 +152,8 @@ impl LocalAssignStatement {
         self.values.last()
     }
 
-    pub fn pop_value(&mut self) {
-        self.values.pop();
+    pub fn pop_value(&mut self) -> Option<Expression> {
+        let value = self.values.pop();
         if let Some(tokens) = &mut self.tokens {
             let length = self.values.len();
             if length == 0 {
@@ -157,6 +167,44 @@ impl LocalAssignStatement {
                 tokens.value_commas.truncate(length.saturating_sub(1));
             }
         }
+        value
+    }
+
+    pub fn remove_value(&mut self, index: usize) -> Option<Expression> {
+        if index < self.values.len() {
+            let value = self.values.remove(index);
+
+            if let Some(tokens) = &mut self.tokens {
+                if index < tokens.value_commas.len() {
+                    tokens.value_commas.remove(index);
+                }
+                if self.values.is_empty() && tokens.equal.is_some() {
+                    tokens.equal = None;
+                }
+            }
+
+            Some(value)
+        } else {
+            None
+        }
+    }
+
+    pub fn remove_variable(&mut self, index: usize) -> Option<Identifier> {
+        let len = self.variables.len();
+
+        if len > 1 && index < len {
+            let variable = self.variables.remove(index);
+
+            if let Some(tokens) = &mut self.tokens {
+                if index < tokens.variable_commas.len() {
+                    tokens.variable_commas.remove(index);
+                }
+            }
+
+            Some(variable)
+        } else {
+            None
+        }
     }
 
     #[inline]
@@ -197,75 +245,36 @@ impl LocalAssignStatement {
 mod test {
     use super::*;
 
-    #[test]
-    fn pop_value_removes_the_equal_sign() {
-        let mut assign = LocalAssignStatement::from_variable("var")
-            .with_value(true)
-            .with_tokens(LocalAssignTokens {
-                local: Token::from_content("local"),
-                equal: Some(Token::from_content("=")),
-                variable_commas: Vec::new(),
-                value_commas: Vec::new(),
-            });
-
-        assign.pop_value();
-
-        pretty_assertions::assert_eq!(
-            assign,
-            LocalAssignStatement::from_variable("var").with_tokens(LocalAssignTokens {
-                local: Token::from_content("local"),
-                equal: None,
-                variable_commas: Vec::new(),
-                value_commas: Vec::new(),
-            })
-        );
-    }
-
-    #[test]
-    fn pop_value_removes_the_last_comma_token() {
-        let mut assign = LocalAssignStatement::from_variable("var")
-            .with_value(true)
-            .with_value(false)
-            .with_tokens(LocalAssignTokens {
-                local: Token::from_content("local"),
-                equal: Some(Token::from_content("=")),
-                variable_commas: Vec::new(),
-                value_commas: vec![Token::from_content(",")],
-            });
-
-        assign.pop_value();
-
-        pretty_assertions::assert_eq!(
-            assign,
-            LocalAssignStatement::from_variable("var")
+    mod pop_value {
+        use super::*;
+
+        #[test]
+        fn removes_the_equal_sign() {
+            let mut assign = LocalAssignStatement::from_variable("var")
                 .with_value(true)
                 .with_tokens(LocalAssignTokens {
                     local: Token::from_content("local"),
                     equal: Some(Token::from_content("=")),
                     variable_commas: Vec::new(),
                     value_commas: Vec::new(),
+                });
+
+            assign.pop_value();
+
+            pretty_assertions::assert_eq!(
+                assign,
+                LocalAssignStatement::from_variable("var").with_tokens(LocalAssignTokens {
+                    local: Token::from_content("local"),
+                    equal: None,
+                    variable_commas: Vec::new(),
+                    value_commas: Vec::new(),
                 })
-        );
-    }
-
-    #[test]
-    fn pop_value_removes_one_comma_token() {
-        let mut assign = LocalAssignStatement::from_variable("var")
-            .with_value(true)
-            .with_value(false)
-            .with_value(true)
-            .with_tokens(LocalAssignTokens {
-                local: Token::from_content("local"),
-                equal: Some(Token::from_content("=")),
-                variable_commas: Vec::new(),
-                value_commas: vec![Token::from_content(","), Token::from_content(",")],
-            });
-
-        assign.pop_value();
-
-        pretty_assertions::assert_eq!(
-            assign,
-            LocalAssignStatement::from_variable("var")
+            );
+        }
+
+        #[test]
+        fn removes_the_last_comma_token() {
+            let mut assign = LocalAssignStatement::from_variable("var")
                 .with_value(true)
                 .with_value(false)
                 .with_tokens(LocalAssignTokens {
@@ -273,7 +282,110 @@ mod test {
                     equal: Some(Token::from_content("=")),
                     variable_commas: Vec::new(),
                     value_commas: vec![Token::from_content(",")],
-                })
-        );
+                });
+
+            assign.pop_value();
+
+            pretty_assertions::assert_eq!(
+                assign,
+                LocalAssignStatement::from_variable("var")
+                    .with_value(true)
+                    .with_tokens(LocalAssignTokens {
+                        local: Token::from_content("local"),
+                        equal: Some(Token::from_content("=")),
+                        variable_commas: Vec::new(),
+                        value_commas: Vec::new(),
+                    })
+            );
+        }
+
+        #[test]
+        fn removes_one_comma_token() {
+            let mut assign = LocalAssignStatement::from_variable("var")
+                .with_value(true)
+                .with_value(false)
+                .with_value(true)
+                .with_tokens(LocalAssignTokens {
+                    local: Token::from_content("local"),
+                    equal: Some(Token::from_content("=")),
+                    variable_commas: Vec::new(),
+                    value_commas: vec![Token::from_content(","), Token::from_content(",")],
+                });
+
+            assign.pop_value();
+
+            pretty_assertions::assert_eq!(
+                assign,
+                LocalAssignStatement::from_variable("var")
+                    .with_value(true)
+                    .with_value(false)
+                    .with_tokens(LocalAssignTokens {
+                        local: Token::from_content("local"),
+                        equal: Some(Token::from_content("=")),
+                        variable_commas: Vec::new(),
+                        value_commas: vec![Token::from_content(",")],
+                    })
+            );
+        }
+    }
+
+    mod remove_variable {
+        use super::*;
+
+        #[test]
+        fn single_variable_returns_none_without_mutating() {
+            let mut assign = LocalAssignStatement::from_variable("var").with_value(true);
+            let copy = assign.clone();
+
+            assert_eq!(assign.remove_variable(0), None);
+
+            pretty_assertions::assert_eq!(assign, copy);
+        }
+
+        #[test]
+        fn single_variable_remove_outside_of_bounds() {
+            let mut assign = LocalAssignStatement::from_variable("var");
+            let copy = assign.clone();
+
+            assert_eq!(assign.remove_variable(1), None);
+            pretty_assertions::assert_eq!(assign, copy);
+
+            assert_eq!(assign.remove_variable(3), None);
+            pretty_assertions::assert_eq!(assign, copy);
+        }
+
+        #[test]
+        fn two_variables_remove_first() {
+            let mut assign = LocalAssignStatement::from_variable("var")
+                .with_variable("var2")
+                .with_value(true)
+                .with_value(false);
+
+            assert_eq!(assign.remove_variable(0), Some(Identifier::new("var")));
+
+            pretty_assertions::assert_eq!(
+                assign,
+                LocalAssignStatement::from_variable("var2")
+                    .with_value(true)
+                    .with_value(false)
+            );
+        }
+
+        #[test]
+        fn two_variables_remove_second() {
+            let mut assign = LocalAssignStatement::from_variable("var")
+                .with_variable("var2")
+                .with_value(true)
+                .with_value(false);
+
+            assert_eq!(assign.remove_variable(1), Some(Identifier::new("var2")));
+
+            pretty_assertions::assert_eq!(
+                assign,
+                LocalAssignStatement::from_variable("var")
+                    .with_value(true)
+                    .with_value(false)
+            );
+        }
     }
 }
diff --git a/src/rules/remove_nil_declarations.rs b/src/rules/remove_nil_declarations.rs
index 9deb151..2ccc433 100644
--- a/src/rules/remove_nil_declarations.rs
+++ b/src/rules/remove_nil_declarations.rs
@@ -1,18 +1,81 @@
 use crate::nodes::{Block, Expression, LocalAssignStatement};
-use crate::process::{DefaultVisitor, NodeProcessor, NodeVisitor};
+use crate::process::{DefaultVisitor, Evaluator, NodeProcessor, NodeVisitor};
 use crate::rules::{
     Context, FlawlessRule, RuleConfiguration, RuleConfigurationError, RuleProperties,
 };
 
 use super::verify_no_rule_properties;
 
-#[derive(Debug, Clone, Default)]
-struct Processor {}
+#[derive(Default)]
+struct Processor {
+    evaluator: Evaluator,
+}
 
 impl NodeProcessor for Processor {
     fn process_local_assign_statement(&mut self, assignment: &mut LocalAssignStatement) {
-        while let Some(Expression::Nil(_)) = assignment.last_value() {
-            assignment.pop_value();
+        {
+            let mut pop_extra_value_at = Vec::new();
+            for (index, extra_value) in assignment
+                .iter_values()
+                .enumerate()
+                .skip(assignment.variables_len())
+            {
+                if !self.evaluator.has_side_effects(extra_value) {
+                    pop_extra_value_at.push(index);
+                }
+            }
+
+            for index in pop_extra_value_at.into_iter().rev() {
+                assignment.remove_value(index);
+            }
+        }
+
+        if assignment.values_len() > assignment.variables_len() {
+            return;
+        }
+
+        let has_nil_value = assignment
+            .iter_values()
+            .any(|value| matches!(value, Expression::Nil(_)));
+
+        if !has_nil_value {
+            return;
+        }
+
+        if assignment.variables_len() > assignment.values_len()
+            && assignment
+                .last_value()
+                .filter(|last_value| self.evaluator.can_return_multiple_values(last_value))
+                .is_some()
+        {
+            return;
+        }
+
+        let mut remove_values_at = Vec::new();
+        for (index, value) in assignment.iter_values().enumerate() {
+            if matches!(value, Expression::Nil(_)) {
+                remove_values_at.push(index);
+            }
+        }
+
+        let insert_variables: Vec<_> = remove_values_at
+            .into_iter()
+            .rev()
+            .filter_map(|index| {
+                assignment.remove_value(index);
+                assignment.remove_variable(index)
+            })
+            .collect();
+
+        for variable in insert_variables.into_iter().rev() {
+            assignment.push_variable(variable);
+        }
+
+        if let Some(last_value) = assignment.last_value() {
+            if self.evaluator.can_return_multiple_values(last_value) {
+                let new_value = assignment.pop_value().unwrap().in_parentheses();
+                assignment.push_value(new_value);
+            }
         }
     }
 }


Test patch:
diff --git a/tests/rule_tests/mod.rs b/tests/rule_tests/mod.rs
index ef497fe..00ef586 100644
--- a/tests/rule_tests/mod.rs
+++ b/tests/rule_tests/mod.rs
@@ -115,16 +115,29 @@ macro_rules! test_rule_wihout_effects {
         $(
             #[test]
             fn $name() {
-                use darklua_core::rules::Rule;
+                use darklua_core::{
+                    rules::Rule,
+                    generator::{LuaGenerator, TokenBasedLuaGenerator},
+                };
 
                 let mut block = $crate::utils::parse_input($input);
                 let expect_block = block.clone();
                 let mut context = darklua_core::rules::Context::default();
 
                 $rule.process(&mut block, &mut context)
-                    .expect("rule should suceed");
+                    .expect("rule should succeed");
 
-                pretty_assertions::assert_eq!(block, expect_block);
+                let mut generator = TokenBasedLuaGenerator::new($input);
+                generator.write_block(&block);
+                let lua_code = generator.into_string();
+
+                pretty_assertions::assert_eq!(
+                    block,
+                    expect_block,
+                    "\nexpected code:\n{}\nbut received:\n{}",
+                    $input,
+                    lua_code,
+                );
             }
         )*
     };
diff --git a/tests/rule_tests/remove_nil_declaration.rs b/tests/rule_tests/remove_nil_declaration.rs
index 292972d..28a8635 100644
--- a/tests/rule_tests/remove_nil_declaration.rs
+++ b/tests/rule_tests/remove_nil_declaration.rs
@@ -4,15 +4,29 @@ test_rule!(
     remove_nil_declaration,
     RemoveNilDeclaration::default(),
     assign_to_nil("local a = nil") => "local a",
+    assign_to_nil_and_extra_true("local a = nil, true") => "local a",
+    assign_to_true_nil_and_false("local a, b, c = true, nil, false") => "local a, c, b = true, false",
     assign_to_true_and_nil("local a, b = true, nil") => "local a, b = true",
     assign_to_nil_and_nil("local a, b = nil, nil") => "local a, b",
+    assign_call_and_nil("local a, b = call(), nil") => "local a, b = (call())",
+    assign_variadic_args_and_nil("local a, b = ..., nil") => "local a, b = (...)",
+    assign_field_expression_and_nil("local a, b = object.prop, nil") => "local a, b = (object.prop)",
+    assign_index_expression_and_nil("local a, b = object[key], nil") => "local a, b = (object[key])",
+    assign_call_and_nil_and_nil("local a, b, c = call(), nil, nil") => "local a, b, c = (call())",
+    // we can re-order variables that gets assigned to `nil`
+    assign_to_nil_and_true("local a, b = nil, true") => "local b, a = true",
+    assign_to_nil_and_nil_and_true("local a, b, c = nil, nil, true") => "local c, a, b = true",
+    assign_to_nil_and_call("local a, b = nil, call()") => "local b, a = (call())",
+    assign_to_nil_and_call_and_false("local a, b, c = nil, call(), false") => "local b, c, a = call(), false",
+    // the rule may trim unnecessary expressions in declarations
+    assign_to_call_and_true_and_variable("local a = call(), true, var") => "local a = call()",
 );
 
 test_rule_wihout_effects!(
     RemoveNilDeclaration::default(),
     assign_to_true("local a = true"),
-    assign_to_nil_and_true("local a = nil, true"),
-    assign_to_true_nil_and_false("local a, b, c = true, nil, false"),
+    assign_to_nil_and_extra_call("local a = nil, call()"),
+    assign_to_nil_and_extract_varargs("local a, b, c = nil, ..."),
 );
 
 #[test]