Add rule for no assignment in function parameters

Author: chrisopperwall-qz

crates/linter/src/rule/correctness/mod.rs

@@ -2,6 +2,7 @@ pub mod assert_description;
 pub mod constant_type;
 pub mod identity_comparison;
 pub mod invalid_open_tag;
+pub mod no_assign_in_argument;
 pub mod no_assign_in_condition;
 pub mod no_boolean_literal_comparison;
 pub mod no_empty_catch_clause;
@@ -17,6 +18,7 @@ pub use assert_description::*;
 pub use constant_type::*;
 pub use identity_comparison::*;
 pub use invalid_open_tag::*;
+pub use no_assign_in_argument::*;
 pub use no_assign_in_condition::*;
 pub use no_boolean_literal_comparison::*;
 pub use no_empty_catch_clause::*;

crates/linter/src/rule/correctness/no_assign_in_argument.rs

@@ -0,0 +1,242 @@
+use indoc::indoc;
+use schemars::JsonSchema;
+use serde::Deserialize;
+use serde::Serialize;
+
+use mago_reporting::Annotation;
+use mago_reporting::Issue;
+use mago_reporting::Level;
+use mago_span::HasSpan;
+use mago_syntax::ast::Assignment;
+use mago_syntax::ast::AssignmentOperator;
+use mago_syntax::ast::Expression;
+use mago_syntax::ast::Node;
+use mago_syntax::ast::NodeKind;
+
+use crate::category::Category;
+use crate::context::LintContext;
+use crate::requirements::RuleRequirements;
+use crate::rule::Config;
+use crate::rule::LintRule;
+use crate::rule_meta::RuleMeta;
+use crate::settings::RuleSettings;
+
+#[derive(Debug, Clone)]
+pub struct NoAssignInArgumentRule {
+    meta: &'static RuleMeta,
+    cfg: NoAssignInArgumentConfig,
+}
+
+#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, Serialize, Deserialize, JsonSchema)]
+#[serde(default, rename_all = "kebab-case", deny_unknown_fields)]
+pub struct NoAssignInArgumentConfig {
+    pub level: Level,
+}
+
+impl Default for NoAssignInArgumentConfig {
+    fn default() -> Self {
+        Self { level: Level::Warning }
+    }
+}
+
+impl Config for NoAssignInArgumentConfig {
+    fn level(&self) -> Level {
+        self.level
+    }
+}
+
+impl LintRule for NoAssignInArgumentRule {
+    type Config = NoAssignInArgumentConfig;
+
+    fn meta() -> &'static RuleMeta {
+        const META: RuleMeta = RuleMeta {
+            name: "No Assign In Argument",
+            code: "no-assign-in-argument",
+            description: indoc! {"
+                Detects assignments in function call arguments which can lead to unexpected behavior and make
+                the code harder to read and understand.
+            "},
+            good_example: indoc! {r"
+                <?php
+
+                $x = 5;
+                foo($x);
+            "},
+            bad_example: indoc! {r"
+                <?php
+
+                foo($x = 5);
+            "},
+            category: Category::Correctness,
+
+            requirements: RuleRequirements::None,
+        };
+
+        &META
+    }
+
+    fn targets() -> &'static [NodeKind] {
+        const TARGETS: &[NodeKind] = &[
+            NodeKind::FunctionCall,
+            NodeKind::MethodCall,
+            NodeKind::NullSafeMethodCall,
+            NodeKind::StaticMethodCall,
+        ];
+
+        TARGETS
+    }
+
+    fn build(settings: &RuleSettings<Self::Config>) -> Self {
+        Self { meta: Self::meta(), cfg: settings.config }
+    }
+
+    fn check<'arena>(&self, ctx: &mut LintContext<'_, 'arena>, node: Node<'_, 'arena>) {
+        let argument_list = match node {
+            Node::FunctionCall(call) => &call.argument_list,
+            Node::MethodCall(call) => &call.argument_list,
+            Node::NullSafeMethodCall(call) => &call.argument_list,
+            Node::StaticMethodCall(call) => &call.argument_list,
+            _ => return,
+        };
+
+        for argument in argument_list.arguments.iter() {
+            let value = argument.value();
+
+            if let Some(assignment) = get_assignment_from_expression(value) {
+                let mut issue = Issue::new(self.cfg.level(), "Avoid assignments in function call arguments.")
+                    .with_code(self.meta.code)
+                    .with_annotation(
+                        Annotation::primary(assignment.span()).with_message("This is an assignment"),
+                    )
+                    .with_annotation(
+                        Annotation::secondary(argument.span()).with_message("This is the argument"),
+                    )
+                    .with_note("Assigning a value within a function call argument can lead to unexpected behavior and make the code harder to read and understand.")
+                    .with_help("Consider assigning the variable before the function call.");
+
+                if matches!(&assignment.operator, AssignmentOperator::Assign(_)) {
+                    issue = issue.with_note("It's easy to confuse assignment (`=`) with comparison (`==`) in this context. Ensure you're using the correct operator.");
+                }
+
+                ctx.collector.report(issue);
+            }
+        }
+    }
+}
+
+#[inline]
+fn get_assignment_from_expression<'ast, 'arena>(
+    expression: &'ast Expression<'arena>,
+) -> Option<&'ast Assignment<'arena>> {
+    match expression {
+        Expression::Parenthesized(parenthesized) => get_assignment_from_expression(parenthesized.expression),
+        Expression::Assignment(assignment) => Some(assignment),
+        _ => None,
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use indoc::indoc;
+
+    use super::NoAssignInArgumentRule;
+    use crate::test_lint_failure;
+    use crate::test_lint_success;
+
+    test_lint_failure! {
+        name = assignment_in_function_argument,
+        rule = NoAssignInArgumentRule,
+        count = 1,
+        code = indoc! {r"
+            <?php
+
+            foo($x = 5);
+        "}
+    }
+
+    test_lint_failure! {
+        name = assignment_in_method_argument,
+        rule = NoAssignInArgumentRule,
+        count = 1,
+        code = indoc! {r"
+            <?php
+
+            $obj->method($x = getValue());
+        "}
+    }
+
+    test_lint_failure! {
+        name = assignment_in_static_method_argument,
+        rule = NoAssignInArgumentRule,
+        count = 1,
+        code = indoc! {r"
+            <?php
+
+            MyClass::method($x = 10);
+        "}
+    }
+
+    test_lint_failure! {
+        name = assignment_in_null_safe_method_argument,
+        rule = NoAssignInArgumentRule,
+        count = 1,
+        code = indoc! {r"
+            <?php
+
+            $obj?->method($x = 5);
+        "}
+    }
+
+    test_lint_failure! {
+        name = assignment_with_parentheses,
+        rule = NoAssignInArgumentRule,
+        count = 1,
+        code = indoc! {r"
+            <?php
+
+            foo(($x = 5));
+        "}
+    }
+
+    test_lint_failure! {
+        name = multiple_arguments_with_assignment,
+        rule = NoAssignInArgumentRule,
+        count = 1,
+        code = indoc! {r"
+            <?php
+
+            foo($a, $b = 5, $c);
+        "}
+    }
+
+    test_lint_success! {
+        name = variable_used_as_argument,
+        rule = NoAssignInArgumentRule,
+        code = indoc! {r"
+            <?php
+
+            $x = 5;
+            foo($x);
+        "}
+    }
+
+    test_lint_success! {
+        name = expression_without_assignment,
+        rule = NoAssignInArgumentRule,
+        code = indoc! {r"
+            <?php
+
+            foo($x + 5);
+        "}
+    }
+
+    test_lint_success! {
+        name = function_call_as_argument,
+        rule = NoAssignInArgumentRule,
+        code = indoc! {r"
+            <?php
+
+            foo(getValue());
+        "}
+    }
+}

crates/linter/src/rule/mod.rs

@@ -222,6 +222,7 @@ define_rules! {
     NoClosingTag(no_closing_tag @ NoClosingTagRule),
     NoBooleanLiteralComparison(no_boolean_literal_comparison @ NoBooleanLiteralComparisonRule),
     NoBooleanFlagParameter(no_boolean_flag_parameter @ NoBooleanFlagParameterRule),
+    NoAssignInArgument(no_assign_in_argument @ NoAssignInArgumentRule),
     NoAssignInCondition(no_assign_in_condition @ NoAssignInConditionRule),
     NoAliasFunction(no_alias_function @ NoAliasFunctionRule),
     LowercaseTypeHint(lowercase_type_hint @ LowercaseTypeHintRule),

crates/linter/src/settings.rs

@@ -39,6 +39,7 @@ use crate::rule::LowercaseKeywordConfig;
 use crate::rule::LowercaseTypeHintConfig;
 use crate::rule::MiddlewareInRoutesConfig;
 use crate::rule::NoAliasFunctionConfig;
+use crate::rule::NoAssignInArgumentConfig;
 use crate::rule::NoAssignInConditionConfig;
 use crate::rule::NoBooleanFlagParameterConfig;
 use crate::rule::NoBooleanLiteralComparisonConfig;
@@ -227,6 +228,7 @@ pub struct RulesSettings {
     pub no_closing_tag: RuleSettings<NoClosingTagConfig>,
     pub no_boolean_literal_comparison: RuleSettings<NoBooleanLiteralComparisonConfig>,
     pub no_boolean_flag_parameter: RuleSettings<NoBooleanFlagParameterConfig>,
+    pub no_assign_in_argument: RuleSettings<NoAssignInArgumentConfig>,
     pub no_assign_in_condition: RuleSettings<NoAssignInConditionConfig>,
     pub no_alias_function: RuleSettings<NoAliasFunctionConfig>,
     pub lowercase_type_hint: RuleSettings<LowercaseTypeHintConfig>,

docs/tools/linter/rules/correctness.md

@@ -13,6 +13,7 @@ This document details the rules available in the `Correctness` category.
 | Constant Type | [`constant-type`](#constant-type) |
 | Identity Comparison | [`identity-comparison`](#identity-comparison) |
 | Invalid Open Tag | [`invalid-open-tag`](#invalid-open-tag) |
+| No Assign In Argument | [`no-assign-in-argument`](#no-assign-in-argument) |
 | No Assign In Condition | [`no-assign-in-condition`](#no-assign-in-condition) |
 | No Boolean Literal Comparison | [`no-boolean-literal-comparison`](#no-boolean-literal-comparison) |
 | No Empty Catch Clause | [`no-empty-catch-clause`](#no-empty-catch-clause) |
@@ -195,6 +196,40 @@ echo 'Hello, world!';
 ```
 
 
+## <a id="no-assign-in-argument"></a>`no-assign-in-argument`
+
+Detects assignments in function call arguments which can lead to unexpected behavior and make
+the code harder to read and understand.
+
+
+
+### Configuration
+
+| Option | Type | Default |
+| :--- | :--- | :--- |
+| `enabled` | `boolean` | `true` |
+| `level` | `string` | `"warning"` |
+
+### Examples
+
+#### Correct code
+
+```php
+<?php
+
+$x = 5;
+foo($x);
+```
+
+#### Incorrect code
+
+```php
+<?php
+
+foo($x = 5);
+```
+
+
 ## <a id="no-assign-in-condition"></a>`no-assign-in-condition`
 
 Detects assignments in conditions which can lead to unexpected behavior and make the code harder