Repository: seaofvoices/darklua (rust)

Issue:
Remove simple method calls
When calling a method like this:
```lua
foo:bar()
```
The code should be reformatted to:
```lua
foo.bar(foo)
```

Pull request:
Add rule to remove method calls
Closes #3 

Add a new rule `remove_method_call` that converts calls using the colon (`:`) syntax to a field call. The rule applies the transform only when the prefix is an identifier.

- [x] add entry to the changelog


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 98713a1..1e36e98 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,7 @@
 # Changelog
 
+* add rule to remove method call syntax (`remove_method_call`) ([#276](https://github.com/seaofvoices/darklua/pull/276))
+* fix `remove_unused_variable` rule to correctly handle trailing unassigned (but used!) variables ([#275](https://github.com/seaofvoices/darklua/pull/275))
 * add rule to convert Luau numbers (`convert_luau_number`) ([#274](https://github.com/seaofvoices/darklua/pull/274))
 * export the `PathRequireMode` struct when using the darklua library and refactor AST node types to reduce size difference between variants ([#273](https://github.com/seaofvoices/darklua/pull/273))
 
diff --git a/site/content/rules/remove_method_call.md b/site/content/rules/remove_method_call.md
new file mode 100644
index 0000000..4aa6570
--- /dev/null
+++ b/site/content/rules/remove_method_call.md
@@ -0,0 +1,13 @@
+---
+description: Removes usages of the method syntax (`:`) in function calls
+added_in: "unreleased"
+parameters: []
+examples:
+  - content: "obj:method()"
+  - content: "game:GetService('RunService')"
+  - content: "return player:LoadCharacter()"
+---
+
+This rule converts function calls that use the colon syntax (`:`) to function calls using dot notation (`.`).
+
+This rule only transforms method calls where the prefix is an identifier. Other method calls using field access, index access or function call prefixes are left unchanged.
diff --git a/src/nodes/arguments.rs b/src/nodes/arguments.rs
index 5187319..d49b28f 100644
--- a/src/nodes/arguments.rs
+++ b/src/nodes/arguments.rs
@@ -1,4 +1,4 @@
-use std::iter;
+use std::{iter, mem};
 
 use crate::nodes::{Expression, StringExpression, TableExpression, Token};
 
@@ -59,10 +59,41 @@ impl TupleArguments {
 
     /// Adds an argument to this tuple.
     pub fn with_argument<T: Into<Expression>>(mut self, argument: T) -> Self {
-        self.values.push(argument.into());
+        self.push(argument.into());
         self
     }
 
+    /// Pushes an argument to this tuple.
+    pub fn push(&mut self, argument: impl Into<Expression>) {
+        let argument = argument.into();
+        let initial_len = self.values.len();
+
+        self.values.push(argument);
+
+        if initial_len != 0 {
+            if let Some(tokens) = &mut self.tokens {
+                if tokens.commas.len() == initial_len - 1 {
+                    tokens.commas.push(Token::from_content(","));
+                }
+            }
+        }
+    }
+
+    /// Inserts an argument at the specified index.
+    pub fn insert(&mut self, index: usize, argument: impl Into<Expression>) {
+        if index >= self.values.len() {
+            self.push(argument.into());
+        } else {
+            self.values.insert(index, argument.into());
+
+            if let Some(tokens) = &mut self.tokens {
+                if index <= tokens.commas.len() {
+                    tokens.commas.insert(index, Token::from_content(","));
+                }
+            }
+        }
+    }
+
     /// Returns the number of arguments in this tuple.
     #[inline]
     pub fn len(&self) -> usize {
@@ -135,6 +166,47 @@ impl Arguments {
         TupleArguments::from(self).with_argument(argument).into()
     }
 
+    /// Pushes an argument to these arguments, converting to a tuple if needed.
+    pub fn push(&mut self, argument: impl Into<Expression>) {
+        let argument = argument.into();
+
+        let tuple_args = match self {
+            Arguments::Tuple(tuple) => {
+                tuple.push(argument);
+                return;
+            }
+            Arguments::String(value) => TupleArguments::default()
+                .with_argument(mem::replace(value, StringExpression::empty())),
+            Arguments::Table(value) => TupleArguments::default().with_argument(mem::take(value)),
+        };
+
+        *self = tuple_args.with_argument(argument).into();
+    }
+
+    /// Inserts an argument at the specified index, converting to a tuple if needed.
+    pub fn insert(&mut self, index: usize, argument: impl Into<Expression>) {
+        let argument = argument.into();
+
+        let mut tuple_args = match self {
+            Arguments::Tuple(tuple) => {
+                tuple.insert(index, argument);
+                return;
+            }
+            Arguments::String(value) => {
+                let string = mem::replace(value, StringExpression::empty());
+                TupleArguments::default().with_argument(Expression::from(string))
+            }
+            Arguments::Table(value) => {
+                let table = mem::take(value);
+                TupleArguments::default().with_argument(Expression::from(table))
+            }
+        };
+
+        tuple_args.insert(index, argument);
+
+        *self = tuple_args.into();
+    }
+
     /// Removes all comments from these arguments.
     pub fn clear_comments(&mut self) {
         match self {
@@ -199,3 +271,69 @@ impl From<StringExpression> for Arguments {
         Self::String(string)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        nodes::{Identifier, Statement},
+        Parser,
+    };
+
+    fn parse_arguments_with_tokens(lua: &str) -> Arguments {
+        let parser = Parser::default().preserve_tokens();
+
+        let code = format!("f {}", lua);
+
+        let block = parser.parse(&code).expect("code should parse");
+        if let Some(statement) = block.first_statement() {
+            if let Statement::Call(call) = statement {
+                return call.get_arguments().clone();
+            }
+        }
+        panic!("failed to parse call arguments from: {}", lua);
+    }
+
+    fn get_tuple_tokens(args: &Arguments) -> &TupleArgumentsTokens {
+        match args {
+            Arguments::Tuple(tuple) => tuple.get_tokens().expect("tuple should have tokens"),
+            Arguments::String(_) | Arguments::Table(_) => panic!("expected tuple arguments"),
+        }
+    }
+
+    fn expect_comma_tokens(args: &Arguments, index: usize) {
+        let tokens = get_tuple_tokens(args);
+        assert_eq!(tokens.commas[index], Token::from_content(","));
+    }
+
+    #[test]
+    fn push_argument_handles_commas() {
+        let mut args = parse_arguments_with_tokens("()");
+
+        args.push(Identifier::new("first"));
+        assert_eq!(get_tuple_tokens(&args).commas.len(), 0);
+
+        args.push(Identifier::new("second"));
+        assert_eq!(get_tuple_tokens(&args).commas.len(), 1);
+        expect_comma_tokens(&args, 0);
+
+        args.push(Identifier::new("third"));
+        assert_eq!(get_tuple_tokens(&args).commas.len(), 2);
+        expect_comma_tokens(&args, 1);
+    }
+
+    #[test]
+    fn insert_argument_handles_commas() {
+        let mut args = parse_arguments_with_tokens("(first, third)");
+
+        args.insert(1, Identifier::new("second"));
+        assert_eq!(get_tuple_tokens(&args).commas.len(), 2);
+        expect_comma_tokens(&args, 1);
+
+        args.insert(3, Identifier::new("fourth"));
+        assert_eq!(get_tuple_tokens(&args).commas.len(), 3);
+
+        args.insert(0, Identifier::new("zero"));
+        assert_eq!(get_tuple_tokens(&args).commas.len(), 4);
+    }
+}
diff --git a/src/nodes/function_call.rs b/src/nodes/function_call.rs
index adf6add..5afdb68 100644
--- a/src/nodes/function_call.rs
+++ b/src/nodes/function_call.rs
@@ -98,6 +98,12 @@ impl FunctionCall {
         self.method.as_ref()
     }
 
+    /// Returns if this call uses a method.
+    #[inline]
+    pub fn has_method(&self) -> bool {
+        self.method.is_some()
+    }
+
     /// Returns the prefix (what is being called) of this function call.
     #[inline]
     pub fn get_prefix(&self) -> &Prefix {
@@ -107,7 +113,11 @@ impl FunctionCall {
     /// Removes and returns the method name, if any.
     #[inline]
     pub fn take_method(&mut self) -> Option<Identifier> {
-        self.method.take()
+        let method = self.method.take();
+        if let Some(tokens) = self.tokens.as_mut() {
+            tokens.colon = None;
+        }
+        method
     }
 
     /// Sets the arguments for this function call.
@@ -136,3 +146,29 @@ impl FunctionCall {
 
     super::impl_token_fns!(iter = [tokens, method]);
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{nodes::Statement, Parser};
+
+    fn parse_call(code: &str) -> FunctionCall {
+        let parser = Parser::default().preserve_tokens();
+        let block = parser.parse(code).expect("code should parse");
+        if let Some(statement) = block.first_statement() {
+            if let Statement::Call(call) = statement {
+                return call.clone();
+            }
+        }
+        panic!("failed to parse call from: {}", code);
+    }
+
+    #[test]
+    fn test_take_method_removes_colon_token() {
+        let mut call = parse_call("obj:method()");
+
+        assert!(call.get_tokens().unwrap().colon.is_some());
+        call.take_method();
+        assert!(call.get_tokens().unwrap().colon.is_none());
+    }
+}
diff --git a/src/rules/mod.rs b/src/rules/mod.rs
index a5f2f9c..f786933 100644
--- a/src/rules/mod.rs
+++ b/src/rules/mod.rs
@@ -27,6 +27,7 @@ mod remove_debug_profiling;
 mod remove_floor_division;
 mod remove_if_expression;
 mod remove_interpolated_string;
+mod remove_method_call;
 mod remove_nil_declarations;
 mod remove_spaces;
 mod remove_types;
@@ -60,6 +61,7 @@ pub use remove_debug_profiling::*;
 pub use remove_floor_division::*;
 pub use remove_if_expression::*;
 pub use remove_interpolated_string::*;
+pub use remove_method_call::*;
 pub use remove_nil_declarations::*;
 pub use remove_spaces::*;
 pub use remove_types::*;
@@ -304,6 +306,7 @@ pub fn get_all_rule_names() -> Vec<&'static str> {
         REMOVE_EMPTY_DO_RULE_NAME,
         REMOVE_FUNCTION_CALL_PARENS_RULE_NAME,
         REMOVE_INTERPOLATED_STRING_RULE_NAME,
+        REMOVE_METHOD_CALL_RULE_NAME,
         REMOVE_METHOD_DEFINITION_RULE_NAME,
         REMOVE_NIL_DECLARATION_RULE_NAME,
         REMOVE_SPACES_RULE_NAME,
@@ -341,6 +344,7 @@ impl FromStr for Box<dyn Rule> {
             REMOVE_FLOOR_DIVISION_RULE_NAME => Box::<RemoveFloorDivision>::default(),
             REMOVE_FUNCTION_CALL_PARENS_RULE_NAME => Box::<RemoveFunctionCallParens>::default(),
             REMOVE_INTERPOLATED_STRING_RULE_NAME => Box::<RemoveInterpolatedString>::default(),
+            REMOVE_METHOD_CALL_RULE_NAME => Box::<RemoveMethodCall>::default(),
             REMOVE_METHOD_DEFINITION_RULE_NAME => Box::<RemoveMethodDefinition>::default(),
             REMOVE_NIL_DECLARATION_RULE_NAME => Box::<RemoveNilDeclaration>::default(),
             REMOVE_SPACES_RULE_NAME => Box::<RemoveSpaces>::default(),
diff --git a/src/rules/remove_method_call.rs b/src/rules/remove_method_call.rs
new file mode 100644
index 0000000..37db41c
--- /dev/null
+++ b/src/rules/remove_method_call.rs
@@ -0,0 +1,121 @@
+use crate::nodes::{Block, Expression, FieldExpression, FunctionCall, Prefix};
+use crate::process::{DefaultVisitor, NodeProcessor, NodeVisitor};
+use crate::rules::{
+    Context, FlawlessRule, RuleConfiguration, RuleConfigurationError, RuleProperties,
+};
+
+use super::verify_no_rule_properties;
+
+#[derive(Debug, Default)]
+struct Processor {}
+
+impl NodeProcessor for Processor {
+    fn process_expression(&mut self, expression: &mut Expression) {
+        if let Expression::Call(call) = expression {
+            self.process_function_call(call);
+        }
+    }
+
+    fn process_function_call(&mut self, call: &mut FunctionCall) {
+        if !call.has_method() {
+            return;
+        }
+
+        let replace_with: Option<Prefix> = match call.get_prefix() {
+            Prefix::Identifier(identifier) => Some(identifier.clone().into()),
+            Prefix::Parenthese(parenthese) => match *parenthese.inner_expression() {
+                Expression::If(_)
+                | Expression::Index(_)
+                | Expression::Field(_)
+                | Expression::Function(_)
+                | Expression::Binary(_)
+                | Expression::Table(_)
+                | Expression::Unary(_)
+                | Expression::VariableArguments(_)
+                | Expression::TypeCast(_)
+                | Expression::InterpolatedString(_)
+                | Expression::Parenthese(_)
+                | Expression::Call(_) => None,
+
+                Expression::Nil(_)
+                | Expression::True(_)
+                | Expression::False(_)
+                | Expression::String(_)
+                | Expression::Number(_)
+                | Expression::Identifier(_) => Some(parenthese.inner_expression().clone().into()),
+            },
+            _ => None,
+        };
+
+        if let Some(new_prefix) = replace_with {
+            let method_name = call
+                .take_method()
+                .expect("method name is expected to exist");
+
+            *call.mutate_prefix() = FieldExpression::new(new_prefix.clone(), method_name).into();
+            call.mutate_arguments()
+                .insert(0, Expression::from(new_prefix));
+        }
+    }
+}
+
+pub const REMOVE_METHOD_CALL_RULE_NAME: &str = "remove_method_call";
+
+/// A rule that converts method calls with identifier prefixes to regular function calls.
+///
+/// This rule transforms calls like `obj:method()` to `obj.method()` when the prefix
+/// is a simple identifier.
+#[derive(Debug, Default, PartialEq, Eq)]
+pub struct RemoveMethodCall {}
+
+impl FlawlessRule for RemoveMethodCall {
+    fn flawless_process(&self, block: &mut Block, _: &Context) {
+        let mut processor = Processor::default();
+        DefaultVisitor::visit_block(block, &mut processor);
+    }
+}
+
+impl RuleConfiguration for RemoveMethodCall {
+    fn configure(&mut self, properties: RuleProperties) -> Result<(), RuleConfigurationError> {
+        verify_no_rule_properties(&properties)?;
+        Ok(())
+    }
+
+    fn get_name(&self) -> &'static str {
+        REMOVE_METHOD_CALL_RULE_NAME
+    }
+
+    fn serialize_to_properties(&self) -> RuleProperties {
+        RuleProperties::new()
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use crate::rules::Rule;
+
+    use insta::assert_json_snapshot;
+
+    fn new_rule() -> RemoveMethodCall {
+        RemoveMethodCall::default()
+    }
+
+    #[test]
+    fn serialize_default_rule() {
+        let rule: Box<dyn Rule> = Box::new(new_rule());
+
+        assert_json_snapshot!("default_remove_method_call", rule);
+    }
+
+    #[test]
+    fn configure_with_extra_field_error() {
+        let result = json5::from_str::<Box<dyn Rule>>(
+            r#"{
+            rule: 'remove_method_call',
+            prop: "something",
+        }"#,
+        );
+        pretty_assertions::assert_eq!(result.unwrap_err().to_string(), "unexpected field 'prop'");
+    }
+}


Test patch:
diff --git a/src/rules/snapshots/darklua_core__rules__remove_method_call__test__default_remove_method_call.snap b/src/rules/snapshots/darklua_core__rules__remove_method_call__test__default_remove_method_call.snap
new file mode 100644
index 0000000..5db2c77
--- /dev/null
+++ b/src/rules/snapshots/darklua_core__rules__remove_method_call__test__default_remove_method_call.snap
@@ -0,0 +1,5 @@
+---
+source: src/rules/remove_method_call.rs
+expression: rule
+---
+"remove_method_call"
diff --git a/src/rules/snapshots/darklua_core__rules__remove_simple_method_call__test__default_remove_simple_method_call.snap b/src/rules/snapshots/darklua_core__rules__remove_simple_method_call__test__default_remove_simple_method_call.snap
new file mode 100644
index 0000000..db99e80
--- /dev/null
+++ b/src/rules/snapshots/darklua_core__rules__remove_simple_method_call__test__default_remove_simple_method_call.snap
@@ -0,0 +1,5 @@
+---
+source: src/rules/remove_simple_method_call.rs
+expression: rule
+---
+"remove_simple_method_call"
diff --git a/src/rules/snapshots/darklua_core__rules__test__all_rule_names.snap b/src/rules/snapshots/darklua_core__rules__test__all_rule_names.snap
index 66e01b2..b3e9370 100644
--- a/src/rules/snapshots/darklua_core__rules__test__all_rule_names.snap
+++ b/src/rules/snapshots/darklua_core__rules__test__all_rule_names.snap
@@ -19,6 +19,7 @@ expression: rule_names
   "remove_empty_do",
   "remove_function_call_parens",
   "remove_interpolated_string",
+  "remove_method_call",
   "remove_method_definition",
   "remove_nil_declaration",
   "remove_spaces",
diff --git a/tests/rule_tests/mod.rs b/tests/rule_tests/mod.rs
index 664fddd..b62bab1 100644
--- a/tests/rule_tests/mod.rs
+++ b/tests/rule_tests/mod.rs
@@ -434,6 +434,7 @@ mod remove_empty_do;
 mod remove_floor_division;
 mod remove_if_expression;
 mod remove_interpolated_string;
+mod remove_method_call;
 mod remove_method_definition;
 mod remove_nil_declaration;
 mod remove_types;
diff --git a/tests/rule_tests/remove_method_call.rs b/tests/rule_tests/remove_method_call.rs
new file mode 100644
index 0000000..891f641
--- /dev/null
+++ b/tests/rule_tests/remove_method_call.rs
@@ -0,0 +1,40 @@
+use darklua_core::rules::{RemoveMethodCall, Rule};
+
+test_rule!(
+    remove_method_call,
+    RemoveMethodCall::default(),
+    simple_method_call("obj:method()") => "obj.method(obj)",
+    method_call_with_args("obj:method(1, 2, 3)") => "obj.method(obj, 1, 2, 3)",
+    method_call_with_string("obj:method('hello')") => "obj.method(obj, 'hello')",
+    method_call_with_string_args("obj:method'hello'") => "obj.method(obj, 'hello')",
+    method_call_with_table("obj:method({key = 'value'})") => "obj.method(obj, {key = 'value'})",
+    method_call_with_table_args("obj:method{key = 'value'}") => "obj.method(obj, {key = 'value'})",
+    assign_method_call("local result = obj:method()") => "local result = obj.method(obj)",
+    nested_method_call("other:func(obj:method())") => "other.func(other, obj.method(obj))",
+    method_call_with_variadic_argument("myTable:insert(...)") => "myTable.insert(myTable, ...)",
+    removes_parentheses_of_a_method_call("(obj):method()") => "obj.method(obj)",
+);
+
+test_rule_without_effects!(
+    RemoveMethodCall::default(),
+    regular_function_call("obj.method()"),
+    regular_function_call_with_args("obj.method(1, 2, 3)"),
+    field_method_call("obj.field:method()"),
+    index_method_call("obj[key]:method()"),
+    call_method_call("obj():method()"),
+);
+
+#[test]
+fn deserialize_from_object_notation() {
+    json5::from_str::<Box<dyn Rule>>(
+        r#"{
+        rule: 'remove_method_call',
+    }"#,
+    )
+    .unwrap();
+}
+
+#[test]
+fn deserialize_from_string() {
+    json5::from_str::<Box<dyn Rule>>("'remove_method_call'").unwrap();
+}