Add failing test cases

[herndlm]

Took me forever to just create those tests. The reason was that I used only createStdClass() before and it never failed for the isNotSame. The reason for that was https://github.com/phpstan/phpstan-src/blob/1.4.8/src/Rules/Comparison/ImpossibleCheckTypeHelper.php#L191 because the expression createStdClass() was already specified in the scope from the isSame.. Which is why I added those strings.

The isNotSame was broken by #1046
But the isSame was already broken before. The problem is the true scope intersection of types just a bit further up.

Limiting it to variable nodes would break isSame(1, 3) expressions. Apparently limiting it to not being CallLike nodes seems to do the trick, but this feels still wrong to me.

Leaving this here for now and I'll come back to it after having a thought.

Refs phpstan/phpstan-phpunit#120 and phpstan/phpstan-webmozart-assert#122

[herndlm]

I think I found a simple fix by not further specifying types if both sides are CallLike nodes. I initially wanted to somehow check if they are "equal" nodes (e.g. having the same name and args or so), but not sure if this is really needed or makes sense.

[herndlm]

Need to investigate/fix callables that resolve to a constanttype still

[herndlm]

I was thinking of checking if the left and right types are constant types and keep specifying then even for CallLike nodes. But this adds complexity and does not really gain much. It would help to find out that e.g. calling foo() === foo() will always evaluate to true in case foo() has a constant type hint like 'foo'. I think this is too much, isn't it? I'm not sure how to handle union types there, basically I would need to check if the union is out of constants. Not sure if it's really worth it.
The bug introduced and another one with Identical comparison should be fixed already.

Nevermind, added the ConstantType behaviour I was thinking of, to me it's looking good now. Time to get more sleep..

[ondrejmirtes]

I've just submitted a failing test - a counter example that shows we can really do this only for the same Variables on both sides. So please update the code to reflect it.

The problem with CallLikes is more complicated. I don't think it's worth diving into this to actually solve it. Just to outline this - we could say that foo('a') === foo('a') is true if the function is pure, meaning hasSideEffects() is no(). But you'd have to inspect the whole chain - you might have $object->impureCall()->pureCall()->impureCall()->pureCall(), or even $object->pureCall(rand(0, 1)).

[herndlm]

Thank you. This is still tricky I think. Limiting the new parts to Variables only fixes the notSame bugs, yes. But there are still 2 problems

• the same bugs in L95 + L107, which were not introduced by me, are still there
• the type specification of $foo == (int) $foo is not working anymore, which, I guess, brings back the webmozart integerish bug :/ but this is a equals only thing which is different to identical, it might be fine there to specify the types, I need to check

[herndlm]

OK, I finally got something useful.

How I came to this
At first I tried to fix the true context / same case also via some if conditionals. Then I noticed that this is becoming weird because of too many exceptions for true/false context (e.g. Variable and Scalar, ..) and is not going to work. Then I realized that the false context if conditional had some logic inside handling ConstantBooleanType expression results (dealing with all those '1' === 2, array{1, 2} === array{3, 4}) that would also be working in the true context. I moved it out and up while moving the wrong true context handling down together with what I added in #1046 for the false context. And it now is only executed / further specifying for both contexts if either left or right expression is a variable. This seems to be enough to prevent weird problems while at the same time keeps my use case with a variable on one side and a cast on the other alive. Maybe it's fine to do it for other expression combis too, but I'm too scared to allow more :)
I hope this makes sense.

I tried to not mess around too much and keep the diff readable, but well, it's not optimal. With whitespace changes hidden it's okayish.

The last failing test is hopefully not related, but I'm not a 100% sure. Why would only this suddenly fail? The error looks valid at least, I'll take a look tomorrow in composer if I broke or fixed something.

Ah I forgot, running ImpossibleCheckTypeMethodCallRuleTest with the new test cases on master would return the following

1) PHPStan\Rules\Comparison\ImpossibleCheckTypeMethodCallRuleTest::testRule
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 78: Call to method ImpossibleMethodCall\Foo::isSame() with stdClass and stdClass will always evaluate to true.
 81: Call to method ImpossibleMethodCall\Foo::isNotSame() with stdClass and stdClass will always evaluate to false.
 84: Call to method ImpossibleMethodCall\Foo::isSame() with *NEVER* and stdClass will always evaluate to false.
+95: Call to method ImpossibleMethodCall\Foo::isSame() with stdClass and stdClass will always evaluate to true.
+98: Call to method ImpossibleMethodCall\Foo::isNotSame() with stdClass and stdClass will always evaluate to false.
 101: Call to method ImpossibleMethodCall\Foo::isSame() with 'foo' and 'foo' will always evaluate to true.
 104: Call to method ImpossibleMethodCall\Foo::isNotSame() with 'foo' and 'foo' will always evaluate to false.
