Add rule to improve performance checking on empty array

[JohJohan]

Feature Request

Just like https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#countarraytoemptyarraycomparisonrector i think it would be good performance to change empty() function call on array values to [] check example:

Diff

$array = [];

-if (empty($array)) {}; 
+if ([] === $array) {};
-if (!empty($array)) {}; 
+ if ([] !== $array) {};

This can only be done if we know that the variable is array. So for typed properties, function arguments or function return types

[samsonasik]

There is SimplifyEmptyArrayCheckRector that require is_array() check on the condition https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#simplifyemptyarraycheckrector

Feel free to create feature PR for it.

Some note:

• nullable array may need get type from Scope->getType() instead of NodeTypeResolver->getType(), see https://github.com/rectorphp/rector-src/pull/2935
• check combination with RemoveUnusedNonEmptyArrayBeforeForeachRector to avoid unnecessary refactor as will be removed after that.

[johanadivare]

@samsonasik am i allowed to extend the rule: SimplifyEmptyArrayCheckRector https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#simplifyemptyarraycheckrector as it describes what i also want only changes the fact to check on in_array() and checking on types array but the rule name also suggests that

[johanadivare]

I added some tests see https://github.com/rectorphp/rector-src/compare/rectorphp:main...JohJohan:7485?expand=1

[samsonasik]

I prefer for new rule for that, as Empty_ is not a FuncCall, also, validating array type may cause an issue without is_array() check like previous issue I mentioned, so different rule is better so that will be easier to locate when found a bug.

[staabm]

Isn't this a feature already provided by code style enforcing tools?

[johanadivare]

@staabm maybe you can give an example? And might still benefit rector as we also have SimplifyEmptyArrayCheckRector rule

[TomasVotruba]

@johanadivare I think new rule is a way to go here, as the construction is a bit different and empty has lot of traps we have to deal with also in other rules :). Could you send a PR?

[johanadivare]

I created https://github.com/rectorphp/rector-src/pull/3069

I did checkout the notes from @samsonasik https://github.com/rectorphp/rector/issues/7485#issuecomment-1251129232 but: note 1: I only check on ArrayType so if the type is nullable i dont replace it note 2: do you want me to write a test in combination with RemoveUnusedNonEmptyArrayBeforeForeachRector

[aragon999]

Sorry to bring up this old issue, but what I am wondering where the statement comes from, that the comparison with an empty array is faster?

Some time ago I found the following StackOverflow answer which includes a benchmark. This benchmark shows that in fact checking against an empty array is the slowest (which might be due to the fact that for this one needs to construct/allocate an empty array). I can reproduce similar results for PHP 8.2.

In general I agree, that one should probably not use the empty check and the bool cast, which has similar issues. I usually use the \count(...) > 0 approach, which is in my option the best compromise.

Have you performed any benchmarks to verify the statements, which tell otherwise? Or do I miss something else?

[samsonasik]

@aragon999 count will be slower on complex array structure, if you have array structure like this:

for ($i = 0; $i < 50; $i++) {
    $arr1[] = 0;
    $arr2[] = [
        [
            'test' => [
                'test' => [
                    'test',
                ],
            ],
        ],
    ];
    $arr3[] = md5($i);
    $arr4[] = $i % 2 ? $i : md5($i);
    $arr5[md5($i)] = $i;
}

[aragon999]

@samsonasik thank you for the feedback. Thanks I will do some further benchmarks for myself, probably disable the rule and use the appropriate method depending on the array.

[staabm]

You should use a profiler to decide whether its worth the effort.

In most applications it doesn't matter at all which variant you choose

[johanadivare]

As the docblock suggest: $array = []; if(empty($values))', '$array = []; if([] === $values) we check [] === $values Yoda style but the actual output will be if($values === []).

@TomasVotruba is this something i can fix with a pull request or is this something for a php ecs fixer?

I found this out after implementing it in our projects and i get allot off: 172 | ERROR | Use Yoda conditions when checking a variable against an expression to avoid an accidental assignment inside the condition statement.

[samsonasik]

@johanadivare you can use YodaStyleFixer from php-cs-fixer on ecs for that:

use PhpCsFixer\Fixer\ControlStructure\YodaStyleFixer;

// ...

    $ecsConfig->ruleWithConfiguration(YodaStyleFixer::class, ['identical' => \true]);