Improve conditionalExpressionTypes

[rajyan]

Steps left

• permit all expression types as conditional expression keys
• change the type guard logic by using the specified scope
• Improve createConditionalExpressions
• fix conditional expression invalidations
• use ExpressionTypeHolders for conditions
• change addConditionalExpressions first arg to expr

fixes phpstan/phpstan#3677
fixes phpstan/phpstan#5623
fixes phpstan/phpstan#5401 (the playground in the comment is another issue, we should open a new issue)
fixes phpstan/phpstan#7292
fixes phpstan/phpstan#8212

[phpstan-bot]

This pull request has been marked as ready for review.

[rajyan]

Sorry miss clicked

[rajyan]

I think we can clean up the conditional expression logic, and make it easier to migrate to LateResolvableTypes

[rajyan]

Memo

phpstan-src/tests/PHPStan/Analyser/data/bug-3677.php

Line 75 in 579b075

// assertType(Field::class, $first ? $first : $second);

https://github.com/rajyan/phpstan-src/blob/e4912564467d603ad64fff988c1249fd6d059fa6/tests/PHPStan/Analyser/data/dependent-variable-certainty.php#L261-L480

[rajyan]

Steps left

1. Improve createConditionalExpressions
2. fix conditional expression invalidations
3. use ExpressionTypeHolders for conditions
4. change addConditionalExpressions first arg to expr

[rajyan]

Another note,

We need to carefully separate assign and specifying

// when $b is conditional of $a = 'foo' => 'foo', $a = 'bar' => 'bar'
echo $b; // 'foo'|'bar'
if ($a === 'foo') {
  echo $b; // 'foo'
}
if ($a === 'bar') {
  echo $b; // 'bar'
}
if ($a === 'foo') {
  $a = 'bar';
  echo $b; // 'foo'
}
$a = 'foo';
echo $b;  // 'foo'|'bar'

Assign is a overwrite and needs invalidation of related resolved expression types, while specifying should be an intersection and no need to invalidate in my opinion.

[ondrejmirtes]

BTW just a minor nitpick - "temporal" is related to time, "temporary" is what you want here :)

[ondrejmirtes]

BTW there are some commented-out tests here https://github.com/phpstan/phpstan-src/blob/1.9.x/tests/PHPStan/Analyser/data/dependent-variable-certainty.php, maybe you could debug that as part of this or some future PR :)

[ondrejmirtes]

This fixes some issues: https://github.com/phpstan/phpstan-src/actions/runs/3420191417

Would be nice to add regression tests here for those which are fixed, to make sure they don't get broken in future iterations.

[rajyan]

@ondrejmirtes
The new issue bot CI looks so wonderful!👍
Thank you for it.

[rajyan]

Memo
SpecifyExpressionType and assignExpression with certanity maybe

[rajyan]

I'm gonna revert changes in createConditionalExpressions of mergeWith for now, because the pull request is getting to large... The changes in this PR can fix a lot of dependant type issues already:smile:

[rajyan]

What I did in this pull request

1. Separated specifying and assigning

• Only invalidate conditionalExpressions in assigning

2. Changed logic of filtering conditionalExpressions

• Use getType when to check matching expressions (to make conditionalExpressions work for all expressions)

3. Refactored the unsetExpression logic

[rajyan]

I think this pull request is mergable now.
The steps left are

• Use ExpressionTypeHolders for ConditionalExpressionTypes conditions (to get rid of exprStringToExpr)
• Make createConditionalExpressions to work for all expressions
• Make processBooleanConditionalTypes to work for all expressions
• improve addConditionalExpressions

and then, we can change ConditionalExpressions to LateResolvableType.

[rajyan]

The phpunit integration test failure seems related

[rajyan]

Found that specified types from getNewConditionalExpressionHolders is wrong in some cases.
I'll create another pull request and fix it there.

[rajyan]

What I'm saying wrong, is this

created from

if ($loaded === false || ($strict && $message !== '')) {

[rajyan]

Fixed another bug and I think this is finally ready!

[rajyan]

The error of rector is legit, but the error message should be improved.
https://github.com/rectorphp/rector-src/blob/71b1142ee41eceef21d9c3d3a1bf5bc4f67ad9d2/packages/NodeTypeResolver/PHPStan/TypeHasher.php#L122

! $currentType instanceof AliasedObjectType

is always false, because it's narrowed in the previous line.

[phpstan-bot]

This pull request has been marked as ready for review.

[herndlm]

Does it make sense to also add assertType() checks to the dependable type related fixed issues? I was looking forward to those in the diff :) The same issue files could be reused in NodeScopeResolverTest.

[ondrejmirtes]

Awesome, thank you! Looking to other improvements :)

[ondrejmirtes]

What doesn't sit well with me and what you haven't changed yet is the places in NodeScopeResolver where addConditionalExpressions are called. It's only for variables so it doesn't work generally for all expressions :)

[rajyan]

@herndlm
Yeah, I'll add them!

@ondrejmirtes
Created a ConditionalExpressionTypes todos to remember what to do, and make others can work on it too if someone is interested.
phpstan/phpstan#8360