+107: Call to method ImpossibleMethodCall\Foo::isSame() with mixed and mixed will always evaluate to true.
+110: Call to method ImpossibleMethodCall\Foo::isNotSame() with mixed and mixed will always evaluate to false.
 113: Call to method ImpossibleMethodCall\Foo::isSame() with array{} and array{} will always evaluate to true.
 116: Call to method ImpossibleMethodCall\Foo::isNotSame() with array{} and array{} will always evaluate to false.
 119: Call to method ImpossibleMethodCall\Foo::isSame() with array{1, 3} and array{1, 3} will always evaluate to true.

[ondrejmirtes]

Oh, I'm really happy with this, the diff makes total sense :)

The one new failure in composer repo (https://github.com/phpstan/phpstan-src/runs/5435424726?check_suite_focus=true) looks something that we changed here and is related because === is involved: https://github.com/composer/composer/blob/575fbfb53fcc2388916d554271c99c8281fea782/src/Composer/Package/Version/VersionGuesser.php#L108

It's related to the type alias: https://github.com/composer/composer/blob/575fbfb53fcc2388916d554271c99c8281fea782/src/Composer/Package/Version/VersionGuesser.php#L31

Can you please reproduce it here in a unit test? Even if it's right or wrong, we want it detected.

[herndlm]

Can you please reproduce it here in a unit test? Even if it's right or wrong, we want it detected.

I can do that, but what really confuses me is that I seem to be able to reproduce it on master already?
https://phpstan.org/r/8cdd9230-ede2-4f7f-9dbf-a37aff3329e3
It can't be something else, right? strange

But I'm gonna add the test and see how it behaves locally.

[herndlm]

Aah ok I think I got it, the error is different. Apparently the new code improves specification there and it is understood that version is not empty because it is the same as feature_version. Wait, other way around, that information is lost. And the non-empty-string error is baselined in https://github.com/composer/composer/blob/076925ebefaab6f13e5834f8025a728691aac175/phpstan/baseline.neon#L3754
If I run the new added test case here on master I get the following

1) PHPStan\Rules\Arrays\NonexistentOffsetInArrayDimFetchRuleTest::testRule
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 436: Cannot access offset 'foo' on 42.
 439: Cannot access offset 'foo' on 4.141.
 443: Cannot access offset 'foo' on array|int.
-504: Offset 'feature_pretty…' does not exist on array{version: string, commit: string|null, pretty_version: string|null, feature_version: non-empty-string, feature_pretty_version?: string|null}.
+504: Offset 'feature_pretty…' does not exist on array{version: non-empty-string, commit: string|null, pretty_version: string|null, feature_version: non-empty-string, feature_pretty_version?: string|null}.
 '

I guess this shows me that there are more expression nodes, additional to a simple Variable, where I can specify more. Let me check.

[herndlm]

Adapted, if I'm lucky the build should be green now. But I think the new $furtherSpecificationPossible function is strange still and I have the feeling it should be different somehow. Also, does the ArrayDimFetch open everything up for new problems again? I guess with the extra check here it should be fine. And should there be other things added as well? Or is it simpler the other way around and specify forbidden things? :/

I think I liked the "dumber" version with the limitation to Variable expressions more :)

I'll take another look later and at least will add multiple level dimfetch support with a NodeScopeResolver test. Also have an idea to make it stricter and safer. That's easy and will be better than the current workaround

[ondrejmirtes]

Yeah, the fix doesn't seem right :) What's the nature of the problem in Composer? Is it even a bug?

[herndlm]

In Composer there is a non-existant array offset access which is correctly reported by PHPStan. And baselined in Composer. But on master the === with the ArrayDimFetch nodes is specified while here it wasn't any more. And that changes the types PHPStan is aware of in the array which changes the error message in the baseline (non-empty-string vs string).
I pushed another adaption but could not make it as strict as I wanted it to be unfortunately. But it should be more correct now and work better with arrays IMO.

[herndlm]

I think that's the best I can come up with for now. Let me know what you think. Also, the NonexistentOffsetInArrayDimFetchRule test case could be removed again I guess.

[ondrejmirtes]

I took your tests and I'm trying an alternative approach: #1058

Let's see how it fares in CI :)

What I don't like in this PR is the instanceof of another Node besides Variable, seems too specific to just fix the Composer issue.

The main thing I want to do in my PR is "Consider === suspicious if the left and right side are the same AND they're Variables. If they're not the same, continue as before."

[ondrejmirtes]

I took your tests and I'm trying an alternative approach: #1058

Let's see how it fares in CI :)

What I don't like in this PR is the instanceof of another Node besides Variable, seems too specific to just fix the Composer issue.

The main thing I want to do in my PR is "Consider === suspicious if the left and right side are the same AND they're Variables. If they're not the same, continue as before."