Repository: seaofvoices/darklua (rust)

Issue:
Add rule to remove `debug.profilebegin` and `debug.profileend` calls

Pull request:
Add rule to remove debug library profiling function calls
Closes #158 

Add a new rule to remove calls to `debug.profilebegin` and `debug.profileend` functions.

- [x] rule docs
- [x] add entry to the changelog


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index bdac5ed..6f0c78f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+* add rule to remove Roblox profiling function calls (`remove_debug_profiling`) ([#162](https://github.com/seaofvoices/darklua/pull/162))
 * add rule to remove interpolated strings (`remove_interpolated_string`) ([#156](https://github.com/seaofvoices/darklua/pull/156))
 * add support for floor division (`//`) operator in binary expressions ([#155](https://github.com/seaofvoices/darklua/pull/155))
 * add support for Luau interpolated strings ([#94](https://github.com/seaofvoices/darklua/pull/94))
diff --git a/site/content/rules/remove_debug_profiling.md b/site/content/rules/remove_debug_profiling.md
new file mode 100644
index 0000000..33ea4dd
--- /dev/null
+++ b/site/content/rules/remove_debug_profiling.md
@@ -0,0 +1,16 @@
+---
+description: Removes call to debug.profilebegin and debug.profileend
+added_in: "unreleased"
+parameters:
+  - name: preserve_arguments_side_effects
+    type: boolean
+    description: Defines how darklua handle arguments passed to the functions. If true, darklua will inspect each argument and preserve any potential side effects. When false, darklua will not perform any verification and simply erase any arguments passed.
+    default: true
+examples:
+  - content: |
+      debug.profilebegin('function name')
+      performUpdate()
+      debug.profileend()
+---
+
+This rule removes all function calls to [`debug.profilebegin`](https://create.roblox.com/docs/reference/engine/libraries/debug#profilebegin) and [`debug.profileend`](https://create.roblox.com/docs/reference/engine/libraries/debug#profileend).
diff --git a/src/rules/mod.rs b/src/rules/mod.rs
index b630676..181d80f 100644
--- a/src/rules/mod.rs
+++ b/src/rules/mod.rs
@@ -13,8 +13,10 @@ mod group_local;
 mod inject_value;
 mod method_def;
 mod no_local_function;
+mod remove_call_match;
 mod remove_comments;
 mod remove_compound_assign;
+mod remove_debug_profiling;
 mod remove_interpolated_string;
 mod remove_nil_declarations;
 mod remove_spaces;
@@ -41,6 +43,7 @@ pub use method_def::*;
 pub use no_local_function::*;
 pub use remove_comments::*;
 pub use remove_compound_assign::*;
+pub use remove_debug_profiling::*;
 pub use remove_interpolated_string::*;
 pub use remove_nil_declarations::*;
 pub use remove_spaces::*;
@@ -221,6 +224,7 @@ pub fn get_all_rule_names() -> Vec<&'static str> {
         INJECT_GLOBAL_VALUE_RULE_NAME,
         REMOVE_COMMENTS_RULE_NAME,
         REMOVE_COMPOUND_ASSIGNMENT_RULE_NAME,
+        REMOVE_DEBUG_PROFILING_RULE_NAME,
         REMOVE_EMPTY_DO_RULE_NAME,
         REMOVE_FUNCTION_CALL_PARENS_RULE_NAME,
         REMOVE_INTERPOLATED_STRING_RULE_NAME,
@@ -251,6 +255,7 @@ impl FromStr for Box<dyn Rule> {
             INJECT_GLOBAL_VALUE_RULE_NAME => Box::<InjectGlobalValue>::default(),
             REMOVE_COMMENTS_RULE_NAME => Box::<RemoveComments>::default(),
             REMOVE_COMPOUND_ASSIGNMENT_RULE_NAME => Box::<RemoveCompoundAssignment>::default(),
+            REMOVE_DEBUG_PROFILING_RULE_NAME => Box::<RemoveDebugProfiling>::default(),
             REMOVE_EMPTY_DO_RULE_NAME => Box::<RemoveEmptyDo>::default(),
             REMOVE_FUNCTION_CALL_PARENS_RULE_NAME => Box::<RemoveFunctionCallParens>::default(),
             REMOVE_INTERPOLATED_STRING_RULE_NAME => Box::<RemoveInterpolatedString>::default(),
diff --git a/src/rules/remove_call_match.rs b/src/rules/remove_call_match.rs
new file mode 100644
index 0000000..5d44cff
--- /dev/null
+++ b/src/rules/remove_call_match.rs
@@ -0,0 +1,160 @@
+use std::ops;
+
+use crate::nodes::{
+    Arguments, Block, DoStatement, Expression, LocalAssignStatement, Prefix, Statement, TableEntry,
+};
+use crate::process::{Evaluator, IdentifierTracker, NodeProcessor};
+
+pub(crate) trait CallMatch<T> {
+    fn matches(&self, identifiers: &IdentifierTracker, prefix: &Prefix) -> bool;
+}
+
+#[derive(Default)]
+pub(crate) struct RemoveFunctionCallProcessor<Args, T: CallMatch<Args>> {
+    identifier_tracker: IdentifierTracker,
+    evaluator: Evaluator,
+    preserve_args_side_effects: bool,
+    matcher: T,
+    _phantom: std::marker::PhantomData<Args>,
+}
+
+impl<F> CallMatch<(&IdentifierTracker, &Prefix)> for F
+where
+    F: Fn(&IdentifierTracker, &Prefix) -> bool,
+{
+    fn matches(&self, identifiers: &IdentifierTracker, prefix: &Prefix) -> bool {
+        (self)(identifiers, prefix)
+    }
+}
+
+impl<F> CallMatch<&Prefix> for F
+where
+    F: Fn(&Prefix) -> bool,
+{
+    fn matches(&self, _identifiers: &IdentifierTracker, prefix: &Prefix) -> bool {
+        (self)(prefix)
+    }
+}
+
+impl<Args, T: CallMatch<Args>> RemoveFunctionCallProcessor<Args, T> {
+    pub(crate) fn new(preserve_args_side_effects: bool, matcher: T) -> Self {
+        Self {
+            identifier_tracker: Default::default(),
+            evaluator: Default::default(),
+            preserve_args_side_effects,
+            matcher,
+            _phantom: Default::default(),
+        }
+    }
+
+    fn preserve_side_effects(&self, arguments: &Arguments) -> Vec<Expression> {
+        match arguments {
+            Arguments::Tuple(tuple) => tuple
+                .iter_values()
+                .filter(|value| self.evaluator.has_side_effects(value))
+                .cloned()
+                .collect(),
+            Arguments::Table(table) => {
+                let mut expressions = Vec::new();
+
+                for entry in table.iter_entries() {
+                    match entry {
+                        TableEntry::Field(field) => {
+                            let expression = field.get_value();
+                            if self.evaluator.has_side_effects(expression) {
+                                expressions.push(expression.clone());
+                            }
+                        }
+                        TableEntry::Index(index) => {
+                            let key = index.get_key();
+                            let value = index.get_value();
+
+                            if self.evaluator.has_side_effects(key) {
+                                expressions.push(key.clone());
+                            }
+                            if self.evaluator.has_side_effects(value) {
+                                expressions.push(value.clone());
+                            }
+                        }
+                        TableEntry::Value(value) => {
+                            if self.evaluator.has_side_effects(value) {
+                                expressions.push(value.clone());
+                            }
+                        }
+                    }
+                }
+
+                expressions
+            }
+            Arguments::String(_) => Vec::new(),
+        }
+    }
+}
+
+impl<Args, T: CallMatch<Args>> ops::Deref for RemoveFunctionCallProcessor<Args, T> {
+    type Target = IdentifierTracker;
+
+    fn deref(&self) -> &Self::Target {
+        &self.identifier_tracker
+    }
+}
+
+impl<Args, T: CallMatch<Args>> ops::DerefMut for RemoveFunctionCallProcessor<Args, T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.identifier_tracker
+    }
+}
+
+impl<Args, T: CallMatch<Args>> NodeProcessor for RemoveFunctionCallProcessor<Args, T> {
+    fn process_statement(&mut self, statement: &mut Statement) {
+        if let Statement::Call(call) = statement {
+            if self
+                .matcher
+                .matches(&self.identifier_tracker, call.get_prefix())
+            {
+                *statement = if self.preserve_args_side_effects {
+                    let values = self.preserve_side_effects(call.get_arguments());
+
+                    if values.is_empty() {
+                        DoStatement::default()
+                    } else {
+                        DoStatement::new(Block::default().with_statement(values.into_iter().fold(
+                            LocalAssignStatement::from_variable("_"),
+                            |assignment, value| assignment.with_value(value),
+                        )))
+                    }
+                } else {
+                    DoStatement::default()
+                }
+                .into();
+            }
+        }
+    }
+
+    fn process_expression(&mut self, expression: &mut Expression) {
+        if let Expression::Call(call) = expression {
+            if self
+                .matcher
+                .matches(&self.identifier_tracker, call.get_prefix())
+            {
+                *expression = if self.preserve_args_side_effects {
+                    let values = self.preserve_side_effects(call.get_arguments());
+
+                    if values.is_empty() {
+                        Expression::nil()
+                    } else if values.len() == 1 {
+                        values
+                            .into_iter()
+                            .next()
+                            .expect("expected to obtain a value")
+                    } else {
+                        todo!()
+                        // (e1 or true) and (e2 or true) and (e3 or true) ...
+                    }
+                } else {
+                    Expression::nil()
+                }
+            }
+        }
+    }
+}
diff --git a/src/rules/remove_debug_profiling.rs b/src/rules/remove_debug_profiling.rs
new file mode 100644
index 0000000..5821d4e
--- /dev/null
+++ b/src/rules/remove_debug_profiling.rs
@@ -0,0 +1,119 @@
+use crate::nodes::{Block, Prefix};
+use crate::process::{IdentifierTracker, NodeVisitor, ScopeVisitor};
+use crate::rules::{
+    Context, FlawlessRule, RuleConfiguration, RuleConfigurationError, RuleProperties,
+};
+
+use super::remove_call_match::RemoveFunctionCallProcessor;
+
+const DEBUG_LIBRARY_NAME: &str = "debug";
+const START_PROFILE_FUNFCTION: &str = "profilebegin";
+const STOP_PROFILE_FUNCTION: &str = "profileend";
+
+pub const REMOVE_DEBUG_PROFILING_RULE_NAME: &str = "remove_debug_profiling";
+
+/// A rule that removes `debug.profilebegin` and `debug.profileend` calls.
+#[derive(Debug, PartialEq, Eq)]
+pub struct RemoveDebugProfiling {
+    preserve_args_side_effects: bool,
+}
+
+impl Default for RemoveDebugProfiling {
+    fn default() -> Self {
+        Self {
+            preserve_args_side_effects: true,
+        }
+    }
+}
+
+fn should_remove_call(identifiers: &IdentifierTracker, prefix: &Prefix) -> bool {
+    if identifiers.is_identifier_used(DEBUG_LIBRARY_NAME) {
+        return false;
+    }
+
+    match prefix {
+        Prefix::Field(field_expression)
+            if field_expression.get_field().get_name() == START_PROFILE_FUNFCTION
+                || field_expression.get_field().get_name() == STOP_PROFILE_FUNCTION =>
+        {
+            matches!(field_expression.get_prefix(), Prefix::Identifier(identifier) if identifier.get_name() == DEBUG_LIBRARY_NAME)
+        }
+        _ => false,
+    }
+}
+
+impl FlawlessRule for RemoveDebugProfiling {
+    fn flawless_process(&self, block: &mut Block, _: &Context) {
+        let mut processor =
+            RemoveFunctionCallProcessor::new(self.preserve_args_side_effects, should_remove_call);
+        ScopeVisitor::visit_block(block, &mut processor);
+    }
+}
+
+impl RuleConfiguration for RemoveDebugProfiling {
+    fn configure(&mut self, properties: RuleProperties) -> Result<(), RuleConfigurationError> {
+        for (key, value) in properties {
+            match key.as_str() {
+                "preserve_arguments_side_effects" => {
+                    self.preserve_args_side_effects = value.expect_bool(&key)?;
+                }
+                _ => return Err(RuleConfigurationError::UnexpectedProperty(key)),
+            }
+        }
+
+        Ok(())
+    }
+
+    fn get_name(&self) -> &'static str {
+        REMOVE_DEBUG_PROFILING_RULE_NAME
+    }
+
+    fn serialize_to_properties(&self) -> RuleProperties {
+        let mut properties = RuleProperties::new();
+
+        if !self.preserve_args_side_effects {
+            properties.insert("preserve_arguments_side_effects".to_owned(), false.into());
+        }
+
+        properties
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use crate::rules::Rule;
+
+    use insta::assert_json_snapshot;
+
+    fn new_rule() -> RemoveDebugProfiling {
+        RemoveDebugProfiling::default()
+    }
+
+    #[test]
+    fn serialize_default_rule() {
+        let rule: Box<dyn Rule> = Box::new(new_rule());
+
+        assert_json_snapshot!("default_remove_debug_profiling", rule);
+    }
+
+    #[test]
+    fn serialize_rule_without_side_effects() {
+        let rule: Box<dyn Rule> = Box::new(RemoveDebugProfiling {
+            preserve_args_side_effects: false,
+        });
+
+        assert_json_snapshot!("remove_debug_profiling_without_side_effects", rule);
+    }
+
+    #[test]
+    fn configure_with_extra_field_error() {
+        let result = json5::from_str::<Box<dyn Rule>>(
+            r#"{
+            rule: 'remove_debug_profiling',
+            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_debug_profiling__test__default_remove_debug_profiling.snap b/src/rules/snapshots/darklua_core__rules__remove_debug_profiling__test__default_remove_debug_profiling.snap
new file mode 100644
index 0000000..79bb51f
--- /dev/null
+++ b/src/rules/snapshots/darklua_core__rules__remove_debug_profiling__test__default_remove_debug_profiling.snap
@@ -0,0 +1,5 @@
+---
+source: src/rules/remove_debug_profiling.rs
+expression: rule
+---
+"remove_debug_profiling"
diff --git a/src/rules/snapshots/darklua_core__rules__remove_debug_profiling__test__remove_debug_profiling_without_side_effects.snap b/src/rules/snapshots/darklua_core__rules__remove_debug_profiling__test__remove_debug_profiling_without_side_effects.snap
new file mode 100644
index 0000000..41f7c4b
--- /dev/null
+++ b/src/rules/snapshots/darklua_core__rules__remove_debug_profiling__test__remove_debug_profiling_without_side_effects.snap
@@ -0,0 +1,8 @@
+---
+source: src/rules/remove_debug_profiling.rs
+expression: rule
+---
+{
+  "rule": "remove_debug_profiling",
+  "preserve_arguments_side_effects": false
+}
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 341ad99..9529793 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
@@ -13,6 +13,7 @@ expression: rule_names
   "inject_global_value",
   "remove_comments",
   "remove_compound_assignment",
+  "remove_debug_profiling",
   "remove_empty_do",
   "remove_function_call_parens",
   "remove_interpolated_string",
diff --git a/tests/rule_tests/mod.rs b/tests/rule_tests/mod.rs
index 18ddc67..1bf0015 100644
--- a/tests/rule_tests/mod.rs
+++ b/tests/rule_tests/mod.rs
@@ -294,6 +294,7 @@ mod no_local_function;
 mod remove_call_parens;
 mod remove_comments;
 mod remove_compound_assignment;
+mod remove_debug_profiling;
 mod remove_empty_do;
 mod remove_interpolated_string;
 mod remove_method_definition;
diff --git a/tests/rule_tests/remove_debug_profiling.rs b/tests/rule_tests/remove_debug_profiling.rs
new file mode 100644
index 0000000..5cf9ebf
--- /dev/null
+++ b/tests/rule_tests/remove_debug_profiling.rs
@@ -0,0 +1,45 @@
+use darklua_core::rules::{RemoveDebugProfiling, Rule};
+
+test_rule!(
+    remove_debug_profiling,
+    RemoveDebugProfiling::default(),
+    remove_profile_begin("debug.profilebegin('label')") => "do end",
+    remove_profile_end("debug.profileend()") => "do end",
+    remove_profiling_around_function_call("debug.profilebegin('label') fn() debug.profileend()") => "do end fn() do end",
+    remove_profile_begin_with_maybe_side_effects("debug.profilebegin(getLabel())") => "do local _ = getLabel() end",
+);
+
+test_rule!(
+    remove_debug_profiling_without_side_effects,
+    json5::from_str::<Box<dyn Rule>>(
+        r#"{
+        rule: 'remove_debug_profiling',
+        preserve_arguments_side_effects: false,
+    }"#,
+    )
+    .unwrap(),
+    remove_profile_begin("debug.profilebegin('label')") => "do end",
+    remove_profile_end("debug.profileend()") => "do end",
+    remove_profiling_around_function_call("debug.profilebegin('label') fn() debug.profileend()") => "do end fn() do end",
+    remove_profile_begin_with_maybe_side_effects("debug.profilebegin(getLabel())") => "do end",
+);
+
+test_rule_without_effects!(
+    RemoveDebugProfiling::default(),
+    debug_library_identifier_used("local debug = nil debug.profilebegin('label')"),
+);
+
+#[test]
+fn deserialize_from_object_notation() {
+    json5::from_str::<Box<dyn Rule>>(
+        r#"{
+        rule: 'remove_debug_profiling',
+    }"#,
+    )
+    .unwrap();
+}
+
+#[test]
+fn deserialize_from_string() {
+    json5::from_str::<Box<dyn Rule>>("'remove_debug_profiling'").unwrap();
+}