feat(linter): add rule for no assignment in function parameters (#821)

Co-authored-by: Seifeddine Gmati <[email protected]>

Author: chrisopperwall-qz

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

@@ -4,6 +4,7 @@ pub mod identity_comparison;
 pub mod ineffective_format_ignore_next;
 pub mod ineffective_format_ignore_region;
 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;
@@ -21,6 +22,7 @@ pub use identity_comparison::*;
 pub use ineffective_format_ignore_next::*;
 pub use ineffective_format_ignore_region::*;
 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,281 @@
+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_span::Span;
+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 mago_syntax::ast::PartialArgument;
+
+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 default_enabled() -> bool {
+        false
+    }
+
+    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::ArgumentList, NodeKind::PartialArgumentList];
+
+        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>) {
+        match node {
+            Node::ArgumentList(list) => {
+                for argument in list.arguments.iter() {
+                    self.check_expression_for_assignment(ctx, argument.value(), list.span());
+                }
+            }
+            Node::PartialArgumentList(partial_list) => {
+                // Check expression for assignment on non-placeholder arguments
+                for argument in partial_list.arguments.iter() {
+                    let value = match argument {
+                        PartialArgument::Positional(pos) => &pos.value,
+                        PartialArgument::Named(named) => &named.value,
+                        _ => continue,
+                    };
+                    self.check_expression_for_assignment(ctx, value, partial_list.span());
+                }
+            }
+            _ => (),
+        }
+    }
+}
+
+impl NoAssignInArgumentRule {
+    fn check_expression_for_assignment<'arena>(
+        &self,
+        ctx: &mut LintContext<'_, 'arena>,
+        expression: &Expression<'arena>,
+        argument_list_span: Span,
+    ) {
+        if let Some(assignment) = get_assignment_from_expression(expression) {
+            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_list_span).with_message("In this argument list"),
+                    )
+                    .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_failure! {
+        name = multiple_arguments_with_assignment_in_partial_arguments,
+        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 = variable_used_as_argument_in_partial_arguments,
+        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

@@ -248,6 +248,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

@@ -42,6 +42,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;
@@ -234,6 +235,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

@@ -15,6 +15,7 @@ This document details the rules available in the `Correctness` category.
 | Ineffective Format Ignore Next | [`ineffective-format-ignore-next`](#ineffective-format-ignore-next) |
 | Ineffective Format Ignore Region | [`ineffective-format-ignore-region`](#ineffective-format-ignore-region) |
 | 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) |
@@ -297,6 +298,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` | `false` |
+| `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