Implement identical/equal comparisons on EnumCaseObjectType

[staabm]

implements the idea of #2095 (comment)

closes phpstan/phpstan#8042
closes phpstan/phpstan#8485
closes phpstan/phpstan#7721
closes phpstan/phpstan#4242
closes phpstan/phpstan#8240

re-implements #2095

[staabm]

I decided against implementing the check in ConstantConditionRuleHelper since the 2 rules in question which trigger the annoying errors, do not yet depend on this helper

[ondrejmirtes]

Sure, these rules needed update too, but I really need this in the helper. The idea is to stop reporting each "always true" in the last elseif and last match arm :) Also make sure to test that these "always true" are still being reported when in not-last elseif and match arm etc. :) Thank you.

[staabm]

what about

phpstan-src/tests/PHPStan/Rules/Comparison/data/strict-comparison.php

Lines 647 to 660 in 926ec9d

	public function doFoo()
	{
	$array = ['foofoofoofoofoofoofoo','foofoofoofoofoofoofob'];
	foreach ($array as $value) {
	if ('foofoofoofoofoofoofoo' === $value) {
	echo 'nope';
	} elseif ('foofoofoofoofoofoofob' === $value) {
	echo 'nop nope';
	}
	}
	}
	}

do we expect a error on line 654, or should this also be prevented? before this PR we get an error there.

[staabm]

The idea is to stop reporting each "always true" in the last elseif and last match arm :)

ok reading this again, I think it should not be reported. we want to stop reporting of each "always true"

[staabm]

this is a new variant of the existing test in the same file on line 654, with a additional else-if so we can see the error are reported when its not the last else-if

[staabm]

since this PR already fixes 4 open issues and AFAIK will fix the anoying errors the 1st implementation had I think we should consider merging it.

Taking the new visitor into account in ConstantConditionRuleHelper and all related rules can be done in a separate PR.. wdyt?

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

Something is broken here, the pipeline didn't run. Also I don't see the expected changes in src/Rules/Comparison/ConstantConditionRuleHelper.php, only an unused use.

[staabm]

Also I don't see the expected changes in src/Rules/Comparison/ConstantConditionRuleHelper.php

I can add it back, but I figured its not required for the current PR feature-set/fixes and we could leave it out for a next PR.

see #2105 (comment)

[ondrejmirtes]

To me to have all the "always true" changes together is more cohesive than only part of the changes for "always true" + enums fix.

To summarize, it'd be best to have two PRs:

1. All changes for "always true"
2. Enums comparison

But I'm fine to have everything in this single PR. Thanks.

[staabm]

lets see:

• MatchExpressionRule already prevents always-true in the last arm
• I have skimmed thru the issue tracker for open "always-true" errors, which are not yet tested here -> couldn't find one
• I have looked at Unreachable*Rules, but cannot think of a case which requires a new test/is missing

I am a bit lost, as I don't have an idea on how to move forwared here/what exactly needs to be done

[ondrejmirtes]

There's also:

• ConstantLooseComparisonRule
• Rules that use checkAlwaysTrueCheckTypeFunctionCall: ImpossibleCheckTypeStaticMethodCallRule, ImpossibleCheckTypeMethodCallRule, ImpossibleCheckTypeFunctionCallRule (which means that examples like these should not be reported https://phpstan.org/r/c7affa79-7bcf-4eb5-baff-5f5c4c2587b0)
• And finally the rules that use ConstantConditionRuleHelper::getBooleanType(). I don't need to come up with new examples - if you look at tests for these rules, and see something reported as "always true" and you put it into a last elseif, it shouldn't be reported anymore.

[staabm]

ok, lets see how you like the latest iteration.

I have worked thru all the mentioned rules. what a ride ;).

I think there is no need to adjust IfConstantConditionRule, TernaryOperatorConstantConditionRule, WhileLoopAlwaysFalseConditionRule, WhileLoopAlwaysTrueConditionRule.
all the other ones were extended with new test-cases and fixes

[staabm]

thinking about TernaryOperatorConstantConditionRule again..

this one might be doable:

class ConditionalAlwaysTrue
{
	public function doFoo(int $i, \stdClass $std)
	{
		if ($i > 0) {
		} elseif ($std ? 'foo' : 'bar') { // always-true should not be reported because last condition
		}

		if ($i > 0) {
		} elseif ($std ? 'foo' : 'bar') { // always-true should be reported, because another condition below
		} elseif (rand(0,1)) {
		}
	}
}

[ondrejmirtes]

Please also add a test for phpstan/phpstan#8240, thanks.

[staabm]

phpstan/phpstan#8240 is interessting. its not fixed by the PR as is.

the problem is similar but different. the root cause is that, $foo in this snippet should be resolved in a union of all cases, but is resolved into a object type and therefore the match-arm cases are not substracted it seems. 

https://phpstan.org/r/7f7b8b7b-f780-41f0-a1fb-02276482859b

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

Thank you! I have more improvements in mind, I started them already: 565fb0f

[staabm]

cool. if you want me to work on more, just open a new issue and give me an idea what we are targeting :)

[ondrejmirtes]

To match the current feature set, we need something like reportAlwaysTrueInLastCondition that's going to be false by default but it's going to be turned on in strict rules :) I'm not gonna open an issue, I'm gonna do it myself if no one beats me to it :)

[ondrejmirtes]

Also there should be a tip about this config option when such error is reported.

[ondrejmirtes]

And also: There is some complicated logic in UnreachableIfBranchesRule which we should rethink for bleedingEdge, because when "always true" is always reported, we might not need another rule to say the same thing...