Squiz.PHP.DisallowComparisonAssignment false positive for PHP 8 match expression

[FFirmenich]

Describe the bug
When using PHP 8 match() expression, the AssignedComparison validation shows a message, telling that this is not valid.

I don't think this is on purpose as this is the way how match should be used accoding to the PHP docs.
https://www.php.net/manual/de/control-structures.match.php

Code sample

<?php
test();

/**
 * @return void
 */
function test() {
    $food = 'cake';

    $returnValue = match (true) {
        $food === 'apple' => 'This food is an apple',
        $food === 'bar' => 'This food is a bar',
        $food === 'cake' => 'This food is a cake',
    };
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
    <rule ref="Squiz.PHP.DisallowComparisonAssignment"/>
</ruleset>

To reproduce
Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above...
2. Run phpcs test.php ...
3. See error message displayed

phpcs: Squiz.PHP.DisallowComparisonAssignment.AssignedComparison: The value of a comparison must not be assigned to a variable

Expected behavior
No warning. Instead it should be a valid usage of the match expression.

Versions (please complete the following information):

• OS: Windows 10
• PHP: 8.1
• PHPCS: 3.7.0
• Standard: Custom

Additional context
none

[gsherwood]

I'm not having any luck reproducing this issue with the code sample and ruleset supplied. I also tried PHP 7.4 and PHPCS 3.6.2 to see if it was an older issue but can't reproduce there as well. The only thing I can't test on is Windows, but that shouldn't impact things.

Looking at the code of the sniff, I'm not sure how the sample code would even think there is a comparison token in there.

Are you sure this sample file produces the error? If so, can you run phpcs wit the -vv flag and post the output.

[FFirmenich]

@gsherwood Thanks for your answer. You are right. My example was wrong. Sorry for that.

I edited the example in the start post.

[jrfnl]

@FFirmenich I've been able to reproduce your issue with the updated code sample. A fix has been pulled in PR #3624. Testing appreciated.

Just out of curiousity: why are you using the "match(true) with a strict comparison in the arms"-pattern ? This was common for switch as it always did a loose type comparison, but shouldn't be needed for match which does a strict type comparison...

The below effectively does exactly the same without the extra cognitive load:

function test() {
    $food = 'cake';

    $returnValue = match ($food) {
        'apple' => 'This food is an apple',
        'bar' => 'This food is a bar',
        'cake' => 'This food is a cake',
    };
}

[gsherwood]

@FFirmenich Thanks a lot for fixing the code sample, and big thanks to @jrfnl for actually fixing the bug.

[FFirmenich]

Just out of curiousity: why are you using the "match(true) with a strict comparison in the arms"-pattern ? This was common for switch as it always did a loose type comparison, but shouldn't be needed for match which does a strict type comparison...

@jrfnl Thanks for fixing it!
This is a commonly used design in Kotlin (there it's 'when'). You can make great things with this design like for example:

public static function checkConstraints(array $params): ?string {
    return match (true) {
        !$params['email'] => 'Keine E-Mail angegeben!',
        !$params['address'] => 'Keine Adresse angegeben!',
        !$params['phone'] => 'Keine Telefonnummer angegeben!',
        !$params['company'] => 'Kein Unternehmen angegeben!',
        !isset($params['isAdmin']) => 'Kein Benutzerstatus hinterlegt.',
        default => null
    };
}

Check Constraints

$constraintsError = $className::checkConstraints($params);
if ($constraintsError) {
    (new Response(
        status: ResponseStatus::ERROR,
        message: $constraintsError,
        data: [],
        httpStatus: HttpStatus::BAD_REQUEST
    ))->show();
}

[jrfnl]

@FFirmenich Thanks for letting me know. I can see how that could make sense. Looking at the above code sample, it feels to me like the error messages are "piece-mealed" (hiding one error behind another), which isn't always that user friendly, but that's completely out of scope for this ticket ;-)