Merge constant types

[rajyan]

Refactoring mentioned in
#1919 (comment)
Now all types are merged with expressionTypes!

Now we are ready to start
phpstan/phpstan#8191

[rajyan]

I don't have enough context for test change 98a960b

[ondrejmirtes]

I think the next step is figure out what to do with TypeSpecifier and filterBySpecifiedTypes so that it also fills nativeExpressionTypes correctly.

Also - I'd love to see us to get rid of $conditionalExpressions and to have some new LateResolvableType as part of $expressionTypes too.

[rajyan]

I'm excited for the next steps!

[rajyan]

I noticed that ScopeFactory::create seems covered by BC break...? The failing test of larastan was cause by this...

[ondrejmirtes]

Also, I'm reviewing MutatingScope.php on top of this PR and there are some inconsistencies in handling $this->expressionTypes and $this->nativeExpressionTypes. For example in:

• specifyExpressionType (lines 3457, 3458) - one is assigned, another one is unset
• invalidateExpression - it goes over keys in $this->expressionTypes, does some logic and then tries to unset the keys on both expressionTypes and nativeExpressionTypes. I'd rather see the same foreach run twice separately.
• Same in invalidateMethodsOnExpression
• Sometimes large blocks of logic are unnecesarily duplicated for expressionTypes and nativeExpressionTypes, it'd be nicer to have some new private methods instead. (for example in processAlwaysIterableForeachScopeWithoutPollute)
• Private method addMoreSpecificTypes only handles expressionTypes and doesn't do anything with nativeExpressionTypes. It's only called from specifyExpressionType but I feel like something should be done here, especially when the method has Type $nativeType in the parameters.
• processClosureScope - same $variableType is used for both expressionTypes and nativeExpressionTypes which is certainly wrong.

I think these should also be cleaned up frist :)

[rajyan]

Thank you for the signposts.
Lot more fun things to do:)

[phpstan-bot]

This pull request has been marked as ready for review.

[rajyan]

Sorry
https://github.com/phpstan/phpstan-src/actions/runs/3355392157/jobs/5559633971
this test failure should be investigated

[rajyan]

and webmozart-assert test too
https://github.com/phpstan/phpstan-src/actions/runs/3355392157/jobs/5559637937

not sure why Assert::notEq(true, true) is failing while Assert::eq(true, true) is working.

[ondrejmirtes]

I have some idea - these language constants should stay the same, true is always true, but your refactoring probably made it not treated specially and thus from that point true is NeverType or something...

[rajyan]

Understood
#1938 (comment)

ConstFetch should be kept everywhere, even out of class

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

Another todo: $this in enterClosureBind and restoreOriginalScopeAfterClosureBind and enterClosureCall :)

[ondrejmirtes]

There are still some test failures :)

[ondrejmirtes]

About the BC break in ScopeFactory - yeah, it's annoying we can't change it as easily as this, I'm trying if Larastan can call it with less arguments here: larastan/larastan#1430

[ondrejmirtes]

My plan to address ScopeFactory is:

• Public ScopeFactory will have just one parameter in create() - ScopeContext
• MutatingScope will use new InternalScopeFactory with create() method parameters as they are now in ScopeFactory

[ondrejmirtes]

I need this for an optimization so I'm gonna merge it now :) Thank you very much!

[herndlm]

Sitting back and watching the step-by-step cleanups is fun. This looks like another good one 😊

[ondrejmirtes]

Would be nice to fix this one because it's now worse than before :) phpstan/phpstan#8034

[rajyan]

There are still some test failures :)

Sorry for it. I had to go sleep:)
Thank you for completing it.

[rajyan]

Would be nice to fix this one because it's now worse than before :) phpstan/phpstan#8034

Steps to fix

• fix DefinedConstantTypeSpecifyingExtension to detect ClassConstFetch and specify them correctly
• fix ClassConst assigned as ConstFetch in NodeScopeResolver https://github.com/rajyan/phpstan-src/blob/69d7e8185af250ce7315a2000b88a3d31f84cf3a/src/Analyser/NodeScopeResolver.php#L1439 assign them as ClassConstFetch
• skip ClassConstantRule if hasExpressionType->yes
• add expressionTypes handling in resolveType of ClassConstFetch

[ondrejmirtes]

This Composer failure is weird: https://github.com/phpstan/phpstan/actions/runs/3359189577/jobs/5566968926

 ------ ---------------------------------------------------------------------- 
  Line   tests/Composer/Test/Package/Archiver/ZipArchiverTest.php              
 ------ ---------------------------------------------------------------------- 
  91     Strict comparison using === between string|null and null will always  
         evaluate to false.                                                    
 ------ ----------------------------------------------------------------------