Skip to content

Report missed loose constant-array comparisons in IfConditionRule#62

Merged
voku merged 14 commits intomasterfrom
copilot/add-random-if-conditions
Apr 28, 2026
Merged

Report missed loose constant-array comparisons in IfConditionRule#62
voku merged 14 commits intomasterfrom
copilot/add-random-if-conditions

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 28, 2026

  • Review the current loose array comparison diagnostics and affected expectations
  • Reinstall dependencies and run PHPUnit/PHPStan baseline in this fresh workspace
  • Replace the current array-vs-bool/null wording with guidance that explains the loose-comparison behavior is readable-but-confusing, not impossible
  • Add/adjust fixture coverage for array == null / != null readability cases
  • Update expected diagnostics for the refined wording
  • Re-run PHPUnit, PHPStan, and final validation

@voku
Copy link
Copy Markdown
Owner

voku commented Apr 28, 2026

/gemini review

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 28, 2026

Coverage Report for CI Build 25046427843

Coverage decreased (-0.3%) to 94.603%

Details

  • Coverage decreased (-0.3%) from the base build.
  • Patch coverage: 9 uncovered changes across 1 file (82 of 91 lines covered, 90.11%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
src/voku/PHPStan/Rules/IfConditionHelper.php 91 82 90.11%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 1019
Covered Lines: 964
Line Coverage: 94.6%
Coverage Strength: 21.13 hits per line

💛 - Coveralls

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new helper method extractSingleConstantArrayType to IfConditionHelper and integrates it into processEqualRules and processNotEqualRules to detect type mismatches involving constant arrays. It also adds several test cases and fixtures to verify these new rules and prevent regressions. The review feedback highlights significant issues with the error messages: the grammar is incorrect, and more importantly, the logic for 'Not Equal' comparisons is inverted, as disjoint types result in a condition that is always true rather than falsy. Additionally, there is an opportunity to refactor the duplicated logic for constant scalars and arrays into a shared method.

Comment on lines +271 to +284
$type_1ConstantArray = self::extractSingleConstantArrayType($type_1);
if (
$type_1ConstantArray !== null
&&
$type_2
&&
$type_2->accepts($type_1ConstantArray, true)->no()
) {
$errors[] = self::buildErrorMessage(
$origNode,
sprintf('Condition between %s and %s are falsy, please do not mix types.', $type_1ConstantArray->describe(VerbosityLevel::value()), $type_2->describe(VerbosityLevel::value())),
$cond->getAttribute('startLine')
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The logic for checking constant arrays is identical to the logic for constant scalars (lines 255-269). Consider refactoring this into a shared private method to improve maintainability and avoid duplication.

Additionally, the error message has a grammatical issue: "Condition ... are falsy" should be "Condition ... is falsy".

Finally, note that using accepts(..., true)->no() for loose equality (==) can be misleading. For example, [] == null and [] == false are true in PHP, but this rule will report them as "falsy" because null and false do not strictly accept an array. While this might be intended to enforce strictness, the wording "are falsy" is technically incorrect in those cases.

        $type_1ConstantArray = self::extractSingleConstantArrayType($type_1);
        if (
            $type_1ConstantArray !== null
            &&
            $type_2
            &&
            $type_2->accepts($type_1ConstantArray, true)->no()
        ) {
            $errors[] = self::buildErrorMessage(
                $origNode,
                sprintf('Condition between %s and %s is always false, please do not mix types.', $type_1ConstantArray->describe(VerbosityLevel::value()), $type_2->describe(VerbosityLevel::value())),
                $cond->getAttribute('startLine')
            );
        }

Comment on lines +383 to +396
$type_1ConstantArray = self::extractSingleConstantArrayType($type_1);
if (
$type_1ConstantArray !== null
&&
$type_2
&&
$type_2->accepts($type_1ConstantArray, true)->no()
) {
$errors[] = self::buildErrorMessage(
$origNode,
sprintf('Condition between %s and %s are falsy, please do not mix types.', $type_1ConstantArray->describe(VerbosityLevel::value()), $type_2->describe(VerbosityLevel::value())),
$cond->getAttribute('startLine')
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In addition to the duplication and grammar issues mentioned in processEqualRules, the error message here is logically incorrect for a "Not Equal" (!=) comparison. If the types are disjoint (accepts()->no()), the condition is actually always true, not "falsy". Reporting it as "falsy" will be confusing to users.

        $type_1ConstantArray = self::extractSingleConstantArrayType($type_1);
        if (
            $type_1ConstantArray !== null
            &&
            $type_2
            &&
            $type_2->accepts($type_1ConstantArray, true)->no()
        ) {
            $errors[] = self::buildErrorMessage(
                $origNode,
                sprintf('Condition between %s and %s is always true, please do not mix types.', $type_1ConstantArray->describe(VerbosityLevel::value()), $type_2->describe(VerbosityLevel::value())),
                $cond->getAttribute('startLine')
            );
        }

@voku voku marked this pull request as ready for review April 28, 2026 06:53
Copilot finished work on behalf of voku April 28, 2026 10:02
@voku voku merged commit 67b8504 into master Apr 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants